analyzer: fix ICE in print_mem_ref [PR98969]
authorDavid Malcolm <dmalcolm@redhat.com>
Fri, 12 Feb 2021 01:31:28 +0000 (20:31 -0500)
committerDavid Malcolm <dmalcolm@redhat.com>
Fri, 12 Feb 2021 01:32:10 +0000 (20:32 -0500)
commit467a48205279cab368dbeb02879bbbbe4b721516
tree5e4231dc20a61c14c4591884117b987eb4ec969a
parent0c5cdb31bd423f650aaa3917cc21de0cde91e0fc
analyzer: fix ICE in print_mem_ref [PR98969]

PR analyzer/98969 and PR analyzer/99064 describes ICEs, in both cases
within print_mem_ref, when falsely reporting memory leaks - though it
is possible to generate the ICE on other diagnostics (which I added
in one of the test cases).

This patch fixes the ICE, leaving the fix for the leak false positives
as followup work.

The analyzer uses region_model::get_representative_path_var and
region_model::get_representative_tree to map back from its svalue
and region classes to the tree type used by the rest of the compiler,
and, in particular, for diagnostics.

The root cause of the ICE is sloppiness about types within those
functions; specifically when casts were stripped off svalues.  To
track these down I added wrapper functions that verify that the
types of the results are correct, and in doing so found various
other type-safety issues, which the patch also fixes.

Doing so led to various changes in diagnostics messages due to
more accurate types, but I felt that these changes weren't
desirable.
For example, the warning at CVE-2005-1689-minimal.c line 48
which expects:
  double-'free' of 'inbuf.data'
changed fo
  double-'free' of '(char *)inbuf.data'

So I added stripping of top-level casts where necessary to avoid
cluttering diagnostics.

Finally, the more accurate types led to worse results from
readability_comparator, where e.g. the event message at line 50
of sensitive-1.c regressed from the precise:
  passing sensitive value 'password' in call to 'called_by_test_5' from 'test_5'
to the vaguer:
  calling 'called_by_test_5' from 'test_5'
This was due to erroneously picking the initial value of "password"
in the caller frame as the best value within the *callee* frame, due to
"char *" vs "const char *", which confuses the logic for tracking values
that pass along callgraph edges.  The patch fixes this by combining the
readability tests for tree and stack depth, rather than performing
them in sequence, so that it favors the value in the deepest frame.

As noted above, the patch fixes the ICEs, but does not fix the
leak false positives.

gcc/analyzer/ChangeLog:
PR analyzer/98969
* engine.cc (readability): Add names for the various arbitrary
values.  Handle NOP_EXPR and INTEGER_CST.
(readability_comparator): Combine the readability tests for
tree and stack depth, rather than performing them sequentially.
(impl_region_model_context::on_state_leak): Strip off top-level
casts.
* region-model.cc (region_model::get_representative_path_var): Add
type-checking, moving the bulk of the implementation to...
(region_model::get_representative_path_var_1): ...here.  Respect
types in casts by recursing and re-adding the cast, rather than
merely stripping them off.  Use the correct type when handling
region_svalue.
(region_model::get_representative_tree): Strip off any top-level
cast.
(region_model::get_representative_path_var): Add type-checking,
moving the bulk of the implementation to...
(region_model::get_representative_path_var_1): ...here.
* region-model.h (region_model::get_representative_path_var_1):
New decl
(region_model::get_representative_path_var_1): New decl.
* store.cc (append_pathvar_with_type): New.
(binding_cluster::get_representative_path_vars): Cast path_vars
to the correct type when adding them to *OUT_PVS.

gcc/testsuite/ChangeLog:
PR analyzer/98969
* g++.dg/analyzer/pr99064.C: New test.
* gcc.dg/analyzer/pr98969.c: New test.
gcc/analyzer/engine.cc
gcc/analyzer/region-model.cc
gcc/analyzer/region-model.h
gcc/analyzer/store.cc
gcc/testsuite/g++.dg/analyzer/pr99064.C [new file with mode: 0644]
gcc/testsuite/gcc.dg/analyzer/pr98969.c [new file with mode: 0644]