Page MenuHomePhabricator

[analyzer] CERT STR rule checkers: STR32-C
Needs RevisionPublic

Authored by Charusso on Dec 4 2019, 2:20 PM.

Details

Summary

This patch introduces a new experimental checker:
alpha.security.cert.str.32c

This checker is implemented based on the following rule:
https://wiki.sei.cmu.edu/confluence/display/c/STR32-C.+Do+not+pass+a+non-null-terminated+character+sequence+to+a+library+function+that+expects+a+string

It warns on reading non-null-terminated strings. This warning is restricted to
the allocations which the Static Analyzer models with unix.Malloc checker.

Also warns on misusing the strncpy() function.

Diff Detail

Event Timeline

Charusso created this revision.Dec 4 2019, 2:20 PM
Charusso marked an inline comment as done.Dec 4 2019, 2:25 PM
Charusso added inline comments.
clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
519

This is a huge assumption to make this checker as simple as possible. On each allocation I would store the memory regions which the size expression consists of. When we encounter a memory/string handler function call which has a size-expression parameter we could match whether the allocation considered the length of the string it will store.

May we will have a better idea, so I would leave that as it is, for now.

Examples generated by CodeChecker from the curl project:

Charusso updated this revision to Diff 232672.EditedDec 6 2019, 4:19 PM
Charusso edited the summary of this revision. (Show Details)
  • Add docs.
  • Move to alpha.
Charusso updated this revision to Diff 233904.EditedDec 13 2019, 6:47 PM
Charusso retitled this revision from [analyzer] CERT: StrChecker: 32.c to [analyzer] CERT: STR32-C.
  • Add notes to the initialization of char-arrays.
  • Mark the string null-terminated calling strcpy() with an appropriate size.

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

Charusso marked an inline comment as done.Dec 13 2019, 6:52 PM
Charusso added inline comments.
clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
513

-1 line

Charusso updated this revision to Diff 241761.Jan 31 2020, 9:58 AM
  • Rebase.
Charusso edited the summary of this revision. (Show Details)Jan 31 2020, 9:58 AM
Charusso updated this revision to Diff 265962.May 24 2020, 9:23 PM
Charusso retitled this revision from [analyzer] CERT: STR32-C to [analyzer] CERT STR rule checkers: STR32-C.
  • Refactor.
  • State out explicitly whether the Analyzer models the dynamic size.
baloghadamsoftware requested changes to this revision.Jun 29 2020, 9:15 AM
baloghadamsoftware added inline comments.
clang/docs/analyzer/checkers.rst
1982

Why? Is retrieving the size of arrays is so much more difficult? We miss the first example of the rule this way.

clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
181

Why do we need this? The constructor of PathSensitiveBugReport takes a StringRef. Msg.str() returns a StringRef. Your solution creates a C++ string first, which means an unnecessary copy.

313

Do we really need to handle indirect pointers here? I means e.g. char ***?

322

What does it mean that the memory allocation could overflow?

457

Why checking PostCall and not PreCall? Usually PostCall is used for modeling and PreCall for checking because for modeling we need the call already evaluated (e.g. we need the return value) but checking should happen before starting the evaluation of the call.

469

Do we chack any call here passing non-null terminated strings? I disagree with that appraocah because there may be functions expecting e.g. binary data. It seems that we are not even checking the type so we could get false positives here for any mem...() function.

clang/test/Analysis/cert/str32-c.cpp
10

Please move this to the system header simulator instead of repeating it in every test file.

13

Why enum?

This revision now requires changes to proceed.Jun 29 2020, 9:15 AM