Skip to content

shared: Use unsigned char in hash calculation #248

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stoeckmann
Copy link
Contributor

Spotted these while investigating #26.

  • Use hash_superfast as intended (UBSAN gave a warning and yes, this modifies the hash result)
  • Improve handling of out of memory conditions
  • Add yet another fgets check (copy & paste code, last time I have just seen one of them)
  • Use proper format specifiers

Since these don't fix the issue but would basically lead all to a single PR, I've grouped them in a single one instead.

Copy link

codecov bot commented Nov 15, 2024

Codecov Report

Attention: Patch coverage is 71.42857% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
libkmod/libkmod-module.c 33.33% 3 Missing and 1 partial ⚠️
Files with missing lines Coverage Δ
libkmod/libkmod-elf.c 51.01% <100.00%> (ø)
libkmod/libkmod.c 51.95% <100.00%> (ø)
shared/hash.c 93.33% <100.00%> (ø)
tools/depmod.c 57.56% <100.00%> (ø)
libkmod/libkmod-module.c 53.56% <33.33%> (ø)

Copy link
Collaborator

@evelikov evelikov left a comment

Choose a reason for hiding this comment

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

Haven't looked at the hash_superfast commit and I suspect we really need a test for it to land.

The rest of the patches look great.

@lucasdemarchi
Copy link
Contributor

  • Use hash_superfast as intended (UBSAN gave a warning and yes, this modifies the hash result)

not very happy with changing the result... if we are changing I think we need to add some instrumentation and compare the amound of collisions we have with the current hash table size. Also... if we are changing, I'd rather add a pre step in which we (possibly) process the first element to get an aligned buffer on u16, so we don't have to use get_unaligned().

However most of our strings are really small, so I'm not sure it will make a noticeable difference.

lucasdemarchi pushed a commit that referenced this pull request Nov 21, 2024
If newly created module in kmod_module_new could not be added to hash,
return an error. Otherwise it is possible to create multiple independent
structs with the same name, which the hash set is supposed to prevent.

An error only occurs in out of memory conditions.

Signed-off-by: Tobias Stoeckmann <[email protected]>
Reviewed-by: Emil Velikov <[email protected]>
Link: #248
Signed-off-by: Lucas De Marchi <[email protected]>
lucasdemarchi pushed a commit that referenced this pull request Nov 21, 2024
Follow up of aad7c69, which fixes the same issue in another function.

Signed-off-by: Tobias Stoeckmann <[email protected]>
Reviewed-by: Emil Velikov <[email protected]>
Link: #248
Signed-off-by: Lucas De Marchi <[email protected]>
lucasdemarchi pushed a commit that referenced this pull request Nov 21, 2024
Use %zu for size_t, not %zd which would be for ssize_t.

Signed-off-by: Tobias Stoeckmann <[email protected]>
Reviewed-by: Emil Velikov <[email protected]>
Link: #248
Signed-off-by: Lucas De Marchi <[email protected]>
@lucasdemarchi
Copy link
Contributor

Applied all but the first. Please rebase and let's continue the discussion on how to land this fix.

Characters in key are supposed to be handled unsigned. Explicitly cast
key[2] in case the key contains 8-bit ASCII. Even though we cannot
support such keys in indices, we might still use them in module names
and tools like modinfo.

Signed-off-by: Tobias Stoeckmann <[email protected]>
@stoeckmann stoeckmann changed the title libkmod, tools: Various minor adjustments shared: Use unsigned char in hash calculation Nov 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants