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 > > >
Re: [PATCH v2] analyzer: add allocation size checker
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) . > | | (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. > > 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 > > * analyzer.opt: Added Wanalyzer-allocation-size. [...snip...] > > gcc/ChangeLog: ...and here > > * doc/invoke.texi: Added Wanalyzer-allocation-size. > > gcc/testsuite/ChangeLog: ...and here > > * 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 [...snip...] > diff --git a/gcc/analyzer/analyzer.opt b/gcc/analyzer/analyzer.opt > index 4aea52d3a87..912def2faf2 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 >
