]> granicus.if.org Git - clang/commitdiff
Add a format warning for "%p" with non-void* args
authorSeth Cantrell <seth.cantrell@gmail.com>
Wed, 4 Mar 2015 03:12:10 +0000 (03:12 +0000)
committerSeth Cantrell <seth.cantrell@gmail.com>
Wed, 4 Mar 2015 03:12:10 +0000 (03:12 +0000)
GCC -pedantic produces a format warning when the "%p" specifier is used with
arguments that are not void*. It's useful for portability to be able to
catch such warnings with clang as well. The warning is off by default in
both gcc and with this patch. This patch enables it either when extensions
are disabled with -pedantic, or with the specific flag -Wformat-pedantic.

The C99 and C11 specs do appear to require arguments corresponding to 'p'
specifiers to be void*: "If any argument is not the correct type for the
corresponding conversion specification, the behavior is undefined."
[7.19.6.1 p9], and of the 'p' format specifier "The argument shall be a
pointer to void." [7.19.6.1 p8]

Both printf and scanf format checking are covered.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@231211 91177308-0d34-0410-b5e6-96231b3b80d8

include/clang/Analysis/Analyses/FormatString.h
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
lib/Analysis/FormatString.cpp
lib/Sema/SemaChecking.cpp
test/SemaCXX/format-strings-0x.cpp

index d67aa5171372b5b73d145d40192ae9d613f40924..64e9c34a673dd610419ab65ce50e1d0d5f053f32 100644 (file)
@@ -231,6 +231,9 @@ class ArgType {
 public:
   enum Kind { UnknownTy, InvalidTy, SpecificTy, ObjCPointerTy, CPointerTy,
               AnyCharTy, CStrTy, WCStrTy, WIntTy };
+
+  enum MatchKind { NoMatch = 0, Match = 1, NoMatchPedantic };
+
 private:
   const Kind K;
   QualType T;
@@ -254,7 +257,7 @@ public:
     return Res;
   }
 
-  bool matchesType(ASTContext &C, QualType argTy) const;
+  MatchKind matchesType(ASTContext &C, QualType argTy) const;
 
   QualType getRepresentativeType(ASTContext &C) const;
 
index 215ff06ddb26ee35035032a732c2184e1f08b848..5affa28f0ac3995bd0d7365cba71004f6ad3ee63 100644 (file)
@@ -551,6 +551,7 @@ def FormatInvalidSpecifier : DiagGroup<"format-invalid-specifier">;
 def FormatSecurity : DiagGroup<"format-security">;
 def FormatNonStandard : DiagGroup<"format-non-iso">;
 def FormatY2K : DiagGroup<"format-y2k">;
+def FormatPedantic : DiagGroup<"format-pedantic">;
 def Format : DiagGroup<"format",
                        [FormatExtraArgs, FormatZeroLength, NonNull,
                         FormatSecurity, FormatY2K, FormatInvalidSpecifier]>,
index 16d9337574110778222ef9e6e5e49f63b6b62e34..878c109717bbccbd9bf26b495cc1e91fa87f5dd6 100644 (file)
@@ -6644,6 +6644,10 @@ def warn_format_conversion_argument_type_mismatch : Warning<
   "format specifies type %0 but the argument has "
   "%select{type|underlying type}2 %1">,
   InGroup<Format>;
+def warn_format_conversion_argument_type_mismatch_pedantic : Extension<
+  "format specifies type %0 but the argument has "
+  "%select{type|underlying type}2 %1">,
+  InGroup<FormatPedantic>;
 def warn_format_argument_needs_cast : Warning<
   "%select{values of type|enum values with underlying type}2 '%0' should not "
   "be used as format arguments; add an explicit cast to %1 instead">,
