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
[FLOSS H3] fix hashing collisions in specification token #4180
[FLOSS H3] fix hashing collisions in specification token #4180
Conversation
5d28874
to
f485785
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[FLOSS H3]
Thanks for your PR, that looks like a really great improvement!
If I execute the testsuite tests/ctest/test_sha-256.c
, which contains the test test_streaming_aware_of_order
and is already present in current master, in the codebase of the master branch, it executes successfully.
I copied the test from issue #4110 as it is and it also succeeds with the Elektra version from the current master branch.
However, when I tried to execute your newly added test test_hashesMetadata
in the testsuite tests/ctest/test_hash.c
, it failed.
When I ran the new test on the codebase of your PR-branch, it succeeded, as already suggested by the passed buildserver checks.
I think to state that the PR close the issues #4110 (comment in line 85 of src/libs/ease/hash.c
) can be misleading because the test that is mentioned in the description of that issue is already succeeding in the current master.
Maybe it would be better to create a new issue and link to it from the issue #4110 or at least post an update to the existing issue.
I don't know if the inclusion of the \0
for calculating the hash value breaks any other things, but at least all tests in Docker and on the buildservers ran successfully, so that looks quite good.
Regarding the commits, it would be good to combine ("squash") the three "fix formatting" commits to one commit and add a module-prefix like written in the PR-description.
Details of what you changed are in commit messages
(first line should havemodule: short statement
syntax)
A rebase with the current master branch is also necessary.
sha_256_write (&sha_256, keyName (currentKey), keyGetNameSize (currentKey) - 1); | ||
/** | ||
* Include NULL teminator in this and all following key/value strings, to avoid the following bug: | ||
* https://github.com/ElektraInitiative/libelektra/issues/4110 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reference to the issue #4110 could be misleading because the testcase that's in the description of the issue already succeeds in current master and your PR adds another test case.
I think this is because there is an error in the test. The test should check for inequality, not equality in the line:
I'll check this out when I am a bit less tired and edit the test if applicable. |
…minators Fixes ElektraInitiative#4110 Added tests to detect this issue as well as changed existing tests so they with the newly generated hashes
a5de6f4
to
b17c8c2
Compare
squashed and rebased |
LGTM. Please add the label "ready to merge" if it is ready to merge. |
Excellent job! 🎆 |
Basics
These points need to be fulfilled for every PR:
(added as entry in
doc/news/_preparation_next_release.md
whichcontains
_(my name)_
)Please always add something to the release notes.
(first line should have
module: short statement
syntax)close #X
, are in the commit messages.doc/news/_preparation_next_release.md
scripts/dev/reformat-all
If you have any troubles fulfilling these criteria, please write
about the trouble as comment in the PR. We will help you.
But we cannot accept PRs that do not fulfill the basics.
Checklist
Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar none of these points below
need to be checked.
(not in the PR description)
Review
Reviewers will usually check the following:
Labels
If you are already Elektra developer:
say that everything is ready to be merged.