Skip to content

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

Merged
mguetschow merged 2 commits intoRIOT-OS:masterfrom
bergzand:pr/sha256/no_static
Jan 16, 2024
Merged

hashes/sha2{24,56}: Remove static variables from sha256#20116
mguetschow merged 2 commits intoRIOT-OS:masterfrom
bergzand:pr/sha256/no_static

Conversation

@bergzand
Copy link
Copy Markdown
Member

@bergzand bergzand commented Nov 29, 2023

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
Copy Markdown
Contributor

@mguetschow mguetschow Nov 29, 2023

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
Copy Markdown
Member Author

@bergzand bergzand Nov 29, 2023

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
Copy Markdown
Member Author

@bergzand bergzand Nov 29, 2023

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
Copy Markdown
Member Author

bergzand commented Nov 29, 2023

And removed the return argument from hmac_sha256 also

mguetschow
mguetschow approved these changes Nov 29, 2023
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Member

maribu commented Dec 9, 2023

Please squash

@OlegHahm
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

mguetschow commented Dec 14, 2023

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
Copy Markdown
Member Author

bergzand commented Jan 16, 2024

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

@maribu maribu maribu approved these changes

@Teufelchen1 Teufelchen1 Teufelchen1 approved these changes

@mguetschow mguetschow mguetschow 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