Fix clang static analyzer false positive #180
Conversation
Fixes troydhanson#128. Clang static analyzer was thinking that it's possible to free() list head without updating head pointer. This assert assures static analyzer that branch without head pointer update could be taken only when we are freeing non-head element.
|
Hello, |
| @@ -473,6 +473,7 @@ do { | |||
| (head)->hh.tbl->tail = HH_FROM_ELMT((head)->hh.tbl, _hd_hh_del->prev); \ | |||
| } \ | |||
| if (_hd_hh_del->prev != NULL) { \ | |||
| assert(&(head)->hh != _hd_hh_del); \ | |||
There was a problem hiding this comment.
My concern with this one is that it's introducing assert (and <assert.h>) into "uthash.h", which historically hasn't been the case. I know "utlist.h" uses <assert.h>, but "uthash.h" itself has a lot more users than "utlist.h", so I'm leery of introducing new header dependencies (even if they are C89 standard) and especially new runtime dependencies (e.g. you're introducing a hidden dependency on the libc's abort function, right?).
Is there any way to silence scan-build's warning here with core-language features instead of assert?
I even wonder if we could just change line 475 from if (_hd_hh_del->prev != NULL) to if (_hd_hh_del != &(head)->hh) — does that work? It shouldn't really be any less efficient, since we have to load (head)->hh on line 476 anyway.
There was a problem hiding this comment.
Btw, I'd also be extremely interested in PRs against our .travis.yml file so that we could actually test the scan-build build against regressions in this area. Any takers?
There was a problem hiding this comment.
I even wonder if we could just change line 475 from if (_hd_hh_del->prev != NULL) to if (_hd_hh_del != &(head)->hh) — does that work?
Nope, this introduced a new warning warning: Dereference of null pointer [clang-analyzer-core.NullDereference].
Using the assert and including <assert.h> in uthash.h solved the problem, can we go ahead with that fix?
There was a problem hiding this comment.
including <assert.h> in uthash.h solved the problem, can we go ahead with that fix?
No.
Any volunteers to add scan-build into .travis.yml?
There was a problem hiding this comment.
Now that I'm able to reproduce locally (thanks @chaitu-tk!) I see that we could silence the false positives on test15, test40, and test59 by adding assert((delptr) != (head)) at the end of HASH_DELETE — i.e., "after deleting an item from the hash table, the deleted item is no longer in the table at all, let alone at the front of the table."
Except that this breaks test18, because we want to permit HASH_DEL(table, table) to mean "delete the first item in the table." And anyway, I still don't want to add a dependency on <assert.h> to uthash.
I've just fixed some low-hanging fruit in 45af88c and f0e1bd9 (which I'll merge up to this repo once Travis passes).
Fixes #128. Clang static analyzer was thinking that it's possible to
free() list head without updating head pointer. This assert assures
static analyzer that branch without head pointer update could be taken
only when we are freeing non-head element.