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

hashes/sha2{24,56}: Remove static variables from sha256 #20116

Merged
merged 2 commits into from
Jan 16, 2024
Merged

hashes/sha2{24,56}: Remove static variables from sha256 #20116

merged 2 commits into from
Jan 16, 2024

Conversation

bergzand
Copy link
Member

Contribution description

This removes the static (thread-unsafe) variables from sha256 and hmac_sha256 to remove a potential footgun. The static variable is only used when the caller does not supply a pointer to store the digest and it is returned via the (undocumented) return value.

This commit removes this option and makes the digest argument mandatory.

Testing procedure

The tests-hashes unittest should still work:

main(): This is RIOT! (Version: 2024.01-devel-240-gc98f3-pr/sha256/no_static)
Help: Press s to start test, r to print it is ready
s
START
...........................................
OK (43 tests)

Issues/PRs references

Introduced in #124

@bergzand bergzand added the Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. label Nov 29, 2023
@bergzand bergzand requested a review from Teufelchen1 November 29, 2023 09:42
@github-actions github-actions bot added the Area: sys Area: System label Nov 29, 2023
@bergzand bergzand mentioned this pull request Nov 29, 2023
Merged
3 tasks
@bergzand bergzand changed the title hashes/sha256: Remove static variables from sha256 hashes/sha2{24,56}: Remove static variables from sha256 Nov 29, 2023
mguetschow
mguetschow reviewed Nov 29, 2023
View reviewed changes
sys/include/hashes/sha256.h Outdated
@@ -132,7 +132,7 @@ static inline void sha256_final(sha256_context_t *ctx, void *digest)
* be SHA256_DIGEST_LENGTH
* if digest == NULL, one static buffer is used
Copy link
Contributor

Choose a reason for hiding this comment

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

You'd need to adapt the documentation as well (in other places, too).

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I just did a quick scan for a doxygen @return and nothing showed up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Adjusted the docs

@bergzand bergzand force-pushed the pr/sha256/no_static branch from 4ef7b88 to d1b197a Compare November 29, 2023 10:02
@bergzand
Copy link
Member Author

And removed the return argument from hmac_sha256 also

mguetschow
mguetschow approved these changes Nov 29, 2023
View reviewed changes
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

LGTM then. Since this is an API breaking change, do we need to note that somewhere for the next release notes?

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 29, 2023
@riot-ci
Copy link

riot-ci commented Nov 29, 2023 •
edited
Loading

Murdock results

✔️ PASSED

434e564 hashes/sha224: Remove static variables from sha224

Success Failures Total Runtime
8101 0 8101 10m:57s

Artifacts

@maribu
Copy link
Member

maribu commented Dec 9, 2023

Please squash

@OlegHahm
Copy link
Member

OlegHahm commented Dec 9, 2023

I assume you have checked that no existing code calls the functions with passing a NULL pointer for digest?

@maribu
Copy link
Member

maribu commented Dec 9, 2023

I assume you have checked that no existing code calls the functions with passing a NULL pointer for digest?

That code would need to make use of the return value, which by changing the signature will result in a compilation error that Murdock should catch.

@OlegHahm
Copy link
Member

OlegHahm commented Dec 9, 2023

I assume you have checked that no existing code calls the functions with passing a NULL pointer for digest?

That code would need to make use of the return value, which by changing the signature will result in a compilation error that Murdock should catch.

Valid point. 😊

@mguetschow
Copy link
Contributor

ping @bergzand for squashing

@bergzand
hashes/sha256: Remove static variables from sha256
bac3f48 
This removes the static (thread-unsafe) variables from sha256 and
hmac_sha256 to remove a potential footgun. The static variable is only
used when the caller does not supply a pointer to store the digest and
it is returned via the (undocumented) return value.

This commit removes this option and makes the digest argument mandatory.
@bergzand
hashes/sha224: Remove static variables from sha224
434e564
@bergzand bergzand force-pushed the pr/sha256/no_static branch from cb9a210 to 434e564 Compare January 16, 2024 15:00
@bergzand
Copy link
Member Author

ping @bergzand for squashing

Done, sorry for the delay!

@mguetschow mguetschow enabled auto-merge January 16, 2024 15:08
@mguetschow mguetschow added this pull request to the merge queue Jan 16, 2024
Merged via the queue into RIOT-OS:master with commit 084dedc Jan 16, 2024
25 checks passed
@MrKevinWeiss MrKevinWeiss added this to the Release 2024.01 milestone Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mguetschow mguetschow mguetschow approved these changes

@Teufelchen1 Teufelchen1 Teufelchen1 approved these changes

@maribu maribu maribu approved these changes

Assignees
No one assigned
Labels
Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care.
Projects
None yet
Milestone
Release 2024.01
Development

Successfully merging this pull request may close these issues.
8 participants
@bergzand @riot-ci @maribu @OlegHahm @mguetschow @Teufelchen1 @benpicco @MrKevinWeiss