Skip to content
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.

Sign up for GitHub

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

Fix clang static analyzer false positive #180

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

Fix clang static analyzer false positive #180

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/uthash.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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); \
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

I can take a stab

Copy link
Collaborator

Choose a reason for hiding this comment

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

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).

HH_FROM_ELMT((head)->hh.tbl, _hd_hh_del->prev)->next = _hd_hh_del->next; \
} else { \
DECLTYPE_ASSIGN(head, _hd_hh_del->next); \
Expand Down