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

Jump to bottom

Possible Over-Iteration in PrettyBytes #139

Open
VaslD opened this issue Jan 7, 2023 · 2 comments · May be fixed by #141
Open

Possible Over-Iteration in PrettyBytes #139

VaslD opened this issue Jan 7, 2023 · 2 comments · May be fixed by #141

Comments

@VaslD
Copy link

VaslD commented Jan 7, 2023 •
edited
Loading

I was looking for a not %02x way of converting binary to hex String, and I stumbled upon the implementation in PrettyBytes.swift.

swift-crypto/Sources/Crypto/Util/PrettyBytes.swift

Lines 45 to 51 in 9cc89f0

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
}
}

I'm not 100% sure about the Sequence conformance of non-contiguous DataProtocol, but it seemed to me that the self on line 46 should be the ignored parameter $0 (or maybe eliminate the region logic since looping over self most likely would have taken non-contiguous memory regions into account?), otherwise the code reads:

For each segment of the data buffer, loop over the entirety of data buffer and convert each byte to two hex chars.

Unless non-contiguous DataProtocol has non-stable indices and iterators, this code will over/re-iterate the data buffer.
@Lukasa
Copy link
Collaborator

Lukasa commented Jan 9, 2023

Yup, it sure does! Would you like to submit a patch to fix it? As a practical matter I think we can ignore the regions here, and just use the sequence conformance directly.

@VaslD VaslD linked a pull request Jan 10, 2023 that will close this issue
Open
5 tasks
@VaslD
Copy link
Author

VaslD commented Jan 10, 2023

PR done. Don't know how to construct a multi-region Data to test it yet, but it shouldn't get worse. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix over-iterating buffer in hex string utility. VaslD/SwiftCrypto
2 participants
@Lukasa @VaslD