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.

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

refactor: make s2n_stuffer_read_hex match s2n_stuffer_read #4726

Merged
merged 4 commits into from
Sep 5, 2024

Conversation

lrstewart
Copy link
Contributor

Description of changes:

@maddeleine called out that s2n_stuffer_read_hex has its arguments reversed compared to s2n_stuffer_read. In s2n_stuffer_read, the data is read from a stuffer and written to a blob. In s2n_stuffer_read_hex, the data is read from a blob and written to a stuffer.

The primary benefit of s2n_stuffer_read_hex reading from a blob (fixed size) and writing to a stuffer (potentially flexible size) is that the caller doesn't need to know the size of the data that a given hex string will produce. That math is easy (hex_size / 2), but I still liked that the method hid it.

However, I think maddeleine is right and that benefit doesn't outweigh the cost of the confusing discrepancy with s2n_stuffer_read. Switching the arguments in existing code wasn't difficult, suggesting that setting the size of the output blob isn't a serious barrier to using the method. We also often don't use growable or oversized stuffers in production code (outside of IO buffers), so the caller needs to know the size of the output anyway.

Testing:

Updated tests + cbmc proof.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Aug 22, 2024
@lrstewart lrstewart marked this pull request as ready for review August 22, 2024 20:42
@lrstewart lrstewart requested a review from goatgoose August 27, 2024 04:30
@lrstewart lrstewart enabled auto-merge (squash) September 4, 2024 17:45
@lrstewart lrstewart merged commit 5328a15 into aws:main Sep 5, 2024
36 checks passed
@lrstewart lrstewart deleted the hex_fix branch September 5, 2024 05:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants