LGTM. I'm not very familiar with this area, but the change seems to be a very clean improvement.

Looks good, thanks for the improvements!

Today we learned that truth is different from falsehood...

I reviewed this change and collected my suggestions in comments.

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.

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.

In D159107#4630764, @steakhal wrote:

In D159107#4630573, @donat.nagy wrote:

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).

My instinct suggests that there is more behind the curtain.

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).

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).

In D159105#4627785, @steakhal wrote:

In D159105#4627724, @donat.nagy wrote:

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?)

I'm just done backporting the first batch to our fork and scheduling a large-scale measurement.
But my gut instinct suggests there won't be too many new reports, as I believe "tainted" pointers shouldn't be really a thing, except for really exotic cases, like scanf a pointer - which is dubious at best in presence of ASLR.

As far as I understand (correct me if I'm wrong), there are situations when TaintChecker marks the memory region of e.g. a returned string as tainted to mark that the contents of the region are unreliable. (There may be other more exotic situations when even the pointer value or the extent becomes unreliable e.g. your testcases where you scanf into a pointer.)

Taint can only bind to symbols, thus a memregion per-say cannot be tainted, only the "conjured address of it" or the "symbol bound to the pointee's/referred lvalue" can be tainted.

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?)

Yes, adding tests that demonstrate the current behavior is a good idea.

In D158707#4621135, @steakhal wrote:

Oh, so we would canonicalize towards a signed extent type. I see. I think I'm okay with that.
However, I think this needs a little bit of polishing and testing when the region does not point to the beginning of an array or object, but rather inside an object, or like this:

int nums[] = {1,2,3};
char *p = (char*)&nums[1];

clang_analyzer_dumpExtent(p);
clang_analyzer_dumpElementCount(p);
++p;
clang_analyzer_dumpExtent(p);
clang_analyzer_dumpElementCount(p);
++p;
int *q = (int*)p;
clang_analyzer_dumpExtent(q);
clang_analyzer_dumpElementCount(q);

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.

Thanks for creating this commit, it's a useful improvement!

In D158499#4608974, @danix800 wrote:

One of the observable issue with inconsistent size type is

void clang_analyzer_eval(int);

typedef unsigned long long size_t;
void *malloc(unsigned long size);
void free(void *);

void symbolic_longlong_and_int0(long long len) {
  char *a = malloc(5);
  (void)a[len + 1]; // no-warning
  // len: [-1,3]
  clang_analyzer_eval(-1 <= len && len <= 3); // expected-warning {{TRUE}}
  clang_analyzer_eval(0 <= len);              // expected-warning {{UNKNOWN}}
  clang_analyzer_eval(len <= 2);              // expected-warning {{UNKNOWN}}
  free(a);
}

which is extracted from clang/test/Analysis/array-bound-v2-constraint-check.c,
with DynamicMemoryModeling turned on,
the second warning does not hold anymore: clang_analyzer_eval(0 <= len);
will be reported as TRUE which is not expected.

DynamicMemoryModeling will record the extent of allocated memory as 5ULL,
ArrayBoundV2 will do len + 1 < 5ULL assumption, simplified to len < 4ULL,
which casts len to unsigned, dropping -1, similar to

void clang_analyzer_eval(int);

void test(int len) {
  if (len >= -1 && len <= 4U) { // len is promoted into unsigned, thus can never be negative
      clang_analyzer_eval(0 <= len);              // TRUE
  }
}

The change looks promising, I only have minor remarks.

In D158156#4604242, @steakhal wrote:

One more thing to mention. Its usually illadvised to rename files or do changes that would seriously impact git blame, unless we have a really good reason doing so.
Aesthetics usually don't count one, especially if the name is not user-facing. However, maintainability is another axis, thus as it's always, not black and white.

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.)

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.

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.

In D156312#4589251, @steakhal wrote:

I don't think we should do anything about it unless it's frequent enough.
Try to come up with a heuristic to be able to measure how often this happens, if you really care.
Once you have a semi-working heuristic for determining if that bugpath suffers from this, you can as well use it for marking the bugreport invalid if that's the case to lean towards suppressing those FPs.

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.

(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.

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.

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.

I'll soon upload a refactored version.

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.

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.

@steakhal Thanks for the review!

LGTM, with a little bikeshedding ;-)

LGTM, I don't have anything significant to add.

In D156312#4537200, @NoQ wrote:

(4) UBOR exhibits buggy behavior in code that involves cast expressions

Hmm, what makes your check more resilient to our overall type-incorrectness?

(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:

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.)

(Just added some side remarks about string handling annoyances.)

In D154423#4485009, @balazske wrote:

It would be more simple to handle the standard streams in StreamChecker only. [...]

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.

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

In D153954#4480296, @steakhal wrote:

[...] I'd not recommend moving the actual implementation anywhere.

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.

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.

I don't have concrete remarks, but I would like to note that I'm happy to see this cleanup :)

In D153776#4467627, @balazske wrote:

The success/failure note tags are not added to all functions, dup is one of these, this means at dup no note tag is shown. A next patch can be made to add these messages to all functions. The other places look good, but CodeChecker is a bit tricky, you must select *_stdclf_notetag_interesting_test_2 at the small arrow after the "found in:" text (upper right corner). The link is good but not that instance of the bug is displayed because only the note tags are different.

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 :) !

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.