shared: Use unsigned char in hash calculation

kmod-project / kmod

kmod - Linux kernel module handling

GNU Lesser General Public License v2.1

50 stars 39 forks source link

shared: Use unsigned char in hash calculation #248

Open stoeckmann opened 2 weeks ago

stoeckmann commented 2 weeks ago

Spotted these while investigating https://github.com/kmod-project/kmod/issues/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.

codecov[bot] commented 2 weeks ago

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 :warning:

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%> (ø)`

lucasdemarchi commented 1 week ago

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 commented 1 week ago

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