Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix over-iterating buffer in hex string utility. #141

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+45 −18
Open

Fix over-iterating buffer in hex string utility. #141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions Sources/Crypto/Util/PrettyBytes.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,10 @@ extension DataProtocol {
var hexChars = [UInt8](repeating: 0, count: hexLen)
var offset = 0

self.regions.forEach { (_) in
for i in self {
hexChars[Int(offset * 2)] = itoh((i >> 4) & 0xF)
hexChars[Int(offset * 2 + 1)] = itoh(i & 0xF)
offset += 1
}
for i in self {
hexChars[Int(offset * 2)] = itoh((i >> 4) & 0xF)
hexChars[Int(offset * 2 + 1)] = itoh(i & 0xF)
offset += 1
}

return String(bytes: hexChars, encoding: .utf8)!
Expand Down
10 changes: 4 additions & 6 deletions Tests/CryptoTests/Utils/PrettyBytes.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,10 @@ extension DataProtocol {
var hexChars = [UInt8](repeating: 0, count: hexLen)
var offset = 0

self.regions.forEach { (_) in
for i in self {
hexChars[Int(offset * 2)] = itoh((i >> 4) & 0xF)
hexChars[Int(offset * 2 + 1)] = itoh(i & 0xF)
offset += 1
}
for i in self {
hexChars[Int(offset * 2)] = itoh((i >> 4) & 0xF)
hexChars[Int(offset * 2 + 1)] = itoh(i & 0xF)
offset += 1
}

return String(bytes: hexChars, encoding: .utf8)!
Expand Down
33 changes: 33 additions & 0 deletions Tests/CryptoTests/Utils/PrettyBytesTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
//===----------------------------------------------------------------------===//
//
// This source file is part of the SwiftCrypto open source project
//
// Copyright (c) 2019-2020 Apple Inc. and the SwiftCrypto project authors
// Licensed under Apache License v2.0
//
// See LICENSE.txt for license information
// See CONTRIBUTORS.md for the list of SwiftCrypto project authors
//
// SPDX-License-Identifier: Apache-2.0
//
//===----------------------------------------------------------------------===//
import Foundation
import XCTest

// Not sure if NSString-bridged "String.init(format:_:)" is available on Linux/Windows.
#if (os(macOS) || os(iOS) || os(tvOS) || os(watchOS)) && CRYPTO_IN_SWIFTPM_FORCE_BUILD_API
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this line needs to be:

#if (os(macOS) || os(iOS) || os(watchOS) || os(tvOS)) && CRYPTO_IN_SWIFTPM && !CRYPTO_IN_SWIFTPM_FORCE_BUILD_API
// Skip tests that require @testable imports of CryptoKit.
#else
#if (os(macOS) || os(iOS) || os(watchOS) || os(tvOS)) && !CRYPTO_IN_SWIFTPM_FORCE_BUILD_API
@testable import CryptoKit
#else
@testable import Crypto
#endif

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did look at what other tests wrote. But I wasn't checking whether to use CryptoKit or Crypto, rather I needed to know (guess) whether String.init(format:_:) was available in the SDK. On appleOS this API is provided for sure by bridging from NSString's initializer, but on Linux and Windows I couldn't know.

The code you suggested would make this test always available cross-platform. Let me verify if Swift's cross-platform Foundation port has this API. Otherwise I'll need to do something like sprintf to make it cross-platform.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also CryptoTests defines its own copy of PrettyBytes. It's weird because I though @testable import means that test target has access to everything non-private in the original Crypto module.

This would mean the PrettyBytesTests does not actually need to import anything, as the test only tests functions defined in the test target itself.

Can you clarify this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with only testing the one in the tests module, tbh.


@testable import Crypto

class PrettyBytesTests: XCTestCase {
func testHexString() {
let random = Data((1...64).map { _ in UInt8.random(in: UInt8.min...UInt8.max)})

let hexString = random.hexString
let target = random.map { String(format: "%02x", $0) }.joined()

XCTAssertEqual(hexString, target)
}
}

#endif
10 changes: 4 additions & 6 deletions Tests/_CryptoExtrasTests/Utils/BytesUtil.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,10 @@ extension DataProtocol {
let ptr = UnsafeMutablePointer<UInt8>.allocate(capacity: hexLen)
var offset = 0

self.regions.forEach { (_) in
for i in self {
ptr[Int(offset * 2)] = itoh((i >> 4) & 0xF)
ptr[Int(offset * 2 + 1)] = itoh(i & 0xF)
offset += 1
}
for i in self {
ptr[Int(offset * 2)] = itoh((i >> 4) & 0xF)
ptr[Int(offset * 2 + 1)] = itoh(i & 0xF)
offset += 1
}

return String(bytesNoCopy: ptr, length: hexLen, encoding: .utf8, freeWhenDone: true)!
Expand Down