https://github.com/NagyDonat requested changes to this pull request.
Thanks for the patch!
Your change seems to eliminate the crash described in the bug #180422, but I
would suggest moving the new logic from its current location (the
`identifyCall` method) to `evalStrcmpCommon`, the callback that is responsible
for handling `strcmp`-like functions (which I marked with an inline comment).
This way the generic `identifyCall` would remain as simple as possible, and
logic that is relevant for only a few functions is sequestered in the callback
method responsible for those particular functions. (IIRC many other callbacks
already start with analogous checks that validate the count and type of the
arguments -- but apparently this was left out by mistake.)
Note that in addition to `strcmp` and `strcasecmp` the callback
`evalStrcmpCommon` is also responsible for handling `strncmp` and `strncasecmp`
(which would also benefit from a parameter type check), so you would probably
need to change the "exactly two parameters" check to e.g. "at least two
parameters".
On the other hand as far as I see this checker doesn't do any modeling for
`strcoll` (as of now), so there is no need to validate its argument types. (I
don't see any immediate reason against modeling `strcoll` in this checker – I
would be open to accepting a separate PR that adds `strcoll` to the list of
modeled functions and extends `evalStrcmpCommon` to also cover that
locale-aware case [perhaps with yet another boolean flag].)
Also, please add a test that would reproduce the crash without your change –
and produce normal behavior [no report] when this PR is applied. You could e.g.
start by creating a test file `clang/test/Analysis/PR180422.c` (named after the
ID of the issue that you fix) with the following contents:
```
// RUN: %clang_analyze_cc1 -verify %s -analyzer-checker=core \
// RUN: -analyzer-checker=unix.StdCLibraryFunctions
// expected-no-diagnostics
// Intentionally non-standard declaration:
int strcmp(const char *s, char c);
void use_nonstandard_strcmp(const char* p) {
if (strcmp(p, '(') == 0) {
// ^ no-crash
}
}
```
Here the `RUN:` lines and `expected-no-diagnostics` are "magic" comments that
are recognized by our test systems (feel free to tweak the `RUN` lines if you
want to pass other arguments to `clang -cc1`); while `no-crash` is just a
customary comment to mark the place where a crash happened in an older version.
I wrote this test code without trying it out, so please check that it is
sufficient for triggering the crash (if you execute it without applying your
fix). You can run the tests of the static analyzer by building the target
`check-clang-analysis` (e.g. with if you use `ninja`, then `ninja
check-clang-analysis`).
If it works, then it would be probably nice to extend this test file with a
second analogous test function that uses e.g. uses a nonstandard `strncasecmp`
(to also cover the three-parameter situation).
https://github.com/llvm/llvm-project/pull/180544
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
