Page MenuHomePhabricator

[analyzer] StdLibraryFunctionsChecker: fix bug with arg constraints
ClosedPublic

Authored by martong on Mar 25 2020, 11:15 AM.

Details

Summary

Previously we induced a state split if there were multiple argument
constraints given for a function. This was because we called
addTransition inside the for loop.
The fix is to is to store the state and apply the next argument
constraint on that. And once the loop is finished we call addTransition.

Diff Detail

Event Timeline

martong created this revision.Mar 25 2020, 11:15 AM

I was thinking about the below test, but then I reverted back because I don't want to add "fake" summaries for testing purposes. Perhaps adding a new checker option could enable these "fake" summaries, @Szelethus what's your opinion?

diff --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
index 64a412bb4db..c1022492429 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -736,6 +736,14 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
   };

   FunctionSummaryMap = {
+      {
+        "__test",
+        Summaries{
+          Summary(ArgTypes{IntTy, IntTy}, RetType{IntTy}, EvalCallAsPure)
+            .ArgConstraint(ArgumentCondition(0U, WithinRange, SingleValue(1)))
+            .ArgConstraint(ArgumentCondition(1U, WithinRange, SingleValue(1)))
+        }
+      },
       // The isascii() family of functions.
       // The behavior is undefined if the value of the argument is not
       // representable as unsigned char or is not equal to EOF. See e.g. C99
diff --git a/clang/test/Analysis/std-c-library-functions-arg-constraints.c b/clang/test/Analysis/std-c-library-functions-arg-constraints.c
index a20b90ad1cc..01109e28b99 100644
--- a/clang/test/Analysis/std-c-library-functions-arg-constraints.c
+++ b/clang/test/Analysis/std-c-library-functions-arg-constraints.c
@@ -85,3 +85,18 @@ void test_notnull_symbolic2(FILE *fp, int *buf) {
     // bugpath-warning{{Function argument constraint is not satisfied}} \
     // bugpath-note{{Function argument constraint is not satisfied}}
 }
+
+int __test(int, int);
+
+void test_multiple_constraints(int x, int y) {
+  __test(x, y);
+  clang_analyzer_eval(x == 1); // \
+  // report-warning{{TRUE}} \
+  // bugpath-warning{{TRUE}} \
+  // bugpath-note{{TRUE}}
+  clang_analyzer_eval(y == 1); // \
+  // report-warning{{TRUE}} \
+  // bugpath-warning{{TRUE}} \
+  // bugpath-note{{TRUE}}
+}
+

The fix looks great!

I was thinking about the below test, but then I reverted back because I don't want to add "fake" summaries for testing purposes. Perhaps adding a new checker option could enable these "fake" summaries, @Szelethus what's your opinion?

The fake summary sounds great. You could hide it behind a debug checker, if we are to be that cautious.

martong updated this revision to Diff 253174.Mar 27 2020, 11:01 AM
  • Add tests with a subchecker
martong updated this revision to Diff 253595.Mar 30 2020, 7:35 AM
  • Test multiple constraints on the same arg
Szelethus accepted this revision.Apr 2 2020, 6:19 AM

Yup, looks great, sorry for the slack!

This revision is now accepted and ready to land.Apr 2 2020, 6:19 AM
This revision was automatically updated to reflect the committed changes.
NoQ added a comment.Apr 6 2020, 11:55 AM

Yup, looks great

Can confirm.