index 662166ccaac92b77ecc80e76ac032926b84bf085..1b608941f0dcfab9d0ebbd94fdfd5bb386158501 100644 (file)
@@ -256,16 +256,17 @@ clang::analyze_format_string::ParseLengthModifier(FormatSpecifier &FS,
 // Methods on ArgType.
 //===----------------------------------------------------------------------===//
 
-bool ArgType::matchesType(ASTContext &C, QualType argTy) const {
+clang::analyze_format_string::ArgType::MatchKind
+ArgType::matchesType(ASTContext &C, QualType argTy) const {
   if (Ptr) {
     // It has to be a pointer.
     const PointerType *PT = argTy->getAs<PointerType>();
     if (!PT)
-      return false;
+      return NoMatch;
 
     // We cannot write through a const qualified pointer.
     if (PT->getPointeeType().isConstQualified())
-      return false;
+      return NoMatch;
 
     argTy = PT->getPointeeType();
   }
@@ -275,8 +276,8 @@ bool ArgType::matchesType(ASTContext &C, QualType argTy) const {
       llvm_unreachable("ArgType must be valid");
 
     case UnknownTy:
-      return true;
-      
+      return Match;
+
     case AnyCharTy: {
       if (const EnumType *ETy = argTy->getAs<EnumType>())
         argTy = ETy->getDecl()->getIntegerType();
@@ -289,18 +290,18 @@ bool ArgType::matchesType(ASTContext &C, QualType argTy) const {
           case BuiltinType::SChar:
           case BuiltinType::UChar:
           case BuiltinType::Char_U:
-            return true;            
+            return Match;
         }
-      return false;
+      return NoMatch;
     }
-      
+
     case SpecificTy: {
       if (const EnumType *ETy = argTy->getAs<EnumType>())
         argTy = ETy->getDecl()->getIntegerType();
       argTy = C.getCanonicalType(argTy).getUnqualifiedType();
 
       if (T == argTy)
-        return true;
+        return Match;
       // Check for "compatible types".
       if (const BuiltinType *BT = argTy->getAs<BuiltinType>())
         switch (BT->getKind()) {
@@ -309,32 +310,33 @@ bool ArgType::matchesType(ASTContext &C, QualType argTy) const {
           case BuiltinType::Char_S:
           case BuiltinType::SChar:
           case BuiltinType::Char_U:
-          case BuiltinType::UChar:                    
-            return T == C.UnsignedCharTy || T == C.SignedCharTy;
+          case BuiltinType::UChar:
+            return T == C.UnsignedCharTy || T == C.SignedCharTy ? Match
+                                                                : NoMatch;
           case BuiltinType::Short:
-            return T == C.UnsignedShortTy;
+            return T == C.UnsignedShortTy ? Match : NoMatch;
           case BuiltinType::UShort:
-            return T == C.ShortTy;
+            return T == C.ShortTy ? Match : NoMatch;
           case BuiltinType::Int:
-            return T == C.UnsignedIntTy;
+            return T == C.UnsignedIntTy ? Match : NoMatch;
           case BuiltinType::UInt:
-            return T == C.IntTy;
+            return T == C.IntTy ? Match : NoMatch;
           case BuiltinType::Long:
-            return T == C.UnsignedLongTy;
+            return T == C.UnsignedLongTy ? Match : NoMatch;
           case BuiltinType::ULong:
-            return T == C.LongTy;
+            return T == C.LongTy ? Match : NoMatch;
           case BuiltinType::LongLong:
-            return T == C.UnsignedLongLongTy;
+            return T == C.UnsignedLongLongTy ? Match : NoMatch;
           case BuiltinType::ULongLong:
-            return T == C.LongLongTy;
+            return T == C.LongLongTy ? Match : NoMatch;
         }
-      return false;
+      return NoMatch;
     }
 
     case CStrTy: {
       const PointerType *PT = argTy->getAs<PointerType>();
       if (!PT)
-        return false;
+        return NoMatch;
       QualType pointeeTy = PT->getPointeeType();
       if (const BuiltinType *BT = pointeeTy->getAs<BuiltinType>())
         switch (BT->getKind()) {
@@ -343,50 +345,56 @@ bool ArgType::matchesType(ASTContext &C, QualType argTy) const {
           case BuiltinType::UChar:
           case BuiltinType::Char_S:
           case BuiltinType::SChar:
-            return true;
+            return Match;
           default:
             break;
         }
 
-      return false;
+      return NoMatch;
     }
 
     case WCStrTy: {
       const PointerType *PT = argTy->getAs<PointerType>();
       if (!PT)
-        return false;
+        return NoMatch;
       QualType pointeeTy =
         C.getCanonicalType(PT->getPointeeType()).getUnqualifiedType();
-      return pointeeTy == C.getWideCharType();
+      return pointeeTy == C.getWideCharType() ? Match : NoMatch;
     }
-    
+
     case WIntTy: {
-      
+
       QualType PromoArg = 
         argTy->isPromotableIntegerType()
           ? C.getPromotedIntegerType(argTy) : argTy;
-      
+
       QualType WInt = C.getCanonicalType(C.getWIntType()).getUnqualifiedType();
       PromoArg = C.getCanonicalType(PromoArg).getUnqualifiedType();
-      
+
       // If the promoted argument is the corresponding signed type of the
       // wint_t type, then it should match.
       if (PromoArg->hasSignedIntegerRepresentation() &&
           C.getCorrespondingUnsignedType(PromoArg) == WInt)
-        return true;
+        return Match;
 
-      return WInt == PromoArg;
+      return WInt == PromoArg ? Match : NoMatch;
     }
 
     case CPointerTy:
-      return argTy->isPointerType() || argTy->isObjCObjectPointerType() ||
-             argTy->isBlockPointerType() || argTy->isNullPtrType();
+      if (argTy->isVoidPointerType()) {
+        return Match;
+      } if (argTy->isPointerType() || argTy->isObjCObjectPointerType() ||
+            argTy->isBlockPointerType() || argTy->isNullPtrType()) {
+        return NoMatchPedantic;
+      } else {
+        return NoMatch;
+      }
 
     case ObjCPointerTy: {
       if (argTy->getAs<ObjCObjectPointerType>() ||
           argTy->getAs<BlockPointerType>())
-        return true;
-      
+        return Match;
+
       // Handle implicit toll-free bridging.
       if (const PointerType *PT = argTy->getAs<PointerType>()) {
         // Things such as CFTypeRef are really just opaque pointers
@@ -395,9 +403,9 @@ bool ArgType::matchesType(ASTContext &C, QualType argTy) const {
         // structs can be toll-free bridged, we just accept them all.
         QualType pointee = PT->getPointeeType();
         if (pointee->getAsStructureType() || pointee->isVoidType())
-          return true;
+          return Match;
       }
-      return false;      
+      return NoMatch;
     }
   }
 
index 01d82155f739f762251de47a6f2278e934a79309..72cb6003f5a172b6a617492a2698594fb5cba3af 100644 (file)
@@ -3669,8 +3669,11 @@ CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
     ExprTy = TET->getUnderlyingExpr()->getType();
   }
 
-  if (AT.matchesType(S.Context, ExprTy))
+  analyze_printf::ArgType::MatchKind match = AT.matchesType(S.Context, ExprTy);
+
+  if (match == analyze_printf::ArgType::Match) {
     return true;
+  }
 
   // Look through argument promotions for our error message's reported type.
   // This includes the integral and floating promotions, but excludes array
@@ -3848,15 +3851,18 @@ CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
     // arguments here.
     switch (S.isValidVarArgType(ExprTy)) {
     case Sema::VAK_Valid:
-    case Sema::VAK_ValidInCXX11:
+    case Sema::VAK_ValidInCXX11: {
+      unsigned diag = diag::warn_format_conversion_argument_type_mismatch;
+      if (match == analyze_printf::ArgType::NoMatchPedantic) {
+        diag = diag::warn_format_conversion_argument_type_mismatch_pedantic;
+      }
+
       EmitFormatDiagnostic(
-        S.PDiag(diag::warn_format_conversion_argument_type_mismatch)
-          << AT.getRepresentativeTypeName(S.Context) << ExprTy << IsEnum
-          << CSR
-          << E->getSourceRange(),
-        E->getLocStart(), /*IsStringLocation*/false, CSR);
+          S.PDiag(diag) << AT.getRepresentativeTypeName(S.Context) << ExprTy
+                        << IsEnum << CSR << E->getSourceRange(),
+          E->getLocStart(), /*IsStringLocation*/ false, CSR);
       break;
-
+    }
     case Sema::VAK_Undefined:
     case Sema::VAK_MSVCUndefined:
       EmitFormatDiagnostic(
@@ -3988,13 +3994,13 @@ bool CheckScanfHandler::HandleScanfSpecifier(
                            FixItHint::CreateRemoval(R));
     }
   }
-  
+
   if (!FS.consumesDataArgument()) {
     // FIXME: Technically specifying a precision or field width here
     // makes no sense.  Worth issuing a warning at some point.
     return true;
   }
-  
+
   // Consume the argument.
   unsigned argIndex = FS.getArgIndex();
   if (argIndex < NumDataArgs) {
@@ -4003,7 +4009,7 @@ bool CheckScanfHandler::HandleScanfSpecifier(
       // function if we encounter some other error.
     CoveredArgs.set(argIndex);
   }
-  
+
   // Check the length modifier is valid with the given conversion specifier.
   if (!FS.hasValidLengthModifier(S.getASTContext().getTargetInfo()))
     HandleInvalidLengthModifier(FS, CS, startSpecifier, specifierLen,
@@ -4020,21 +4026,28 @@ bool CheckScanfHandler::HandleScanfSpecifier(
   // The remaining checks depend on the data arguments.
   if (HasVAListArg)
     return true;
-  
+
   if (!CheckNumArgs(FS, CS, startSpecifier, specifierLen, argIndex))
     return false;
-  
+
   // Check that the argument type matches the format specifier.
   const Expr *Ex = getDataArg(argIndex);
   if (!Ex)
     return true;
 
   const analyze_format_string::ArgType &AT = FS.getArgType(S.Context);
-  if (AT.isValid() && !AT.matchesType(S.Context, Ex->getType())) {
+  analyze_format_string::ArgType::MatchKind match =
+      AT.matchesType(S.Context, Ex->getType());
+  if (AT.isValid() && match != analyze_format_string::ArgType::Match) {
     ScanfSpecifier fixedFS = FS;
-    bool success = fixedFS.fixType(Ex->getType(),
-                                   Ex->IgnoreImpCasts()->getType(),
-                                   S.getLangOpts(), S.Context);
+    bool success =
+        fixedFS.fixType(Ex->getType(), Ex->IgnoreImpCasts()->getType(),
+                        S.getLangOpts(), S.Context);
+
+    unsigned diag = diag::warn_format_conversion_argument_type_mismatch;
+    if (match == analyze_format_string::ArgType::NoMatchPedantic) {
+      diag = diag::warn_format_conversion_argument_type_mismatch_pedantic;
+    }
 
     if (success) {
       // Get the fix string from the fixed format specifier.
@@ -4043,23 +4056,20 @@ bool CheckScanfHandler::HandleScanfSpecifier(
       fixedFS.toString(os);
 
       EmitFormatDiagnostic(
-        S.PDiag(diag::warn_format_conversion_argument_type_mismatch)
-          << AT.getRepresentativeTypeName(S.Context) << Ex->getType() << false
-          << Ex->getSourceRange(),
-        Ex->getLocStart(),
-        /*IsStringLocation*/false,
-        getSpecifierRange(startSpecifier, specifierLen),
-        FixItHint::CreateReplacement(
+          S.PDiag(diag) << AT.getRepresentativeTypeName(S.Context)
+                        << Ex->getType() << false << Ex->getSourceRange(),
+          Ex->getLocStart(),
+          /*IsStringLocation*/ false,
           getSpecifierRange(startSpecifier, specifierLen),
-          os.str()));
+          FixItHint::CreateReplacement(
+              getSpecifierRange(startSpecifier, specifierLen), os.str()));
     } else {
       EmitFormatDiagnostic(
-        S.PDiag(diag::warn_format_conversion_argument_type_mismatch)
-          << AT.getRepresentativeTypeName(S.Context) << Ex->getType() << false
-          << Ex->getSourceRange(),
-        Ex->getLocStart(),
-        /*IsStringLocation*/false,
-        getSpecifierRange(startSpecifier, specifierLen));
+          S.PDiag(diag) << AT.getRepresentativeTypeName(S.Context)
+                        << Ex->getType() << false << Ex->getSourceRange(),
+          Ex->getLocStart(),
+          /*IsStringLocation*/ false,
+          getSpecifierRange(startSpecifier, specifierLen));
     }
   }
 
index 7e41c7fdbf9d50ff4a1b218c44b5512f2d78df6b..ad57b773e0a061fbfd641d40a1142e9a935f33d2 100644 (file)
@@ -8,6 +8,9 @@ extern int printf(const char *restrict, ...);
 void f(char **sp, float *fp) {
   scanf("%as", sp); // expected-warning{{format specifies type 'float *' but the argument has type 'char **'}}
 
+  printf("%p", sp); // expected-warning{{format specifies type 'void *' but the argument has type 'char **'}}
+  scanf("%p", sp);  // expected-warning{{format specifies type 'void **' but the argument has type 'char **'}}
+
   printf("%a", 1.0);
   scanf("%afoobar", fp);
   printf(nullptr);