Bug 1302163 - Show an error for [v]snprintf with literals using the clang plugin. r=ehsan
authorTom Schuster <evilpies@gmail.com>
Tue, 04 Oct 2016 17:57:50 +0200
changeset 316392 b66170ee0000101ccda9b4887ff41d2d8ecddc07
parent 316391 f3f755f8c1e472b093b053688f7431eb3e8933e4
child 316393 628c7aa9630fa150df658132d475647816a02f04
push id20656
push userkwierso@gmail.com
push dateWed, 05 Oct 2016 00:44:03 +0000
treeherderfx-team@ea104eeb14cc [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersehsan
bugs1302163
milestone52.0a1
Bug 1302163 - Show an error for [v]snprintf with literals using the clang plugin. r=ehsan
build/autoconf/clang-plugin.m4
build/clang-plugin/clang-plugin.cpp
build/clang-plugin/tests/TestSprintfLiteral.cpp
build/clang-plugin/tests/moz.build
--- a/build/autoconf/clang-plugin.m4
+++ b/build/autoconf/clang-plugin.m4
@@ -116,16 +116,45 @@ if test -n "$ENABLE_CLANG_PLUGIN"; then
             CXXFLAGS="$_SAVE_CXXFLAGS"
             export MACOSX_DEPLOYMENT_TARGET="$_SAVE_MACOSX_DEPLOYMENT_TARGET"
             AC_LANG_RESTORE
         ])
     if test "$ac_cv_have_new_ASTMatcher_names" = "yes"; then
       LLVM_CXXFLAGS="$LLVM_CXXFLAGS -DHAVE_NEW_ASTMATCHER_NAMES"
     fi
 
