In D71433#1784238, @NoQ wrote:

Currently the check may warn on non-bugs of the following kind:

void foo() {
  char env[] = "NAME=value";
  putenv(env);
  doStuff();
  putenv("NAME=anothervalue");
}

I have created a notes.cpp test-file to test the notes in my checkers, but I think this checker is fine without that test file. @NoQ, what do you think?

In order to bypass the CK_LValueToRValue evalCast() we have to create en ElementRegion as a return-value of the problematic function call. In that case for a mythical reason we miss the fact the pointer is nullable. I have not figured out yet why, but tried to create an appropriate return-value.

• Try to conjure the index of the 'ElementRegion'.

We need to add the interestingness to the NoteTags so that we only emit notes on initialization/function calls which leads to an error.

• Add notes to the initialization of char-arrays.

• Iterate over parameters rather arguments for searching string-reading.
• Better notes of the argument's index.
• Avoid C.getPredecessor(), use the error-node instead.

In D70411#1776460, @NoQ wrote:

Ok, so, what should the checker warn on? What should be the intended state machine for this checker?

We basically have two possible categories of answers to this question:

• "The checker warns every time it finds an execution path on which unintended behavior occurs" - describe unintended behavior (say, "out-of-bounds buffer access") and define the checker in such a way that all warnings describe actual bugs of this kind (in our example, out-of-bound buffer accesses) as long as the rest of the static analyzer works correctly.
• Or, you could say "The checker enforces a certain coding guideline on the user" - and then you should describe the guideline in a very short and easy-to-understand manner (say, "don't pass the string by pointer anywhere before you null-terminate it"), and then make checker check exactly this guideline. You may introduce some suppressions for intentional violations of the guideline, but the guideline itself should be simple, it should be always possible to follow it, and it should sound nice enough for developers to feel good about themselves when they follow it.

• Fix.

Could you write a test for strcmp please? I have found out the evalMemcmp behaves differently than evalStrcmp. However when the memcmp is going to be used like strcmp/strncmp the ideal would be to delegate the work to evalStrcmp. At the moment there is no connection between the two evaluation at all, which surprised me at first.

Cool, thanks!

In D70805#1776527, @NoQ wrote:

Usually this kind of code is hard to re-use because all use cases require different definitions of "has descendant". We already have at least 3 such definitions floating around and we can't re-use them.

Even in the world of ASTMatchers the actual hasDescendant matcher is basically an anti-pattern as you always want to use a matcher for a specific parent-child relation instead (cf. http://lists.llvm.org/pipermail/cfe-dev/2019-September/063260.html).

Forgotten amend of commit message. Commited as:

I think it is fine, but please let us wait with the final thoughts from @aaron.ballman.

Examples generated by CodeChecker from the curl project:

• Add docs.
• Move to alpha.

Please let us measure your examples at first to have a consensus what we would like to see from this checker.

• Make the checker alpha.
• Add docs.
• Copy-paste @NoQ's test.

I would change the order of CCh and scan-build because we usually list stuff in alphabetical order. Also the chronological order is that, the newest is the first.

Examples generated by CodeChecker from the curl project:

Here is a possible piece of code from the Tidy world: anyOf(declRefExpr(), hasDescendant(declRefExpr()), integerLiteral(), hasDescendant(integerLiteral())), that is why I recommend to allow equality of symbols to prevent boilerplate.

• Add back a comment.

• Add more tests.
• Let us specify the descendant as allowing equality of symbols.

I have picked curl as an example, because it has a measurable amount of reports. I still recommend to make this alpha, because the expression-tracking cannot track members, and peoples are using members everywhere.

• Tweaking.

In D70725#1767579, @xazax.hun wrote:

Just a small side note. If the state was same as the previous we do not end up creating a new ExplodedNode right? Is this also the case when we add a NoteTag?

It suppresses stuff in curl like:

char buf[4096];
size_t linelen = strlen(line);
char *ptr = realloc(line, linelen + strlen(buf) + 1);
strcpy(&line[linelen], buf);

The false positive suppression by going backwards on the bug-path of stored reports was a very simple concept, which turned out to be a useless one. The rough idea was that to invalidate every report in a path, and every other report would be left because they are reachable from a different path and they are valid errors. So that:

void test_wonky_null_termination(const char *src) {
  char dest[128];
  strcpy(dest, src);
  dest[sizeof(dest) - 1] = '\0';
  do_something(dest); // no-warning
}

The above example null-terminates the string on every path, so we remove the report, and:

• Do not emit a report on re-binding a not null-terminated string, it may will be properly terminated (test added).
• Added a test for the necessity of SValVisitor in this checker.
• Some refactor.

Now I have simplified the checker so we do not need any ASCII-art, I believe. Do we have any better logic than the current implementation to catch when the string is read?

• Remove the report storing map so we do not traverse backwards on the bug-path.
• Use NoteTags instead of reports on problematic function calls.
• Emit a report only if a not null-terminated string is read.
• Store whether the string is null-terminated.

In D70411#1754356, @NoQ wrote:

I think it would really help if you draw a state machine for the checker, like the ASCII-art thing in D70470; you don't need to spend a lot of time turning it into ASCII-art, a photo of a quick hand-drawn picture would be totally fine, because it's, first and foremost, for discussion :)

