[-Wunsafe-buffer-usage] Handle unevaluated contexts that contain unsafe buffer usages

[malavikasamak]

Summary

This patch handles unevaluated contexts to ensure no warnings are produced by the machinery for buffer access made within an unevaluated contexts. However, such accesses must be considered by a FixableGadget and produce the necessary fixits.

[malavikasamak]

Rebased and removed spurious changes.

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

84–85

We should still visit other subexpressions when ignoreUnevaluatedContext is false.

[malavikasamak]

Addressing the comment on _Generic handling and fixing the machinery to correctly track all DREs.

[malavikasamak]

clang/lib/Analysis/UnsafeBufferUsage.cpp

84–85

Fixed!

[NoQ]

Aha, looks correct now!

We probably want some more tests to demonstrate that even though warnings aren't emitted against evaluated code, fixits still are.

clang/lib/Analysis/UnsafeBufferUsage.cpp

48

Looks like some of these spaces are tabs.

[NoQ]

(these tests can also demonstrate that your fix for _Generic is correct!)

[jkorous]

@NoQ I don't want to be annoying but I think you meant "warnings aren't emitted against UNEVALUATED code", is that right?

[NoQ]

In D144905#4179108, @jkorous wrote:

I think you meant "warnings aren't emitted against UNEVALUATED code"

💯

[jkorous]

clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp

113

Do we think splitting this off as a separate test would help make the tests better?
I am somewhat worried that having many tests in a single file makes it harder to act on failures.

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

1010

I like this idea: skipping unevaluated-contexts from the very beginning!

A nitpick to this change is that now we traverse the function body twice (one for fixables and one for warnings). But I suspect if the overhead (if there is any) is ever gonna be visible.

clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp

113

I agree. Other than these, this patch LGTM so I'm going to accept it.

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

1002

Just realized that any_dre and PointerReferenceGadget can match the same node.

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

1002

Hmm but wouldn't PointerReferenceGadget include a UPC or something?

1010

Warning gadgets can be found at arbitrary depths inside unevaluated contexts, so you can't simply whitelist a few patterns, you need to communicate with the traversal methods to properly ignore them.

So another approach to this problem would be to maintain a count of reasons why current context is unevaluated in the visitor, and have the visitor accept two sub-matchers: one is invoked every time, the other is invoked only when the count is zero. That's very ugly but we can try that if we find it useful for performance.

I also like the current approach because it maps nicely to what kinds of traversal matchers I'd like to have in clang-tidy. Namely, forEveryDescendantEvaluatedStmt() is what people actually want 90% of the time when they write forEachDescendant() and then hopefully deal with unintended consequences manually. (We still want to move these matchers to ASTMatchers.h right???)

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

176–188

While we're at it, maybe let's return to clang's existing naming convention now that the Stmt suffix makes it unambiguous?

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

1002

you are right, though I did encounter such a bug once. But I failed to reproduce it now. Let's not worry about this issue until it appears again.

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

1002

I have a reproducer now. Then I realized that it is not that any_dre and PointerReferenceGadget can race in anyOf. It is in fact FixableGadgets race in anyOf.
So it is a bug but unrelated to this patch. I will explain and fix it in a separated patch.

[malavikasamak]

Move tests to a different file.

[malavikasamak]

clang/lib/Analysis/UnsafeBufferUsage.cpp

1002

@ziqingluo-90: Can you please add a link to the bug description?

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

1002

An example of two FixableGadgets race in anyOf is at a call statement foo(++p, p) where p is a raw pointer. The two FixableGadgets represent a pre-increment and a standalone pointer, respectively, in context of an unsafe call.