+    dnl Check if we can compile has(ignoringParenImpCasts()) because
+    dnl before 3.9 that ignoringParenImpCasts was done internally by "has".
+    dnl See https://www.mail-archive.com/cfe-commits@lists.llvm.org/msg25234.html
+    AC_CACHE_CHECK(for has with ignoringParenImpCasts,
+                   ac_cv_has_accepts_ignoringParenImpCasts,
+        [
+            AC_LANG_SAVE
+            AC_LANG_CPLUSPLUS
+            _SAVE_CXXFLAGS="$CXXFLAGS"
+            _SAVE_CXX="$CXX"
+            _SAVE_MACOSX_DEPLOYMENT_TARGET="$MACOSX_DEPLOYMENT_TARGET"
+            unset MACOSX_DEPLOYMENT_TARGET
+            CXXFLAGS="${LLVM_CXXFLAGS}"
+            CXX="${HOST_CXX}"
+            AC_TRY_COMPILE([#include "clang/ASTMatchers/ASTMatchers.h"],
+                           [using namespace clang::ast_matchers;
+                            expr(has(ignoringParenImpCasts(declRefExpr())));
+                           ],
+                           ac_cv_has_accepts_ignoringParenImpCasts="yes",
+                           ac_cv_has_accepts_ignoringParenImpCasts="no")
+            CXX="$_SAVE_CXX"
+            CXXFLAGS="$_SAVE_CXXFLAGS"
+            export MACOSX_DEPLOYMENT_TARGET="$_SAVE_MACOSX_DEPLOYMENT_TARGET"
+            AC_LANG_RESTORE
+        ])
+    if test "$ac_cv_has_accepts_ignoringParenImpCasts" = "yes"; then
+      LLVM_CXXFLAGS="$LLVM_CXXFLAGS -DHAS_ACCEPTS_IGNORINGPARENIMPCASTS"
+    fi
+
     AC_DEFINE(MOZ_CLANG_PLUGIN)
 fi
 
 AC_SUBST(LLVM_CXXFLAGS)
 AC_SUBST(LLVM_LDFLAGS)
 AC_SUBST(CLANG_LDFLAGS)
 
 AC_SUBST(ENABLE_CLANG_PLUGIN)
--- a/build/clang-plugin/clang-plugin.cpp
+++ b/build/clang-plugin/clang-plugin.cpp
@@ -37,16 +37,24 @@ typedef ASTConsumer *ASTConsumerPtr;
 // ones for compatibility with older versions.
 #define cxxConstructExpr constructExpr
 #define cxxConstructorDecl constructorDecl
 #define cxxMethodDecl methodDecl
 #define cxxNewExpr newExpr
 #define cxxRecordDecl recordDecl
 #endif
 
+#ifndef HAS_ACCEPTS_IGNORINGPARENIMPCASTS
+#define hasIgnoringParenImpCasts(x) has(x)
+#else
+// Before clang 3.9 "has" would behave like has(ignoringParenImpCasts(x)),
+// however doing that explicitly would not compile.
+#define hasIgnoringParenImpCasts(x) has(ignoringParenImpCasts(x))
+#endif
+
 // Check if the given expression contains an assignment expression.
 // This can either take the form of a Binary Operator or a
 // Overloaded Operator Call.
 bool hasSideEffectAssignment(const Expr *Expression) {
   if (auto OpCallExpr = dyn_cast_or_null<CXXOperatorCallExpr>(Expression)) {
     auto BinOp = OpCallExpr->getOperator();
     if (BinOp == OO_Equal || (BinOp >= OO_PlusEqual && BinOp <= OO_PipeEqual)) {
       return true;
@@ -158,16 +166,21 @@ private:
     virtual void run(const MatchFinder::MatchResult &Result);
   };
 
   class KungFuDeathGripChecker : public MatchFinder::MatchCallback {
   public:
     virtual void run(const MatchFinder::MatchResult &Result);
   };
 
+  class SprintfLiteralChecker : public MatchFinder::MatchCallback {
+  public:
+    virtual void run(const MatchFinder::MatchResult &Result);
+  };
+
   ScopeChecker Scope;
   ArithmeticArgChecker ArithmeticArg;
   TrivialCtorDtorChecker TrivialCtorDtor;
   NaNExprChecker NaNExpr;
   NoAddRefReleaseOnReturnChecker NoAddRefReleaseOnReturn;
   RefCountedInsideLambdaChecker RefCountedInsideLambda;
   ExplicitOperatorBoolChecker ExplicitOperatorBool;
   NoDuplicateRefCntMemberChecker NoDuplicateRefCntMember;
@@ -175,16 +188,17 @@ private:
   NonMemMovableTemplateArgChecker NonMemMovableTemplateArg;
   NonMemMovableMemberChecker NonMemMovableMember;
   ExplicitImplicitChecker ExplicitImplicit;
   NoAutoTypeChecker NoAutoType;
   NoExplicitMoveConstructorChecker NoExplicitMoveConstructor;
   RefCountedCopyConstructorChecker RefCountedCopyConstructor;
   AssertAssignmentChecker AssertAttribution;
   KungFuDeathGripChecker KungFuDeathGrip;
+  SprintfLiteralChecker SprintfLiteral;
   MatchFinder AstMatcher;
 };
 
 namespace {
 
 std::string getDeclarationNamespace(const Decl *Declaration) {
   const DeclContext *DC =
       Declaration->getDeclContext()->getEnclosingNamespaceContext();
@@ -279,16 +293,45 @@ bool isIgnoredPathForImplicitConversion(
       // Ignore security/sandbox/chromium but not ipc/chromium.
       ++Begin;
       return Begin != End && Begin->compare_lower(StringRef("sandbox")) == 0;
     }
   }
   return false;
 }
 
+bool isIgnoredPathForSprintfLiteral(const CallExpr *Call, const SourceManager &SM) {
+  SourceLocation Loc = Call->getLocStart();
+  SmallString<1024> FileName = SM.getFilename(Loc);
+  llvm::sys::fs::make_absolute(FileName);
+  llvm::sys::path::reverse_iterator Begin = llvm::sys::path::rbegin(FileName),
+                                    End = llvm::sys::path::rend(FileName);
+  for (; Begin != End; ++Begin) {
+    if (Begin->compare_lower(StringRef("angle")) == 0 ||
+        Begin->compare_lower(StringRef("chromium")) == 0 ||
+        Begin->compare_lower(StringRef("crashreporter")) == 0 ||
+        Begin->compare_lower(StringRef("google-breakpad")) == 0 ||
+        Begin->compare_lower(StringRef("harfbuzz")) == 0 ||
+        Begin->compare_lower(StringRef("libstagefright")) == 0 ||
+        Begin->compare_lower(StringRef("mtransport")) == 0 ||
+        Begin->compare_lower(StringRef("protobuf")) == 0 ||
+        Begin->compare_lower(StringRef("skia")) == 0 ||
+        // Gtest uses snprintf as GTEST_SNPRINTF_ with sizeof
+        Begin->compare_lower(StringRef("testing")) == 0) {
+      return true;
+    }
+    if (Begin->compare_lower(StringRef("webrtc")) == 0) {
+      // Ignore trunk/webrtc, but not media/webrtc
+      ++Begin;
+      return Begin != End && Begin->compare_lower(StringRef("trunk")) == 0;
+    }
+  }
+  return false;
+}
+
 bool isInterestingDeclForImplicitConversion(const Decl *Declaration) {
   return !isInIgnoredNamespaceForImplicitConversion(Declaration) &&
          !isIgnoredPathForImplicitConversion(Declaration);
 }
 
 bool isIgnoredExprForMustUse(const Expr *E) {
   if (const CXXOperatorCallExpr *OpCall = dyn_cast<CXXOperatorCallExpr>(E)) {
     switch (OpCall->getOperator()) {
@@ -856,16 +899,33 @@ AST_MATCHER(CallExpr, isAssertAssignment
   static const std::string AssertName = "MOZ_AssertAssignmentTest";
   const FunctionDecl *Method = Node.getDirectCallee();
 
   return Method
       && Method->getDeclName().isIdentifier()
       && Method->getName() == AssertName;
 }
 
+AST_MATCHER(CallExpr, isSnprintfLikeFunc) {
+  static const std::string Snprintf = "snprintf";
+  static const std::string Vsnprintf = "vsnprintf";
+  const FunctionDecl *Func = Node.getDirectCallee();
+
+  if (!Func || isa<CXXMethodDecl>(Func)) {
+    return false;
+  }
+
+  StringRef Name = getNameChecked(Func);
+  if (Name != Snprintf && Name != Vsnprintf) {
+    return false;
+  }
+
+  return !isIgnoredPathForSprintfLiteral(&Node, Finder->getASTContext().getSourceManager());
+}
+
 AST_MATCHER(CXXRecordDecl, isLambdaDecl) {
   return Node.isLambda();
 }
 
 AST_MATCHER(QualType, isRefPtr) {
   return typeIsRefPtr(Node);
 }
 }
@@ -1086,19 +1146,19 @@ DiagnosticsMatcher::DiagnosticsMatcher()
       &ArithmeticArg);
 
   AstMatcher.addMatcher(cxxRecordDecl(hasTrivialCtorDtor()).bind("node"),
                         &TrivialCtorDtor);
 
   AstMatcher.addMatcher(
       binaryOperator(
           allOf(binaryEqualityOperator(),
-                hasLHS(has(
+                hasLHS(hasIgnoringParenImpCasts(
                     declRefExpr(hasType(qualType((isFloat())))).bind("lhs"))),
-                hasRHS(has(
+                hasRHS(hasIgnoringParenImpCasts(
                     declRefExpr(hasType(qualType((isFloat())))).bind("rhs"))),
                 unless(anyOf(isInSystemHeader(), isInSkScalarDotH()))))
           .bind("node"),
       &NaNExpr);
 
   // First, look for direct parents of the MemberExpr.
   AstMatcher.addMatcher(
       callExpr(
@@ -1190,16 +1250,27 @@ DiagnosticsMatcher::DiagnosticsMatcher()
       &RefCountedCopyConstructor);
 
   AstMatcher.addMatcher(
       callExpr(isAssertAssignmentTestFunc()).bind("funcCall"),
       &AssertAttribution);
 
   AstMatcher.addMatcher(varDecl(hasType(isRefPtr())).bind("decl"),
                         &KungFuDeathGrip);
+
+  AstMatcher.addMatcher(
+      callExpr(isSnprintfLikeFunc(),
+        allOf(hasArgument(0, ignoringParenImpCasts(declRefExpr().bind("buffer"))),
+                             anyOf(hasArgument(1, sizeOfExpr(hasIgnoringParenImpCasts(declRefExpr().bind("size")))),
+                                   hasArgument(1, integerLiteral().bind("immediate")),
+                                   hasArgument(1, declRefExpr(to(varDecl(hasType(isConstQualified()),
+                                                                         hasInitializer(integerLiteral().bind("constant")))))))))
+        .bind("funcCall"),
+      &SprintfLiteral
+  );
 }
 
 // These enum variants determine whether an allocation has occured in the code.
 enum AllocationVariety {
   AV_None,
   AV_Global,
   AV_Automatic,
   AV_Temporary,
@@ -1829,16 +1900,71 @@ void DiagnosticsMatcher::KungFuDeathGrip
     NoteThing = "value";
   }
 
   // We cannot provide the note if we don't have an initializer
   Diag.Report(D->getLocStart(), ErrorID) << D->getType() << ErrThing;
   Diag.Report(E->getLocStart(), NoteID) << NoteThing << getNameChecked(D);
 }
 
+void DiagnosticsMatcher::SprintfLiteralChecker::run(
+    const MatchFinder::MatchResult &Result) {
+  if (!Result.Context->getLangOpts().CPlusPlus) {
+    // SprintfLiteral is not usable in C, so there is no point in issuing these
+    // warnings.
+    return;
+  }
+
+  DiagnosticsEngine &Diag = Result.Context->getDiagnostics();
+  unsigned ErrorID = Diag.getDiagnosticIDs()->getCustomDiagID(
+    DiagnosticIDs::Error, "Use %1 instead of %0 when writing into a character array.");
+  unsigned NoteID = Diag.getDiagnosticIDs()->getCustomDiagID(
+    DiagnosticIDs::Note, "This will prevent passing in the wrong size to %0 accidentally.");
+
+  const CallExpr *D = Result.Nodes.getNodeAs<CallExpr>("funcCall");
+
+  StringRef Name = D->getDirectCallee()->getName();
+  const char *Replacement;
+  if (Name == "snprintf") {
+    Replacement = "SprintfLiteral";
+  } else {
+    assert(Name == "vsnprintf");
+    Replacement = "VsprintfLiteral";
+  }
+
+  const DeclRefExpr *Buffer = Result.Nodes.getNodeAs<DeclRefExpr>("buffer");
+  const DeclRefExpr *Size = Result.Nodes.getNodeAs<DeclRefExpr>("size");
+  if (Size) {
+    // Match calls like snprintf(x, sizeof(x), ...).
+    if (Buffer->getFoundDecl() != Size->getFoundDecl()) {
+      return;
+    }
+
+    Diag.Report(D->getLocStart(), ErrorID) << Name << Replacement;
+    Diag.Report(D->getLocStart(), NoteID) << Name;
+    return;
+  }
+
+  const QualType QType = Buffer->getType();
+  const ConstantArrayType *Type = dyn_cast<ConstantArrayType>(QType.getTypePtrOrNull());
+  if (Type) {
+    // Match calls like snprintf(x, 100, ...), where x is int[100];
+    const IntegerLiteral *Literal = Result.Nodes.getNodeAs<IntegerLiteral>("immediate");
+    if (!Literal) {
+      // Match calls like: const int y = 100; snprintf(x, y, ...);
+      Literal = Result.Nodes.getNodeAs<IntegerLiteral>("constant");
+    }
+
+    if (Type->getSize().ule(Literal->getValue())) {
+      Diag.Report(D->getLocStart(), ErrorID) << Name << Replacement;
+      Diag.Report(D->getLocStart(), NoteID) << Name;
+    }
+  }
+}
+
 class MozCheckAction : public PluginASTAction {
 public:
   ASTConsumerPtr CreateASTConsumer(CompilerInstance &CI,
                                    StringRef FileName) override {
 #if CLANG_VERSION_FULL >= 306
     std::unique_ptr<MozChecker> Checker(llvm::make_unique<MozChecker>(CI));
     ASTConsumerPtr Other(Checker->getOtherConsumer());
 
new file mode 100644
--- /dev/null
+++ b/build/clang-plugin/tests/TestSprintfLiteral.cpp
@@ -0,0 +1,47 @@
+#include <cstdio>
+
+void bad() {
+  char x[100];
+  snprintf(x, sizeof(x), "bar"); // expected-error {{Use SprintfLiteral instead of snprintf when writing into a character array.}} expected-note {{This will prevent passing in the wrong size to snprintf accidentally.}}
+  snprintf(x, 100, "bar"); // expected-error {{Use SprintfLiteral instead of snprintf when writing into a character array.}} expected-note {{This will prevent passing in the wrong size to snprintf accidentally.}}
+  snprintf(x, 101, "bar"); // expected-error {{Use SprintfLiteral instead of snprintf when writing into a character array.}} expected-note {{This will prevent passing in the wrong size to snprintf accidentally.}}
+  const int hundred = 100;
+  snprintf(x, hundred, "bar"); // expected-error {{Use SprintfLiteral instead of snprintf when writing into a character array.}} expected-note {{This will prevent passing in the wrong size to snprintf accidentally.}}
+  const int hundredandone = 101;
+  snprintf(x, hundredandone, "bar"); // expected-error {{Use SprintfLiteral instead of snprintf when writing into a character array.}} expected-note {{This will prevent passing in the wrong size to snprintf accidentally.}}
+}
+
+void ok() {
+  char x[100];
+  int y;
+  snprintf(x, sizeof(y), "what");
+
+  snprintf(x, 50, "what");
+
+  int nothundred = 100;
+  nothundred = 99;
+  snprintf(x, nothundred, "what");
+}
+
+void vargs_bad(va_list args) {
+  char x[100];
+  vsnprintf(x, sizeof(x), "bar", args); // expected-error {{Use VsprintfLiteral instead of vsnprintf when writing into a character array.}} expected-note {{This will prevent passing in the wrong size to vsnprintf accidentally.}}
+  vsnprintf(x, 100, "bar", args); // expected-error {{Use VsprintfLiteral instead of vsnprintf when writing into a character array.}} expected-note {{This will prevent passing in the wrong size to vsnprintf accidentally.}}
+  vsnprintf(x, 101, "bar", args); // expected-error {{Use VsprintfLiteral instead of vsnprintf when writing into a character array.}} expected-note {{This will prevent passing in the wrong size to vsnprintf accidentally.}}
+  const int hundred = 100;
+  vsnprintf(x, hundred, "bar", args); // expected-error {{Use VsprintfLiteral instead of vsnprintf when writing into a character array.}} expected-note {{This will prevent passing in the wrong size to vsnprintf accidentally.}}
+  const int hundredandone = 101;
+  vsnprintf(x, hundredandone, "bar", args); // expected-error {{Use VsprintfLiteral instead of vsnprintf when writing into a character array.}} expected-note {{This will prevent passing in the wrong size to vsnprintf accidentally.}}
+}
+
+void vargs_good(va_list args) {
+  char x[100];
+  int y;
+  vsnprintf(x, sizeof(y), "what", args);
+
+  vsnprintf(x, 50, "what", args);
+
+  int nothundred = 100;
+  nothundred = 99;
+  vsnprintf(x, nothundred, "what", args);
+}
--- a/build/clang-plugin/tests/moz.build
+++ b/build/clang-plugin/tests/moz.build
@@ -28,14 +28,15 @@ SOURCES += [
     'TestNoDuplicateRefCntMember.cpp',
     'TestNoExplicitMoveConstructor.cpp',
     'TestNonHeapClass.cpp',
     'TestNonMemMovable.cpp',
     'TestNonMemMovableStd.cpp',
     'TestNonTemporaryClass.cpp',
     'TestNoRefcountedInsideLambdas.cpp',
     'TestRefCountedCopyConstructor.cpp',
+    'TestSprintfLiteral.cpp',
     'TestStackClass.cpp',
     'TestTrivialCtorDtor.cpp',
 ]
 
 DISABLE_STL_WRAPPING = True
 NO_VISIBILITY_FLAGS = True