• Added a report when the not null-terminated string is read.
• Added comments.
• Fixed some uninteresting nits.
• No ASCII-art yet.

This patch moved to D70411.

Somehow the MallocBugVisitor is broken in the wild, and we need to support a lot more to call it non-alpha (see the FIXMEs). I do not want to spend time on fixing the visitor, but rather I would make this checker alpha, and move on. After a while when we have enough alarms we could see what could go wrong. The reports are not bad, I tried to make it super false-positive free.

Given that we are having two different projects at first let us create the path-sensitive error-catching + false positive suppression + design of the CERT rules, and when we are fine, we get back to the impossible-to-solve problem: to adjust fix-its (path-sensitively).

• The packaging have not been addressed yet.
• Inject the "zombie" size expression to the new function call (fgets) if none of the size expression's regions have been modified.

In D69813#1736611, @aaron.ballman wrote:

Would it make sense to use cert.str.31.c to remove the random dash? Would this also help the user to do something like cert.str.*.cpp? if they want just the CERT C++ STR rules checked? Or can they do that already even with the -?

In D69813#1735988, @aaron.ballman wrote:

I'm not @NoQ, but I do agree that there should be a separate check per rule in terms of the UI presented to the user. The name should follow the rule ID like they do in clang-tidy, for some consistency there.
I think that the rule number should be in the name. I'd probably go with cert.STR31-C or cert.str31-c (so it's clear which CERT standard the rule came from).

In D69813#1734193, @Szelethus wrote:

Hmm, so this checker is rather a collection of CERT rule checkers, right? Shouldn't the checker name contain the actual rule name (STR31-C)? User interfacewise, I would much prefer smaller, leaner checkers than a big one with a lot of options, which are barely supported for end-users. I would expect a cert package to contain subpackages like cert.str, and checker names cert.str.31StringSize, or similar.

• Support alloca().

• Do not try to fix-it an array with offsets.

• Use existing visitors.
• Make the MallocBugVisitor available for every checker.
• Fix duplication of fix-its on the warning path piece when we emit a note as well.

In D69745#1732739, @NoQ wrote:

Basically this, but the other way round: your BeginAnalysis is BeginFunction with extra steps (namely, checking C.inTopFrame()).

Backstory: I wanted to add BeginAnalysis a few years ago because i wanted to mark argc and argv as tainted when analyzing main(). But then i noticed that we already have BeginFunction so i decided not to add it. No, i didn't mark them as tainted yet :/

In D69726#1732334, @Szelethus wrote:

Changes to MallocChecker really highlight the positive effects of this patch. Nice!

In D69745#1732348, @Szelethus wrote:

YES PLEASE. Debug checkers that only dump from check::EndAnalysis won't rely on the analysis not actually crashing. Which is ironically exactly when we want to use them.

• Do not restrict what you supposed to do with the callback.

On multiple evaluation of the same call the Analyzer will warn and prevent you to do so.

• Remove llvm_unreacheable from error-handling.

Hey! I would like to reuse your script without any reinvention. It serves every needs, but at some point we start to heavily diverge. When I started with the Tidy I really enjoyed that script, and most of the people I know both develop Tidy and the Analyzer, so I would keep the design of the script to make it consistent to use.

I felt that it will be easier, eh.

In D69731#1730956, @Charusso wrote:

I am thinking of a callback which is something like:

void checkBeginAnalysis(const Decl *D, BugReporter &BR) const;

so it would be easy and meaningful to have a place for the Preprocessor logic. Do you think it would worth it?

• Use less const, it prevented the usage of non-const methods.

I am thinking of a callback which is something like:

void checkBeginAnalysis(const Decl *D, BugReporter &BR) const;

so it would be easy and meaningful to have a place for the Preprocessor logic. Do you think it would worth it?