User Details
- User Since
- Sep 13 2018, 4:03 AM (263 w, 4 d)
Tue, Sep 19
LGTM. I'm not very familiar with this area, but the change seems to be a very clean improvement.
Mon, Sep 18
Looks good, thanks for the improvements!
Tue, Sep 12
Thu, Sep 7
Today we learned that truth is different from falsehood...
Wed, Sep 6
I reviewed this change and collected my suggestions in comments.
Tue, Sep 5
Mon, Sep 4
Please add a testcase that demonstrates this issue (fails when your change in MemRegion.cpp isn't added) and shows that your commit fixes it.
Sep 1 2023
As I thought more about this commit I realized that I have two vague but significant concerns. I'm sorry that I wasn't able to describe these before you started to dig into the details of the heuristics.
Aug 31 2023
As a rough solution we can simply say that this checker ignores &arr[idx] (because it's just a different way of writing arr + idx) and only checks expressions where "real" dereference happens. This way the checker would won't emit false positives on past-the-end pointers and still report every out of bound memory access (including off-by-one errors).
I don't think that the &arr[N] issue is too serious: we can just increment the array extent when the parent expression of the array subscript operator is the unary operator &. If the past-the-end pointer ends up dereferenced later, the current code is sufficient to report it as a bug (as the checker monitors all dereferences).
Aug 30 2023
LGTM if the test results are good.
LGTM if the test results are also good.
Good direction of development, this will be useful for providing better bug reports (in addition to ensuring correct behavior some situations).
Thanks for the clarification! As I dig deeper I see that the functions that look like "let's taint a MemRegion" are in fact only affecting SymbolicRegions (targeting the corresponding symbol). This taint may be seen by the logic that checks the taintedness of the ElementRegion -- but as it's limited to symbolic regions it probably won't appear in the underflow test [which is skipped for symbolic regions in unknown space]. (There are also some calls which taint the pointee's/referred value -- that won't affect us.)
This seems to be a straightforward improvement over the current situation; LGTM if you test(ed) it on some real-life code (to ensure that it doesn't introduce a corner case that crashes).
I was thinking about adding an improvement like this for the sake of consistency, but I fear that this might cause a surprising amount of false positives. (Did you test it on larger codebases?)
Aug 28 2023
Yes, adding tests that demonstrate the current behavior is a good idea.
Thanks for the updates! I didn't spot any important issue, so I'm accepting this commit, but let's wait for the opinion of @steakhal before merging this.
LGTM. I'm not familiar with the Iterator checker family, but this is a very straightforward change.
Aug 25 2023
Aug 24 2023
Thanks for creating this commit, it's a useful improvement!
Aug 23 2023
The change looks promising, I only have minor remarks.
Aug 21 2023
Yup, there was a superfluous line break that was corrected by git clang-format; thanks for bringing it to my attention. As this is a very trivial change, I'll merge the fixed commit directly, without uploading it into Phabricator.
By the way, what would you think about putting the implementation of the two checkers (DeleteWithNonVirtualDtor, ArrayDelete) into two separate checker classes and placing the shared code into a common base class of these classes? I think this could be a significant simplification: we could avoid the manual management of the two checker names and bug types and register two regular checker objects instead of the one dummy prerequisite checker + two "fake" checker registrations that just toggle boolean flags.
A (somewhat delayed) ping to @steakhal because you asked me to tell you when I have the results. (I'm writing this only because I don't know whether you have seen the table that I uploaded on Friday.)
Aug 18 2023
The results on open-source projects are depressing, but acceptable. This checker is looking for a serious defect, so it doesn't find any true positives on stable releases; however it produces a steady trickle of false positives because the Clang SA engine regularly misinterprets complicated code. As the only effect of this patch is that it allows this checker to analyze more situations, it introduces no true positives and a manageable amount of false positives (on average ~1/project).
This checker is a straightforward, clean implementation of the SEI-CERT rule EXP51-CPP, it'll be a good addition to Static Analyzer. I'm satisfied with the implementation, although there might be others who have a more refined taste in C++ coding style. The only issue that I found is the misleading name of the "common ancestor" hidden checker that I mentioned in the inline comment.
Aug 16 2023
A few minutes ago I accidentally re-uploaded the previous version of the patch (diffs #550318 and #550703 are identical); now I'm fixing this by uploading the actually updated variant.
Updated the release notes. This could be the final version of this commit if you agree that it's good enough.
Minor clarification: these are not FPs, these are true positives with a bad error message. I was annoyed when I found this surprising bug on the almost-ready checker that I was working on; but I wouldn't say that this is an especially severe issue.
Aug 15 2023
(The release notes entry is still missing, I'll try to add it tomorrow.)
I verified that the checker handles the examples in the documentation correctly (and added them to the test suite). However, as I was tweaking the examples in the documentation, I accidentally found a situation where the checker produces a very surprising and arguably incorrect error message.
Aug 14 2023
I handled the inline comments, I'll check the example code and edit the release notes tomorrow.
I didn't upload the test results yet because right now there is some incompatibility between our local fork and the upstream. There is ongoing work to fix this (by rebasing our fork), but until it's done I can't use our automated tools to run the analysis with revisions that are close to the upstream main branch.
Aug 10 2023
Uploading a new version based on the reviews.
Thanks for the suggestions, I answered those where I had something to say and I'll implement the rest of them.
Aug 9 2023
I'll soon upload a refactored version.
Aug 8 2023
Why is this checker placed in the "unix" group (instead of e.g. "core")? As far as I understand it can check some POSIX-specific functions with a flag (that's off by default) but the core functionality checks platform-independent standard library functions.
Aug 4 2023
I'll try to upload results on open source projects, probably on Tuesday (I'm on vacation on Monday).
I'm abandoning this commit because it amalgamates several unrelated changes and I think it'd be better to handle them separately:
Note: this commit is split off from the larger commit https://reviews.llvm.org/D150446, which combined this simple improvement with other, more complex changes that had problematic side-effects.
Aug 3 2023
@steakhal Thanks for the review!
LGTM, with a little bikeshedding ;-)
Aug 2 2023
LGTM, I don't have anything significant to add.
Aug 1 2023
Jul 27 2023
Jul 26 2023
(Fix incorrect filename mark in the license header.)
I tested this commit on several open source projects (in the default mode Pedantic=false), the following table summarizes the results:
Jul 25 2023
Mostly LGTM, but I'd like to ask you to ensure consistent line wrapping (to e.g. 72 or 80 characters) in the free-flowing text that was added or rewritten by this commit. (I know that this is a minor question because the linebreaks won't be visible in the final output, but the raw markdown is itself a human-readable format and we should keep it reasonably clean.)
Jul 13 2023
(Just added some side remarks about string handling annoyances.)
Jul 10 2023
LGTM, thanks!
Ok, then I think you can finally merge this commit and its two predecessors. I'm happy to see that this improvement is complete.
Jul 7 2023
I looked through most of the open source results, and they look promising.
By the way here is a concrete example where this patch is needed to squash a false positive: https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=postgres_REL_13_0_stdclf_notetag_interesting_test_3&is-unique=on&report-hash=49939ae1896dff1ad782092b8534bd68&report-filepath=%2arelcache.c&report-id=1942889
I think the old "report when the value stored in an enum type is not equal to one of the enumerators" behavior would be useful as an opt-in checker that could be adopted by some projects; while the new "relaxed" version is too bland to be really useful. I'd suggest keeping the old behavior in the general case, eliminating the "obvious" false positives like std::byte (don't report types that have no enumerators) and moving this checker towards the optin group.
LGTM.
The commit looks good in general, I have a few minor suggestions and a serious question about the state transition bureaucracy.
Jul 5 2023
These code changes seem to be promising. Please upload results on the open source projects (like the ones that you uploaded previously), and I hope that after verifying those I we can finally merge this set of commits.
Jul 4 2023
I don't have concrete remarks, but I would like to note that I'm happy to see this cleanup :)
Jul 3 2023
Jun 30 2023
See results and their discussion on the review of the tightly connected follow-up commit D153776.
Thanks for attaching the test results! Unfortunately it seems that it was important to look at them, because I noticed that in the posgresql codebase there are many bug reports on calls like func1(func2(...), ...) where the checker does not print the expected "Assuming that 'func2' fails" note: fdopen/dup, fstat/fileno, isatty/fileno, another isatty/fileno, a third isatty/fileno, a second dup/fileno (this is not an exhaustive list, but I think I listed the majority of them).
When I tried to compile with gcc 7.5 after your fix commit (2f7d30dee826), I got another very similar compilation error from a different location and I pushed commit cec30e2b190b to fix that. After this second correction I can successfully run ninja check-clang-analysis with gcc7.5.
Thank you for the fix :) !
Jun 29 2023
LGTM if you rebase these changes onto the most recent version of D153612. Thanks for adding this interestingness check!
LGTM, but please wait until you can merge this together with D153776.
Thanks for the update! I have two minor remarks (string handling is complicated in C++...) but the code looks good otherwise.
LGTM.