The matcher representing the context is callExpr(forEachArgumentWithParam(.... The match succeeds as long as there is at least one parameter matches the sub-matcher (i.e., pre-increment or standalone pointer). Therefore, both Gadgets match the same call statement.

A quick fix is to change anyOf to eachOf but maybe there are more efficient solutions.

[malavikasamak]

clang/lib/Analysis/UnsafeBufferUsage.cpp

1002

If I recall correctly, we discussed changing this from anyOf to eachOf offline and were concerned about potential clashes among the fixits. So, it was decided to leave it alone for now and handle this as a separate patch.

But you are right, this can be a real pain point for us and we need to address this soonish. For your example, this may prevent us from generating any fixits as at least one of the of the DREs will never get claimed.

[NoQ]

Code looks great now, and wow that's a lot of tests!

clang/lib/Analysis/UnsafeBufferUsage.cpp

1002

call statement foo(++p, p)

Can we already add such test case? Regardless of whether it passes, it can help us confirm that we're not doing something terrible (eg. crashing) in such cases. At a glance, switching to eachOf() could totally crash us, so I'd rather document the potential problem in the form of a test while it's hot.

[NoQ]

Thanks! LGTM!

[malavikasamak]

clang/lib/Analysis/UnsafeBufferUsage.cpp

1002

So, it looks like this has already been changed to eachOf on the llvm main branch. Updating this to reflect the same.

clang/lib/Analysis/UnsafeBufferUsage.cpp

public:
   typedef RecursiveASTVisitor<MatchDescendantVisitor> VisitorBase;
 
   // Creates an AST visitor that matches `Matcher` on all
   // descendants of a given node "n" except for the ones
   // belonging to a different callable of "n".
   MatchDescendantVisitor(const internal::DynTypedMatcher *Matcher,
                          internal::ASTMatchFinder *Finder,
                          internal::BoundNodesTreeBuilder *Builder,
-                         internal::ASTMatchFinder::BindKind Bind)
+                         internal::ASTMatchFinder::BindKind Bind,
+       const bool ignoreUnevaluatedContext)
       : Matcher(Matcher), Finder(Finder), Builder(Builder), Bind(Bind),
-        Matches(false) {}
+        Matches(false), ignoreUnevaluatedContext(ignoreUnevaluatedContext) {}
 
   // Returns true if a match is found in a subtree of `DynNode`, which belongs
   // to the same callable of `DynNode`.
   bool findMatch(const DynTypedNode &DynNode) {
     Matches = false;
     if (const Stmt *StmtNode = DynNode.get<Stmt>()) {
       TraverseStmt(const_cast<Stmt *>(StmtNode));
       *Builder = ResultBindings;
bool TraverseDecl(Decl *Node) {
     if (!match(*Node))
       return false;
     // To skip callables:
     if (isa<FunctionDecl, BlockDecl, ObjCMethodDecl>(Node))
       return true;
     // Traverse descendants
     return VisitorBase::TraverseDecl(Node);
   }
    
+  bool TraverseGenericSelectionExpr(GenericSelectionExpr *Node) {
+    // These are unevaluated, except the result expression.
+    if(ignoreUnevaluatedContext)
+      return TraverseStmt(Node->getResultExpr());
+    return VisitorBase::TraverseGenericSelectionExpr(Node);
+  }
+
+  bool TraverseUnaryExprOrTypeTraitExpr(UnaryExprOrTypeTraitExpr *Node) {
+    // Unevaluated context.
+    if(ignoreUnevaluatedContext)
+      return true;
+    return VisitorBase::TraverseUnaryExprOrTypeTraitExpr(Node);
+  }
+
+  bool TraverseTypeOfExprTypeLoc(TypeOfExprTypeLoc Node) {
+    // Unevaluated context.
+    if(ignoreUnevaluatedContext)
+      return true;
+    return VisitorBase::TraverseTypeOfExprTypeLoc(Node);
+  }
+
+  bool TraverseDecltypeTypeLoc(DecltypeTypeLoc Node) {
+    // Unevaluated context.
+    if(ignoreUnevaluatedContext)
+      return true;
+    return VisitorBase::TraverseDecltypeTypeLoc(Node);
+  }
+
+  bool TraverseCXXNoexceptExpr(CXXNoexceptExpr *Node) {
+    // Unevaluated context.
+    if(ignoreUnevaluatedContext)
+      return true;
+    return VisitorBase::TraverseCXXNoexceptExpr(Node);
+  }
+
+  bool TraverseCXXTypeidExpr(CXXTypeidExpr *Node) {
+    // Unevaluated context.
+    if(ignoreUnevaluatedContext)
+      return true;
+    return VisitorBase::TraverseCXXTypeidExpr(Node);
+  }
+
   bool TraverseStmt(Stmt *Node, DataRecursionQueue *Queue = nullptr) {
     if (!Node)
       return true;
     if (!match(*Node))
       return false;
     // To skip callables:
     if (isa<LambdaExpr>(Node))
       return true;
private:
   }
 
   const internal::DynTypedMatcher *const Matcher;
   internal::ASTMatchFinder *const Finder;
   internal::BoundNodesTreeBuilder *const Builder;
   internal::BoundNodesTreeBuilder ResultBindings;
   const internal::ASTMatchFinder::BindKind Bind;
   bool Matches;
+  bool ignoreUnevaluatedContext;
 };
 
 // Because we're dealing with raw pointers, let's define what we mean by that.
 static auto hasPointerType() {
   return hasType(hasCanonicalType(pointerType()));
 }
 
 static auto hasArrayType() { return hasType(hasCanonicalType(arrayType())); }
 
-AST_MATCHER_P(Stmt, forEveryDescendant, internal::Matcher<Stmt>, innerMatcher) {
+AST_MATCHER_P(Stmt, forEveryDescendantEvaluatedStmt, internal::Matcher<Stmt>, innerMatcher) {
   const DynTypedMatcher &DTM = static_cast<DynTypedMatcher>(innerMatcher);
 
-  MatchDescendantVisitor Visitor(&DTM, Finder, Builder, ASTMatchFinder::BK_All);
+  MatchDescendantVisitor Visitor(&DTM, Finder, Builder, ASTMatchFinder::BK_All, true);
   return Visitor.findMatch(DynTypedNode::create(Node));
 }
 
+AST_MATCHER_P(Stmt, forEveryDescendantStmt, internal::Matcher<Stmt>, innerMatcher) {
+  const DynTypedMatcher &DTM = static_cast<DynTypedMatcher>(innerMatcher);
+
+  MatchDescendantVisitor Visitor(&DTM, Finder, Builder, ASTMatchFinder::BK_All, false);
+  return Visitor.findMatch(DynTypedNode::create(Node));
+}
+
+
 // Matches a `Stmt` node iff the node is in a safe-buffer opt-out region
 AST_MATCHER_P(Stmt, notInSafeBufferOptOut, const UnsafeBufferUsageHandler *,
               Handler) {
   return !Handler->isSafeBufferOptOut(Node.getBeginLoc());
 }
 
 AST_MATCHER_P(CastExpr, castSubExpr, internal::Matcher<Expr>, innerMatcher) {
   return innerMatcher.matches(*Node.getSubExpr(), Finder, Builder);
#include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
     }
   };
 
   MatchFinder M;
   GadgetFinderCallback CB;
 
   // clang-format off
   M.addMatcher(
-    stmt(forEveryDescendant(
-      eachOf(
+    stmt(eachOf(
       // A `FixableGadget` matcher and a `WarningGadget` matcher should not disable
       // each other (they could if they were put in the same `anyOf` group).
       // We also should make sure no two `FixableGadget` (resp. `WarningGadget`) matchers
       // match for the same node, so that we can group them
       // in one `anyOf` group (for better performance via short-circuiting).
-      stmt(anyOf(
+      forEveryDescendantStmt(stmt(anyOf(
 #define FIXABLE_GADGET(x)                                                              \
         x ## Gadget::matcher().bind(#x),
 #include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
-        // Also match DeclStmts because we'll need them when fixing
-        // their underlying VarDecls that otherwise don't have
-        // any backreferences to DeclStmts.
-        declStmt().bind("any_ds")
-      )),
-      stmt(anyOf(
+        // In parallel, match all DeclRefExprs so that to find out
+        // whether there are any uncovered by gadgets.
+        declRefExpr(anyOf(hasPointerType(), hasArrayType()), to(varDecl())).bind("any_dre")
+      ))),
+      forEveryDescendantEvaluatedStmt(stmt(anyOf(
         // Add Gadget::matcher() for every gadget in the registry.
 #define WARNING_GADGET(x)                                                              \
         allOf(x ## Gadget::matcher().bind(#x), notInSafeBufferOptOut(&Handler)),
 #include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
-        // In parallel, match all DeclRefExprs so that to find out
-        // whether there are any uncovered by gadgets.
-        declRefExpr(anyOf(hasPointerType(), hasArrayType()), to(varDecl())).bind("any_dre")
-      )))
-    )),
+        // Also match DeclStmts because we'll need them when fixing
+        // their underlying VarDecls that otherwise don't have
+        // any backreferences to DeclStmts. 
+        declStmt().bind("any_ds")
+      ))
+    ))),
     &CB
   );
   // clang-format on
 
   M.match(*D->getBody(), D->getASTContext());
 
   // Gadgets "claim" variables they're responsible for. Once this loop finishes,
   // the tracker will only track DREs that weren't claimed by any gadgets,

clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp

void testAsSystemHeader(char *p) {
 
   foo(p[1],
       ap1[1],
       ap2[2][3]);
 }
 
 #else
 
+namespace std {
+  class type_info;
+  class bad_cast;
+  class bad_typeid;
+}
+using size_t = __typeof(sizeof(int));
+void *malloc(size_t);
+
 void testIncrement(char *p) { // expected-warning{{'p' is an unsafe pointer used for buffer access}}
   ++p; // expected-note{{used in pointer arithmetic here}}
   p++; // expected-note{{used in pointer arithmetic here}}
   --p; // expected-note{{used in pointer arithmetic here}}
   p--; // expected-note{{used in pointer arithmetic here}}
 }
 
 void * voidPtrCall(void);
foo(ap3[1][1]); // expected-note{{used in buffer access here}}
                   // expected-warning@-1{{unsafe buffer access}}
 
   auto ap4 = *pp; // expected-warning{{'ap4' is an unsafe pointer used for buffer access}} \
            expected-note{{change type of 'ap4' to 'std::span' to preserve bounds information}}
 
   foo(ap4[1]);    // expected-note{{used in buffer access here}}
 }
 
-//TODO: do not warn for unevaluated context
-void testUnevaluatedContext(int * p) {// expected-warning{{'p' is an unsafe pointer used for buffer access}}
-  foo(sizeof(p[1]),             // expected-note{{used in buffer access here}}
-      sizeof(decltype(p[1])));  // expected-note{{used in buffer access here}}
+void testUnevaluatedContext(int * p) { // expected-warning{{'p' is an unsafe pointer used for buffer access}}
+  foo(sizeof(p[1]), // no-note
+      sizeof(decltype(p[1]))); // no-note
+  __typeof(p[5]) x; // no-note
+  int *q = (int *)malloc(sizeof(p[5])); // no-note
+  int y = sizeof(p[5]); // no-note
+  __is_pod(__typeof(p[5])); // no-note
+  __is_trivially_constructible(__typeof(p[5]), decltype(p[5])); // no-note
+  _Generic(p[1], int: 2, float: 3); // no-note
+  _Generic(1, int: p[2], float: 3); // expected-note{{used in buffer access here}}
+   _Generic(1, int: 2, float: p[3]); // no-note
+  decltype(p[2]) var = y; // no-note
+  noexcept(p[2]); // no-note
+  typeid(p[3]); // no-note
 }
 
 void testQualifiedParameters(const int * p, const int * const q, const int a[10], const int b[10][10]) {
   // expected-warning@-1{{'p' is an unsafe pointer used for buffer access}}
   // expected-warning@-2{{'q' is an unsafe pointer used for buffer access}}
   // expected-warning@-3{{'a' is an unsafe pointer used for buffer access}}
   // expected-warning@-4{{'b' is an unsafe pointer used for buffer access}}