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 359307 b66170ee0000101ccda9b4887ff41d2d8ecddc07
parent 359306 f3f755f8c1e472b093b053688f7431eb3e8933e4
child 359308 628c7aa9630fa150df658132d475647816a02f04
push id6795
push userjlund@mozilla.com
push dateMon, 23 Jan 2017 14:19:46 +0000
treeherdermozilla-beta@76101b503191 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersehsan
bugs1302163
milestone52.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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