Analyzer wiki page moved
On our wiki I've renamed: https://gcc.gnu.org/wiki/DavidMalcolm/StaticAnalyzer to: https://gcc.gnu.org/wiki/StaticAnalyzer since it's not just me working on the analyzer. I've updated all the internal links within the wiki accordingly; please update any external links you see. Thanks Dave
Re: [PATCH v2] analyzer: add allocation size checker
On Wed Jun 29, 2022 at 7:39 PM CEST, David Malcolm wrote: > On Wed, 2022-06-29 at 17:39 +0200, Tim Lange wrote: > > > Hi, > > Thanks for the updated patch. > > Overall, looks nearly ready; various nits inline below, throughout... > > > > > I've addressed most of the points from the review. > > * The allocation size warning warns whenever region_model::get_capacity > > returns > > something useful, i.e. also on statically-allocated regions. > > Thanks. Looks like you added test coverage for this in allocation- > size-5.c > > > * I added a new virtual function to the pending-diagnostic class, so > that it > > is possible to emit a custom region creation description. > > * The test cases should have a better coverage now. > > * Conservative struct handling > > > > The warning now looks like this: > > /path/to/main.c:9:8: warning: allocated buffer size is not a multiple of > > the pointee's size [CWE-131] [-Wanalyzer-allocation-size] > > 9 | int *iptr = ptr; > > |^~~~ > > ‘main’: events 1-2 > > | > > |8 | void *ptr = malloc((long unsigned int)n * sizeof(short)); > > | | ^ > > | | | > > | | (1) allocated ‘(long unsigned int)n * 2’ bytes > > here > > |9 | int *iptr = ptr; > > | | > > | || > > | |(2) assigned to ‘int *’ here; ‘sizeof(int)’ is ‘4’ > > | > > Looks great. > > > > > /path/to/main.c:15:15: warning: allocated buffer size is not a multiple of > > the pointee's size [CWE-131] [-Wanalyzer-allocation-size] > >15 | int *ptrw = malloc (sizeof (short)); > > | ^~~ > > ‘main’: events 1-2 > > | > > | 15 | int *ptrw = malloc (sizeof (short)); > > | | ^~~ > > | | | > > | | (1) allocated ‘2’ bytes here > > Looks a bit weird to be quoting a number here; maybe whenever the > expression is just a constant, print it unquoted? (though that could > be fiddly to implement, so can be ignored if it turns out to be) . Isn't the 'q' in '%qE' responsible for quoting. Using '%E' instead if m_expr is an INTEGER_CST works. Otherwise, I've left the quoting on the "'sizeof (int)' is '4'" note. I do think thata looks better than without. /path/to/main.c:16:15: warning: allocated buffer size is not a multiple of the pointee's size [CWE-131] [-Wanalyzer-allocation-size] 16 | int *ptrw = malloc (21 * sizeof (short)); | ^~~~ ‘main’: events 1-2 | | 16 | int *ptrw = malloc (21 * sizeof (short)); | | ^~~~ | | | | | (1) allocated 42 bytes here | | (2) assigned to ‘int *’ here; ‘sizeof (int)’ is ‘4’ | > > > > | | (2) assigned to ‘int *’ here; ‘sizeof (int)’ is > > ‘4’ > > | > > The only thing I couldn't address was moving the second event toward the > > lhs or > > assign token here. I tracked it down till get_stmt_location where it seems > > that > > the rhs is actually the location of the statement. Is there any way to get > > (2) > > to be focused on the lhs? > > Annoyingly, we've lost a lot of location information by the time the > analyzer runs. > > In theory we could special-case for when we have the def-stmt of the > SSA_NAME that's that default (i.e. initial) value of a VAR_DECL, and if > we see the write is there, we could use the DECL_SOUCE_LOCATION of the > VAR_DECL for the write, so that we'd get: > > | 15 | int *ptrw = malloc (sizeof (short)); > | |^~~~ ^~~ > | || | > | || (1) allocated ‘2’ bytes here > | |(2) assigned to ‘int *’ here; ‘sizeof (int)’ is ‘4’ > | > > which is perhaps slightly more readable. I'm not sure it's worth it > though. Hm, okay. I've left that out for now. > > > > > Otherwise, the patch compiled coreutils, openssh, curl and httpd without any > > false-positive (but none of them contained a bug found by the checker > > either). > > Great. > > > `make check-gcc RUNTESTFLAGS="analyzer.exp"` tests pass and as I just > > worked on > > the event splitting, the regression tests are yet to run. > > > > - Tim > > > > > > This patch adds an checker that warns about code paths in which a buffer is > > assigned to a incompatible type, i.e. when the allocated buffer size is not > > a > > multiple of the pointee's size. > > > > gcc/analyzer/ChangeLog: > > You should add a reference to the RFE bug to the top of the ChangeLog entries: > PR analyzer/105900 > > Please also add it to the commit message, in the form " [PR105900]"; > see the examples section twoards the end of > https://gcc.gnu.org/contribute.html#patches > > > >
[PATCH v3] analyzer: add allocation size checker [PR105900]
Hi, here's the updated patch that should address all the comments from the v2. - Tim This patch adds an checker that warns about code paths in which a buffer is assigned to a incompatible type, i.e. when the allocated buffer size is not a multiple of the pointee's size. 2022-07-30 Tim Lange gcc/analyzer/ChangeLog: PR analyzer/105900 * analyzer.opt: Added Wanalyzer-allocation-size. * checker-path.cc (region_creation_event::get_desc): Added call to new virtual function pending_diagnostic::describe_region_creation_event. * checker-path.h: Added region_creation_event::get_desc. * diagnostic-manager.cc (diagnostic_manager::add_event_on_final_node): New function. * diagnostic-manager.h: Added diagnostic_manager::add_event_on_final_node. * pending-diagnostic.h (struct region_creation): New event_desc struct. (pending_diagnostic::describe_region_creation_event): Added virtual function to overwrite description of a region creation. * region-model.cc (class dubious_allocation_size): New class. (capacity_compatible_with_type): New helper function. (class size_visitor): New class. (struct_or_union_with_inheritance_p): New helper function. (is_any_cast_p): New helper function. (region_model::check_region_size): New function. (region_model::set_value): Added call to region_model::check_region_size. * region-model.h (class region_model): New function check_region_size. * svalue.cc (region_svalue::accept): Changed to post-order traversal. (initial_svalue::accept): Likewise. (unaryop_svalue::accept): Likewise. (binop_svalue::accept): Likewise. (sub_svalue::accept): Likewise. (repeated_svalue::accept): Likewise. (bits_within_svalue::accept): Likewise. (widening_svalue::accept): Likewise. (unmergeable_svalue::accept): Likewise. (compound_svalue::accept): Likewise. (conjured_svalue::accept): Likewise. (asm_output_svalue::accept): Likewise. (const_fn_result_svalue::accept): Likewise. gcc/ChangeLog: PR analyzer/105900 * doc/invoke.texi: Added Wanalyzer-allocation-size. gcc/testsuite/ChangeLog: PR analyzer/105900 * gcc.dg/analyzer/pr96639.c: Changed buffer size to omit warning. * gcc.dg/analyzer/allocation-size-1.c: New test. * gcc.dg/analyzer/allocation-size-2.c: New test. * gcc.dg/analyzer/allocation-size-3.c: New test. * gcc.dg/analyzer/allocation-size-4.c: New test. * gcc.dg/analyzer/allocation-size-5.c: New test. Signed-off-by: Tim Lange --- gcc/analyzer/analyzer.opt | 4 + gcc/analyzer/checker-path.cc | 11 +- gcc/analyzer/checker-path.h | 2 +- gcc/analyzer/diagnostic-manager.cc| 61 +++ gcc/analyzer/diagnostic-manager.h | 4 + gcc/analyzer/pending-diagnostic.h | 20 + gcc/analyzer/region-model.cc | 370 ++ gcc/analyzer/region-model.h | 2 + gcc/analyzer/svalue.cc| 26 +- gcc/doc/invoke.texi | 14 + .../gcc.dg/analyzer/allocation-size-1.c | 116 ++ .../gcc.dg/analyzer/allocation-size-2.c | 155 .../gcc.dg/analyzer/allocation-size-3.c | 45 +++ .../gcc.dg/analyzer/allocation-size-4.c | 60 +++ .../gcc.dg/analyzer/allocation-size-5.c | 36 ++ gcc/testsuite/gcc.dg/analyzer/pr96639.c | 2 +- 16 files changed, 912 insertions(+), 16 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/allocation-size-3.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/allocation-size-4.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/allocation-size-5.c diff --git a/gcc/analyzer/analyzer.opt b/gcc/analyzer/analyzer.opt index 4aea52d3a87..1d612246a30 100644 --- a/gcc/analyzer/analyzer.opt +++ b/gcc/analyzer/analyzer.opt @@ -54,6 +54,10 @@ The minimum number of supernodes within a function for the analyzer to consider Common Joined UInteger Var(param_analyzer_max_enodes_for_full_dump) Init(200) Param The maximum depth of exploded nodes that should appear in a dot dump before switching to a less verbose format. +Wanalyzer-allocation-size +Common Var(warn_analyzer_allocation_size) Init(1) Warning +Warn about code paths in which a pointer to a buffer is assigned to an incompatible type. + Wanalyzer-double-fclose Common Var(warn_analyzer_double_fclose) Init(1) Warning Warn about code paths in which a stdio FILE can be closed more than once. diff --git a/gcc/analyzer/checker-path.cc b/gcc/analyzer/checker-path.cc index 0133dc94137..953e192cd55 100644 --- a/gcc/analyzer/checker-path.cc ++
gcc-10-20220630 is now available
Snapshot gcc-10-20220630 is now available on https://gcc.gnu.org/pub/gcc/snapshots/10-20220630/ and on various mirrors, see http://gcc.gnu.org/mirrors.html for details. This snapshot has been generated from the GCC 10 git branch with the following options: git://gcc.gnu.org/git/gcc.git branch releases/gcc-10 revision 7471cc92b7c7907a0e5499b49fecb3e1a8e2c926 You'll find: gcc-10-20220630.tar.xz Complete GCC SHA256=3edc526b0e226e2c9cf8424222f0b21b9e4e05fff7b68e444f88d2c77dfef33e SHA1=8323986f66d983d785df84a6a8fd2b0c2ec5365c Diffs from 10-20220623 are available in the diffs/ subdirectory. When a particular snapshot is ready for public consumption the LATEST-10 link is updated and a message is sent to the gcc list. Please do not use a snapshot before it has been announced that way.
Re: [PATCH v3] analyzer: add allocation size checker [PR105900]
On Fri, 2022-07-01 at 00:11 +0200, Tim Lange wrote: > Hi, > > here's the updated patch that should address all the comments from the > v2. > > - Tim > > This patch adds an checker that warns about code paths in which a > buffer is > assigned to a incompatible type, i.e. when the allocated buffer size is > not a > multiple of the pointee's size. > > 2022-07-30 Tim Lange > > gcc/analyzer/ChangeLog: > > PR analyzer/105900 > * analyzer.opt: Added Wanalyzer-allocation-size. > * checker-path.cc (region_creation_event::get_desc): Added call > to new > virtual function > pending_diagnostic::describe_region_creation_event. > * checker-path.h: Added region_creation_event::get_desc. > * diagnostic-manager.cc > (diagnostic_manager::add_event_on_final_node): > New function. > * diagnostic-manager.h: > Added diagnostic_manager::add_event_on_final_node. > * pending-diagnostic.h (struct region_creation): New event_desc > struct. > (pending_diagnostic::describe_region_creation_event): Added > virtual > function to overwrite description of a region creation. > * region-model.cc (class dubious_allocation_size): New class. > (capacity_compatible_with_type): New helper function. > (class size_visitor): New class. > (struct_or_union_with_inheritance_p): New helper function. > (is_any_cast_p): New helper function. > (region_model::check_region_size): New function. > (region_model::set_value): Added call to > region_model::check_region_size. > * region-model.h (class region_model): New function > check_region_size. > * svalue.cc (region_svalue::accept): Changed to post-order > traversal. > (initial_svalue::accept): Likewise. > (unaryop_svalue::accept): Likewise. > (binop_svalue::accept): Likewise. > (sub_svalue::accept): Likewise. > (repeated_svalue::accept): Likewise. > (bits_within_svalue::accept): Likewise. > (widening_svalue::accept): Likewise. > (unmergeable_svalue::accept): Likewise. > (compound_svalue::accept): Likewise. > (conjured_svalue::accept): Likewise. > (asm_output_svalue::accept): Likewise. > (const_fn_result_svalue::accept): Likewise. > > gcc/ChangeLog: > > PR analyzer/105900 > * doc/invoke.texi: Added Wanalyzer-allocation-size. > > gcc/testsuite/ChangeLog: > > PR analyzer/105900 > * gcc.dg/analyzer/pr96639.c: Changed buffer size to omit warning. > * gcc.dg/analyzer/allocation-size-1.c: New test. > * gcc.dg/analyzer/allocation-size-2.c: New test. > * gcc.dg/analyzer/allocation-size-3.c: New test. > * gcc.dg/analyzer/allocation-size-4.c: New test. > * gcc.dg/analyzer/allocation-size-5.c: New test. > > Signed-off-by: Tim Lange Thanks for the v3 patch. Content-wise, the v3 patch looks ready to me, though there's something weird with the formatting of the ChangeLog entry for pr96639.c in the commit message - does the patch pass: ./contrib/gcc-changelog/git_check_commit.py HEAD ? (this script gets run server-side on our git repository, and it won't let you push a patch unless the script passes) You didn't specify to what extent you've tested it. If you've successfully bootstrapped gcc with this patch applied, and run the test suite with no regressions, then this is OK to push to trunk. [...snip...] Thanks Dave
