| Message ID | 20230519074108.401180-1-dmantipov@yandex.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | shared: avoid passing {NULL, 0} array to bsearch() | expand |
On Fri, May 19, 2023 at 10:41:08AM +0300, Dmitry Antipov wrote: >Fix the following warning reported by UBSan (as of gcc-13.1.1): > >shared/hash.c:244:35: runtime error: null pointer passed as >argument 2, which is declared to never be null > >Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu> >Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> >--- > shared/hash.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > >diff --git a/shared/hash.c b/shared/hash.c >index 7fe3f80..f90e22d 100644 >--- a/shared/hash.c >+++ b/shared/hash.c >@@ -241,12 +241,13 @@ void *hash_find(const struct hash *hash, const char *key) > .key = key, > .value = NULL > }; >- const struct hash_entry *entry = bsearch( >- &se, bucket->entries, bucket->used, >- sizeof(struct hash_entry), hash_entry_cmp); >- if (entry == NULL) >+ if (bucket->entries) { >+ const struct hash_entry *entry = >+ bsearch(&se, bucket->entries, bucket->used, >+ sizeof(struct hash_entry), hash_entry_cmp); >+ return entry ? (void *)entry->value : NULL; >+ } else I'd avoid the unbalanced brackets and replace the bucket->entries check with a return-early style. Would you be ok with me squashing this into your patch? diff --git a/shared/hash.c b/shared/hash.c index f90e22d..a87bc50 100644 --- a/shared/hash.c +++ b/shared/hash.c @@ -241,13 +241,15 @@ void *hash_find(const struct hash *hash, const char *key) .key = key, .value = NULL }; - if (bucket->entries) { - const struct hash_entry *entry = - bsearch(&se, bucket->entries, bucket->used, - sizeof(struct hash_entry), hash_entry_cmp); - return entry ? (void *)entry->value : NULL; - } else + const struct hash_entry *entry; + + if (!bucket->entries) return NULL; + + entry = bsearch(&se, bucket->entries, bucket->used, + sizeof(struct hash_entry), hash_entry_cmp); + + return entry ? (void *)entry->value : NULL; } int hash_del(struct hash *hash, const char *key) However I'm curious about this *runtime* error you went through. Does it have a backtrace? There are other places we call bsearch() passing bucket->entries, but that should be an imposibble runtime situation since we bail out on context creation if we can't create the hash table. thanks Lucas De Marchi
On 5/30/23 23:21, Lucas De Marchi wrote: > I'd avoid the unbalanced brackets and replace the bucket->entries > check with a return-early style. Would you be ok with me squashing this > into your patch? Sure as you wish. > However I'm curious about this *runtime* error you went through. Does it > have a backtrace? There are other places we call bsearch() passing > bucket->entries, but that should be an imposibble runtime situation > since we bail out on context creation if we can't create the hash table. This one is from something running under 'make check' (note I've added sleep() calls to have a time frame to attach gdb, so actual line numbers are shifted): (gdb) bt #0 0x000000000040924a in hash_find (hash=0x1034400, key=0x7fff71657d50 "btusb") at shared/hash.c:246 #1 0x0000000000405661 in kmod_pool_get_module (ctx=0x10342a0, key=0x7fff71657d50 "btusb") at libkmod/libkmod.c:403 #2 0x0000000000407cfe in kmod_module_new (ctx=0x10342a0, key=0x7fff71657d50 "btusb", name=0x7fff71657d50 "btusb", namelen=5, alias=0x0, aliaslen=0, mod=0x7fff71658d88) at libkmod/libkmod-module.c:270 #3 0x0000000000407f3f in kmod_module_new_from_name (ctx=0x10342a0, name=0x7fff71658d90 "btusb", mod=0x7fff71658d88) at libkmod/libkmod-module.c:341 #4 0x000000000040824b in kmod_module_new_from_loaded (ctx=0x10342a0, list=0x7fff71659df8) at libkmod/libkmod-module.c:1736 #5 0x000000000040262a in loaded_1 (t=0x40c0b8 <sloaded_10>) at testsuite/test-loaded.c:41 #6 0x0000000000402be9 in test_run_spawned (t=0x40c0b8 <sloaded_10>) at testsuite/testsuite.c:151 #7 0x0000000000404d3e in test_run (t=0x40c0b8 <sloaded_10>) at testsuite/testsuite.c:1080 #8 0x00000000004028ac in main (argc=3, argv=0x7fff7165a038) at testsuite/test-loaded.c:91 (gdb) bt full #0 0x000000000040924a in hash_find (hash=0x1034400, key=0x7fff71657d50 "btusb") at shared/hash.c:246 keylen = 5 hashval = 2921571348 pos = 20 bucket = 0x1034558 se = {key = 0x7fff71657d50 "btusb", value = 0x0} entry = 0x0 (More stack frames follow...) (gdb) p *((struct hash_bucket *)0x1034558) $1 = {entries = 0x0, used = 0, total = 0} That is, the bucket is non-NULL but empty, so bsearch() is called as bsearch([whatever], NULL, 0, [some more stuff]). On my system (Fedora 38 with glibc 2.37), bsearch() is declared as: extern void *bsearch (const void *__key, const void *__base, size_t __nmemb, size_t __size, __compar_fn_t __compar) __nonnull ((1, 2, 5)); So NULL '__base' causes the sanitizer to complain. Dmitry
On Wed, May 31, 2023 at 08:01:37AM +0300, Dmitry Antipov wrote: >On 5/30/23 23:21, Lucas De Marchi wrote: > >>I'd avoid the unbalanced brackets and replace the bucket->entries >>check with a return-early style. Would you be ok with me squashing this >>into your patch? > >Sure as you wish. > >>However I'm curious about this *runtime* error you went through. Does it >>have a backtrace? There are other places we call bsearch() passing >>bucket->entries, but that should be an imposibble runtime situation >>since we bail out on context creation if we can't create the hash table. > >This one is from something running under 'make check' (note I've added sleep() >calls to have a time frame to attach gdb, so actual line numbers are shifted): > >(gdb) bt >#0 0x000000000040924a in hash_find (hash=0x1034400, key=0x7fff71657d50 "btusb") at shared/hash.c:246 >#1 0x0000000000405661 in kmod_pool_get_module (ctx=0x10342a0, key=0x7fff71657d50 "btusb") > at libkmod/libkmod.c:403 >#2 0x0000000000407cfe in kmod_module_new (ctx=0x10342a0, key=0x7fff71657d50 "btusb", > name=0x7fff71657d50 "btusb", namelen=5, alias=0x0, aliaslen=0, mod=0x7fff71658d88) > at libkmod/libkmod-module.c:270 >#3 0x0000000000407f3f in kmod_module_new_from_name (ctx=0x10342a0, name=0x7fff71658d90 "btusb", > mod=0x7fff71658d88) at libkmod/libkmod-module.c:341 >#4 0x000000000040824b in kmod_module_new_from_loaded (ctx=0x10342a0, list=0x7fff71659df8) > at libkmod/libkmod-module.c:1736 >#5 0x000000000040262a in loaded_1 (t=0x40c0b8 <sloaded_10>) at testsuite/test-loaded.c:41 >#6 0x0000000000402be9 in test_run_spawned (t=0x40c0b8 <sloaded_10>) at testsuite/testsuite.c:151 >#7 0x0000000000404d3e in test_run (t=0x40c0b8 <sloaded_10>) at testsuite/testsuite.c:1080 >#8 0x00000000004028ac in main (argc=3, argv=0x7fff7165a038) at testsuite/test-loaded.c:91 ok, that makes sense. I had missed that code path. applied, thanks Lucas De Marchi > >(gdb) bt full >#0 0x000000000040924a in hash_find (hash=0x1034400, key=0x7fff71657d50 "btusb") at shared/hash.c:246 > keylen = 5 > hashval = 2921571348 > pos = 20 > bucket = 0x1034558 > se = {key = 0x7fff71657d50 "btusb", value = 0x0} > entry = 0x0 >(More stack frames follow...) > >(gdb) p *((struct hash_bucket *)0x1034558) >$1 = {entries = 0x0, used = 0, total = 0} > >That is, the bucket is non-NULL but empty, so bsearch() is called >as bsearch([whatever], NULL, 0, [some more stuff]). On my system >(Fedora 38 with glibc 2.37), bsearch() is declared as: > >extern void *bsearch (const void *__key, const void *__base, > size_t __nmemb, size_t __size, __compar_fn_t __compar) > __nonnull ((1, 2, 5)); > >So NULL '__base' causes the sanitizer to complain. > >Dmitry
diff --git a/shared/hash.c b/shared/hash.c index 7fe3f80..f90e22d 100644 --- a/shared/hash.c +++ b/shared/hash.c @@ -241,12 +241,13 @@ void *hash_find(const struct hash *hash, const char *key) .key = key, .value = NULL }; - const struct hash_entry *entry = bsearch( - &se, bucket->entries, bucket->used, - sizeof(struct hash_entry), hash_entry_cmp); - if (entry == NULL) + if (bucket->entries) { + const struct hash_entry *entry = + bsearch(&se, bucket->entries, bucket->used, + sizeof(struct hash_entry), hash_entry_cmp); + return entry ? (void *)entry->value : NULL; + } else return NULL; - return (void *)entry->value; } int hash_del(struct hash *hash, const char *key)