Page MenuHomePhabricator

[Analyzer] Fix for incorrect use of container and iterator checkers
ClosedPublic

Authored by baloghadamsoftware on Feb 26 2020, 6:11 AM.

Details

Summary

Iterator checkers (and planned container checkers) need the option aggressive-binary-operation-simplification to be enabled. Without this option they may cause assertions. To prevent such misuse, this patch adds a preventive check which issues a warning and denies the registartion of the checker if this option is disabled.

Diff Detail

Event Timeline

Do you have access to the Driver somehow? Instead of a cerr (-ish) output, something that is formatted like a "true" error diagnostic (or warning), such as "no such file or directory" would be much better, I fear this [ERROR] will simply be flooded away with the usual diagnostic output on screen, especially if multiple files are concerned.

Are you trying to land this before 10.0 goes out? In this case, it could be okay, but for the long term, this seems a very crude solution.

clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
1070–1071

In Clang diagnostics, "identifiers" are put between apostrophes:

'aggressive-binary-operation-simplification'

Also, errors should conceptually break the operation at hand, so this thing should be a warning instead?

Szelethus requested changes to this revision.Feb 26 2020, 9:09 AM

Im sorry, i though we talked about internal code and we're stuck with the checker interface -- this should definitely be solved by changing the parameter of the shouldRegister function to CheckerManager. If you want to emit errors for incorrect file, there is an example in UninitializedObjectChecker and maybe in GenericTaintChecker.

This revision now requires changes to proceed.Feb 26 2020, 9:09 AM

Im sorry, i though we talked about internal code and we're stuck with the checker interface -- this should definitely be solved by changing the parameter of the shouldRegister function to CheckerManager. If you want to emit errors for incorrect file, there is an example in UninitializedObjectChecker and maybe in GenericTaintChecker.

Sorry, but it does not work. It is impossible to use CheckerManager as parameter for shouldRegisterXXX() because CheckerRegistry does not have it. If I add CheckerManager to CheckerRegistry then the printXXX() functions will not work because they do not have CheckerManager at all. I tried to add AnalyzerOptions as a second parameter (see D75271), but it does not help in printing error message. I need a way to solve 44998 by rejecting the checker if the option is disabled and printing an error message.

This is the so called "correct" solution. However, it does not even compile because getAnalyzerOptions() and getASTContext() are non-const functions of CheckerManager, but shouldRegisterXXX() functions get it as const reference. Even if I use const_cast (not in this patch, just for testing) it does not work, it does not prevent the crash: when trying to register IteratorModeling which depends on ContainerModeling both checkers are registered after shouldRegisterContainerModeling() function returning false.

Szelethus requested changes to this revision.Mar 6 2020, 7:34 AM

You seem to have uploaded the wrong diff :)

This is the so called "correct" solution. However, it does not even compile because getAnalyzerOptions() and getASTContext() are non-const functions of CheckerManager, but shouldRegisterXXX() functions get it as const reference.

That doesn't sound too drastic, why don't you make those methods const?

Even if I use const_cast (not in this patch, just for testing) it does not work, it does not prevent the crash: when trying to register IteratorModeling which depends on ContainerModeling both checkers are registered after shouldRegisterContainerModeling() function returning false.

Huh, that sounds interesting. I just tried it locally and it works like a charm. Would you mind me commandeering this patch to demonstrate?

This revision now requires changes to proceed.Mar 6 2020, 7:34 AM
Szelethus added a comment.EditedMar 6 2020, 8:01 AM

Actually, this is the diff:

diff --git a/clang/include/clang/Basic/DiagnosticDriverKinds.td b/clang/include/clang/Basic/DiagnosticDriverKinds.td
index ecd871e36ee..1f2c8c50a01 100644
--- a/clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ b/clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -341,6 +341,8 @@ def err_analyzer_checker_option_unknown : Error<
   "checker '%0' has no option called '%1'">;
 def err_analyzer_checker_option_invalid_input : Error<
   "invalid input for checker option '%0', that expects %1">;
