Page MenuHomePhabricator

[analyzer] CERT: STR30-C
Needs ReviewPublic

Authored by Charusso on Dec 6 2019, 4:21 PM.

Details

Reviewers
NoQ
Summary

This checker is implemented based on the following rule:
https://wiki.sei.cmu.edu/confluence/display/c/STR30-C.+Do+not+attempt+to+modify+string+literals

It warns on misusing the following functions: strpbrk(), strchr(),
strrchr(), strstr(), memchr().

Diff Detail

Event Timeline

Charusso created this revision.Dec 6 2019, 4:21 PM

Examples generated by CodeChecker from the curl project:

NoQ added inline comments.Dec 9 2019, 7:23 PM
clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
2109

Why does it matter whether the region is symbolic or not?

2110

Mmm, that's not a correct return value for these functions. These functions don't simply pass through their first argument.

2122

Why "heap"?

Charusso marked 2 inline comments as done.Dec 9 2019, 7:41 PM
Charusso added inline comments.
clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
2109

Hm, I think you are right, we could omit that if-statement. I wanted to specify to reuse conjured symbols.

2110

Yes, but we need some index here. It requires a NonLoc, so I just randomly picked the first index, but I like the idea of an unknown index. Would we like to introduce UnknownVal for indices?

2122

Well, a string which length is at least 16 characters long is going to be allocated on the heap. I have to conjure the string here to create its element.

NoQ added inline comments.Dec 9 2019, 7:48 PM
clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
2110

Use the correct region but conjure the index.

2122

o.o

void foo() {
  // This string is 20 characters long
  // but it's clearly on the stack.
  char str[] = "12345678901234567890";
  // This one is therefore also on the stack.
  char *ptr = strchr(str, '0');
}
Charusso updated this revision to Diff 233905.Dec 13 2019, 6:53 PM
Charusso marked 6 inline comments as done.
  • Try to conjure the index of the 'ElementRegion'.
Charusso retitled this revision from [analyzer] CERT: StrChecker: 30.c to [analyzer] CERT: STR30-C.EditedDec 13 2019, 7:01 PM
Charusso marked 3 inline comments as done.

In order to bypass [1] 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 using a VarRegion so we could refer to the underlying constant string's name.

[1] bypass: If I tried to evaluate the call by returning a pointer to the element it immediately wraps the SymbolicRegion into an ElementRegion, like with HeapSpaceRegions it is useful, but for us it is bad.

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
2122

Well, a string which length is at least 16 characters long is going to be allocated on the heap. I have to conjure the string here to create its element.

I really felt that the std::string should behave like the C-strings, but C-strings are on the stack whatever it takes, yes, my bad. Thanks for pointing that out!

clang/test/Analysis/cert/str30-c-notes.cpp
29

Needs to be an assumption.