Page MenuHomePhabricator

[analyzer] SValHasDescendant: Determine whether the SVal has an SVal descendant
AbandonedPublic

Authored by Charusso on Nov 28 2019, 1:40 AM.

Details

Reviewers
NoQ
Summary

This relies on the SValVisitor and behaves something similar to the
Clang Tidy's hasDescendant(). The difference is it allows equality.

Diff Detail

Event Timeline

Charusso created this revision.Nov 28 2019, 1:40 AM
Charusso added a comment.EditedNov 28 2019, 1:47 AM

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);
char buffer[256];
char *string = NULL;
size_t buflen = strlen(buffer);
char *ptr = realloc(string, stringlen + buflen + 1);
string = ptr;
strcpy(string + stringlen, buffer);
char *enc = curl_easy_escape(NULL, postdata, (int)size);
size_t outlen = nlen + strlen(enc) + 2;
char *n = malloc(outlen);
strcpy(n, enc);
Charusso updated this revision to Diff 232203.Dec 4 2019, 2:02 PM
Charusso edited the summary of this revision. (Show Details)
  • Add more tests.
  • Let us specify the descendant as allowing equality of symbols.
Charusso updated this revision to Diff 232206.Dec 4 2019, 2:04 PM
  • Add back a comment.

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.

NoQ added a comment.Dec 9 2019, 7:41 PM

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

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

When I encounter the following:

unsigned Foo = Foo.Bar[Baz]->Qux;
malloc(strlen(src) + Foo + 1);

At the moment I cannot lookup the strlen(src) and nor the + 1. There is no way to say the strlen(src) which parent is a malloc(). I do not think we could make the full support of pattern-matching on symbolic level, so at least I wanted to see if it is possible. I am even thinking of using the ASTImporter to normalize the expression so that in a flatten/serialized form the AST-matcher could look for such constructs without any wonky DeclRefExpr or ReturnStmt matching. I believe the latter is the way we could benefit a lot, and the former, the current patch needs to be eliminated. For now I cannot relax the AST or invent a symbolic-matcher language so I would prefer this simple workaround and may we will have better ideas later.

NoQ added inline comments.Dec 10 2019, 4:28 PM
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValHasDescendant.h
56

Arithmetic is indeed easy, but for example this part requires a much deeper justification.

Charusso marked 2 inline comments as done.Jan 31 2020, 9:22 AM
Charusso added inline comments.
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValHasDescendant.h
56

Well, this is an experiment. I have checked out the Clang Tidy's matcher-writing language's framework so I believe their own language is way more better, than implementing hasDescendant() only. Some kind of framework would be neat, but this is what I came up with as the hasDescendant() is the most powerful matcher in their world.

Charusso updated this revision to Diff 241758.Jan 31 2020, 9:43 AM
Charusso marked an inline comment as done.
  • Rebase.
Charusso abandoned this revision.May 24 2020, 9:26 PM

Way more sophisticated matching: https://reviews.llvm.org/D77745