+def err_analyzer_checker_incompatible_analyzer_option : Error<
+  "checker cannot be enabled with analyzer option '%0' == %1">;
 
 def err_drv_invalid_hvx_length : Error<
   "-mhvx-length is not supported without a -mhvx/-mhvx= flag">;
diff --git a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
index a3a85bfac13..680cb92e1ff 100644
--- a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
+++ b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
@@ -169,8 +169,8 @@ public:
   void finishedCheckerRegistration();
 
   const LangOptions &getLangOpts() const { return LangOpts; }
-  AnalyzerOptions &getAnalyzerOptions() { return AOptions; }
-  ASTContext &getASTContext() {
+  AnalyzerOptions &getAnalyzerOptions() const { return AOptions; }
+  ASTContext &getASTContext() const {
     assert(Context);
     return *Context;
   }
diff --git a/clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp b/clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
index 16a913e761c..f739417a37b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
@@ -12,6 +12,7 @@
 
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/Driver/DriverDiagnostic.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
@@ -1036,5 +1037,15 @@ void ento::registerContainerModeling(CheckerManager &mgr) {
 }
 
 bool ento::shouldRegisterContainerModeling(const CheckerManager &mgr) {
+  if (!mgr.getLangOpts().CPlusPlus)
+    return false;
+
+  if (!mgr.getAnalyzerOptions().ShouldAggressivelySimplifyBinaryOperation) {
+    mgr.getASTContext().getDiagnostics().Report(
+        diag::err_analyzer_checker_incompatible_analyzer_option)
+      << "aggressive-binary-operation-simplification" << "false";
+    return false;
+  }
+
   return true;
 }
diff --git a/clang/test/Analysis/checker-dependencies.c b/clang/test/Analysis/checker-dependencies.c
index 6c8583adb35..54d585561d1 100644
--- a/clang/test/Analysis/checker-dependencies.c
+++ b/clang/test/Analysis/checker-dependencies.c
@@ -18,3 +18,11 @@
 
 // CHECK-IMPLICITLY-DISABLED-NOT: osx.cocoa.RetainCountBase
 // CHECK-IMPLICITLY-DISABLED-NOT: osx.cocoa.RetainCount
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=cplusplus.IteratorModeling \
+// RUN:   -analyzer-config aggressive-binary-operation-simplification=false \
+// RUN:   -analyzer-list-enabled-checkers \
+// RUN:   2>&1 | FileCheck %s -check-prefix=DEPENDENCY-SHOULD-NOT-REGISTER
+
+// DEPENDENCY-SHOULD-NOT-REGISTER: 'aggressive-binary-operation-simplification' == false

Yay, this is very nice!

clang/test/Analysis/container-modeling-no-aggressive-binary-operation-simplification-warn.cpp
2–11

What does this file do that clang/test/Analysis/iterator-modeling-no-aggressive-binary-operation-simplification-no-crash.cpp doesn't?

clang/test/Analysis/iterator-modeling-no-aggressive-binary-operation-simplification-no-crash.cpp
39–40

Is this where the crash supposed to happen? Can you mark it with // no-crash?

This revision is now accepted and ready to land.Mar 10 2020, 4:36 PM
This revision was automatically updated to reflect the committed changes.

@baloghadamsoftware @Szelethus it would be great to have the name of the checkers in the error message
The error is "error: checker cannot be enabled with analyzer option 'aggressive-binary-operation-simplification' == false"
and I had to look at this patch to understand that it is about iterator

@baloghadamsoftware @Szelethus it would be great to have the name of the checkers in the error message
The error is "error: checker cannot be enabled with analyzer option 'aggressive-binary-operation-simplification' == false"
and I had to look at this patch to understand that it is about iterator

Huh, that is a fair point -- Adam, can you patch it in please?