[clang] Fortify warning for scanf calls with field width too big.

[enh]

"shut up and take my money!" :-)

clang/lib/Sema/SemaChecking.cpp

664

(stray tabs?)

[mbenfield]

clang/lib/Sema/SemaChecking.cpp

664

Not sure what you're referring to. AFAICT there are no tabs in this file.

[enh]

clang/lib/Sema/SemaChecking.cpp

664

oh, gerrit uses the >> symbol to mean "tab". is this tool just saying "indentation changed", not specifically "someone added a tab"?

[mbenfield]

clang/lib/Sema/SemaChecking.cpp

664

Ah I see. Yes, I think that's correct, that it doesn't mean a tab here.

[george.burgess.iv]

LGTM % nits -- thanks for this! :)

clang/include/clang/Basic/DiagnosticSemaKinds.td

836

nit: s/Warning </Warning</

838

nit: please s/null/NUL/ for consistency with elsewhere

clang/lib/Sema/SemaChecking.cpp

415

optional: llvm generally prefers FunctionRefs for simplicity and speed. if it's easy to refactor to use those (seems like it may be), please do. otherwise, it's not a big deal.

[mbenfield]

respond to comments: null to NUL, remove stray space, use function_ref

[george.burgess.iv]

LGTM. Thanks again!

clang/lib/Sema/SemaChecking.cpp

735

nit: const auto if possible (and below)

756

Please put this in a variable and pass that into H's constructor. function_ref doesn't own the function it points to, so any use of this at line >= 758 (e.g., line 768) is a use-after-free.

[mbenfield]

rebase and rerun tests

[mbenfield]

rebase and rerun tests

[mbenfield]

fix function_ref use-after-free

[mbenfield]

const

[thakis]

This doesn't seem to be working very well:

thakis@thakis:~/src/llvm-project$ cat test.cc
#include <inttypes.h>
#include <stdio.h>
#include <stdint.h>

int main() {
  uint16_t hextets[8];
  int chars_scanned;
  char buf[] = "1234:5678:9abc:def0:1234:5678:9abc:def0";
  sscanf(buf,
	 "%4" SCNx16 ":%4" SCNx16 ":%4" SCNx16 ":%4" SCNx16 ":%4" SCNx16
	 ":%4" SCNx16 ":%4" SCNx16 ":%4" SCNx16 "%n",
	 &hextets[0], &hextets[1], &hextets[2], &hextets[3], &hextets[4],
	 &hextets[5], &hextets[6], &hextets[7], &chars_scanned);

  for (int i = 0; i < 8; ++i)
    printf("%x ", hextets[i]);
  printf("%d\n", chars_scanned);
}
thakis@thakis:~/src/llvm-project$ out/gn/bin/clang test.cc -Wall
test.cc:9:3: warning: 'sscanf' may overflow; destination buffer in argument 9 has size 4, but the corresponding field width plus NUL byte is 5 [-Wfortify-source]
  sscanf(buf,
  ^
test.cc:9:3: warning: 'sscanf' may overflow; destination buffer in argument 10 has size 2, but the corresponding field width plus NUL byte is 5 [-Wfortify-source]
2 warnings generated.
thakis@thakis:~/src/llvm-project$ ./a.out 
1234 5678 9abc def0 1234 5678 9abc def0 39

1. The warning is emitted twice, but doesn't point at code the 2nd time round
2. That code looks correct to me (ie there shouldn't be any warnings), maybe %n isn't handled correctly?
3. The diag points at the start of the scanf instead of at the faulty arg.

Especially 2 is breaking builds, so I'll revert this for now. Looks like a cool warning though, looking forward to the relanding :)

[mbenfield]

Only diagnose if conversion specifier is %s.

Give the location of the particular argument rather than the location of the function call.

Test to make sure we don't warn on %d.

[mbenfield]

Thanks for reverting. Here's another try.

This just shouldn't be warning on any specifier other than %s, so I fixed that.

As far as not pointing at code for the second warning, I verified manually that it does now point at code even when warning twice, but it doesn't seem to be feasible to include a test for that.

[enh]

add tests for %c and %[ too? (it's genuinely unclear to me from a quick skim whether this patch covers them.)

[mbenfield]

Support %c and %[ specifiers.

Changed the diagnostic message to accommodate.

[mbenfield]

Previously this patch did not cover %c and %[, but now it does.

[enh]

In D111833#3094868, @mbenfield wrote:

Previously this patch did not cover %c and %[, but now it does.

thanks!

[mbenfield]

Ignore specifiers with *.

clang/include/clang/Basic/DiagnosticSemaKinds.td

def warn_fortify_strlen_overflow: Warning<
   " but the source string has length %2 (including NUL byte)">,
   InGroup<FortifySource>;
 
 def warn_fortify_source_format_overflow : Warning<
   "'%0' will always overflow; destination buffer has size %1,"
   " but format string expands to at least %2">,
   InGroup<FortifySource>;
 
+def warn_fortify_scanf_overflow : Warning<
+  "'%0' may overflow; destination buffer in argument %1 has size "
+  "%2, but the corresponding field width plus NUL byte is %3">,
+  InGroup<FortifySource>;
 
 /// main()
 // static main() is not an error in C, just in C++.
 def warn_static_main : Warning<"'main' should not be declared static">,
     InGroup<Main>;
 def err_static_main : Error<"'main' is not allowed to be declared static">;
 def err_inline_main : Error<"'main' is not allowed to be declared inline">;
 def ext_variadic_main : ExtWarn<

clang/lib/Sema/SemaChecking.cpp

static bool SemaBuiltinCallWithStaticChain(Sema &S, CallExpr *BuiltinCall) {
   BuiltinCall->setCallee(Builtin);
   BuiltinCall->setArg(1, ChainResult.get());
 
   return false;
 }
 
 namespace {
 
+class ScanfDiagnosticFormatHandler
+    : public analyze_format_string::FormatStringHandler {
+  // Accepts the argument index (relative to the first destination index) of the
+  // argument whose size we want.
+  using ComputeSizeFunction =
+      llvm::function_ref<Optional<llvm::APSInt>(unsigned)>;
+
+  // Accepts the argument index (relative to the first destination index), the
+  // destination size, and the source size).
+  using DiagnoseFunction =
+      llvm::function_ref<void(unsigned, unsigned, unsigned)>;
+
+  ComputeSizeFunction ComputeSizeArgument;
+  DiagnoseFunction Diagnose;
+
+public:
+  ScanfDiagnosticFormatHandler(ComputeSizeFunction ComputeSizeArgument,
+                               DiagnoseFunction Diagnose)
+      : ComputeSizeArgument(ComputeSizeArgument), Diagnose(Diagnose) {}
+
+  bool HandleScanfSpecifier(const analyze_scanf::ScanfSpecifier &FS,
+                            const char *StartSpecifier,
+                            unsigned specifierLen) override {
+    auto OptionalFW = FS.getFieldWidth();
+    if (OptionalFW.getHowSpecified() !=
+        analyze_format_string::OptionalAmount::HowSpecified::Constant)
+      return true;
+
+    // We have to write the data plus a NUL byte.
+    unsigned SourceSize = OptionalFW.getConstantAmount() + 1;
+
+    auto DestSizeAPS = ComputeSizeArgument(FS.getArgIndex());
+    if (!DestSizeAPS)
+      return true;
+
+    unsigned DestSize = DestSizeAPS->getZExtValue();
+
+    if (DestSize < SourceSize)
+      Diagnose(FS.getArgIndex(), DestSize, SourceSize);
+
+    return true;
+  }
+};
+
 class EstimateSizeFormatHandler
     : public analyze_format_string::FormatStringHandler {
   size_t Size;
 
 public:
   EstimateSizeFormatHandler(StringRef Format)
       : Size(std::min(Format.find(0), Format.size()) +
              1 /* null byte always written by sprintf */) {}
auto ComputeExplicitObjectSizeArgument =
     return Result.Val.getInt();
   };
 
   auto ComputeSizeArgument = [&](unsigned Index) -> Optional<llvm::APSInt> {
     // If the parameter has a pass_object_size attribute, then we should use its
     // (potentially) more strict checking mode. Otherwise, conservatively assume
     // type 0.
     int BOSType = 0;
+    // This check can fail for variadic functions.
+    if (Index < FD->getNumParams()) {
       if (const auto *POS =
               FD->getParamDecl(Index)->getAttr<PassObjectSizeAttr>())
         BOSType = POS->getType();
+    }
 
     const Expr *ObjArg = TheCall->getArg(Index);
     uint64_t Result;
     if (!ObjArg->tryEvaluateObjectSize(Result, getASTContext(), BOSType))
       return llvm::None;
 
     // Get the object size in the target's size_t width.
     return llvm::APSInt::getUnsigned(Result).extOrTrunc(SizeTypeWidth);
   };
 
   auto ComputeStrLenArgument = [&](unsigned Index) -> Optional<llvm::APSInt> {
     Expr *ObjArg = TheCall->getArg(Index);
     uint64_t Result;
     if (!ObjArg->tryEvaluateStrLen(Result, getASTContext()))
       return llvm::None;
     // Add 1 for null byte.
     return llvm::APSInt::getUnsigned(Result + 1).extOrTrunc(SizeTypeWidth);
   };
 
   Optional<llvm::APSInt> SourceSize;
   Optional<llvm::APSInt> DestinationSize;
   unsigned DiagID = 0;
   bool IsChkVariant = false;
 
+  auto GetFunctionName = [&]() {
+    StringRef FunctionName = getASTContext().BuiltinInfo.getName(BuiltinID);
+    // Skim off the details of whichever builtin was called to produce a better
+    // diagnostic, as it's unlikely that the user wrote the __builtin
+    // explicitly.
+    if (IsChkVariant) {
+      FunctionName = FunctionName.drop_front(std::strlen("__builtin___"));
+      FunctionName = FunctionName.drop_back(std::strlen("_chk"));
+    } else if (FunctionName.startswith("__builtin_")) {
+      FunctionName = FunctionName.drop_front(std::strlen("__builtin_"));
+    }
+    return FunctionName;
+  };
+
   switch (BuiltinID) {
   default:
     return;
   case Builtin::BI__builtin_strcpy:
   case Builtin::BIstrcpy: {
     DiagID = diag::warn_fortify_strlen_overflow;
     SourceSize = ComputeStrLenArgument(1);
     DestinationSize = ComputeSizeArgument(0);
     break;
   }
 
   case Builtin::BI__builtin___strcpy_chk: {
     DiagID = diag::warn_fortify_strlen_overflow;
     SourceSize = ComputeStrLenArgument(1);
     DestinationSize = ComputeExplicitObjectSizeArgument(2);
     IsChkVariant = true;
     break;
   }
 
+  case Builtin::BIscanf:
+  case Builtin::BIfscanf:
+  case Builtin::BIsscanf: {
+    unsigned FormatIndex = 1;
+    unsigned DataIndex = 2;
+    if (BuiltinID == Builtin::BIscanf) {
+      FormatIndex = 0;
+      DataIndex = 1;
+    }
+
+    const auto *FormatExpr =
+        TheCall->getArg(FormatIndex)->IgnoreParenImpCasts();
+
+    const auto *Format = dyn_cast<StringLiteral>(FormatExpr);
+    if (!Format)
+      return;
+
+    if (!Format->isAscii() && !Format->isUTF8())
+      return;
+
+    auto Diagnose = [&](unsigned ArgIndex, unsigned DestSize,
+                        unsigned SourceSize) {
+      DiagID = diag::warn_fortify_scanf_overflow;
+      StringRef FunctionName = GetFunctionName();
+      DiagRuntimeBehavior(TheCall->getBeginLoc(), TheCall,
+                          PDiag(DiagID)
+                              << FunctionName << (ArgIndex + DataIndex + 1)
+                              << DestSize << SourceSize);
+    };
+
+    StringRef FormatStrRef = Format->getString();
+    auto ShiftedComputeSizeArgument = [&](unsigned Index) {
+      return ComputeSizeArgument(Index + DataIndex);
+    };
+    ScanfDiagnosticFormatHandler H(ShiftedComputeSizeArgument, Diagnose);
+    const char *FormatBytes = FormatStrRef.data();
+    const ConstantArrayType *T =
+        Context.getAsConstantArrayType(Format->getType());
+    assert(T && "String literal not of constant array type!");
+    size_t TypeSize = T->getSize().getZExtValue();
+
+    // In case there's a null byte somewhere.
+    size_t StrLen =
+        std::min(std::max(TypeSize, size_t(1)) - 1, FormatStrRef.find(0));
+
+    analyze_format_string::ParseScanfString(H, FormatBytes,
+                                            FormatBytes + StrLen, getLangOpts(),
+                                            Context.getTargetInfo());
+
+    // Unlike the other cases, in this one we have already issued the diagnostic
+    // here, so no need to continue (because unlike the other cases, here the
+    // diagnostic refers to the argument number).
+    return;
+  }
+
   case Builtin::BIsprintf:
   case Builtin::BI__builtin___sprintf_chk: {
     size_t FormatIndex = BuiltinID == Builtin::BIsprintf ? 1 : 3;
     auto *FormatExpr = TheCall->getArg(FormatIndex)->IgnoreParenImpCasts();
 
     if (auto *Format = dyn_cast<StringLiteral>(FormatExpr)) {
 
       if (!Format->isAscii() && !Format->isUTF8())
case Builtin::BI__builtin_vsnprintf: {
     break;
   }
   }
 
   if (!SourceSize || !DestinationSize ||
       SourceSize.getValue().ule(DestinationSize.getValue()))
     return;
 
-  StringRef FunctionName = getASTContext().BuiltinInfo.getName(BuiltinID);
-  // Skim off the details of whichever builtin was called to produce a better
-  // diagnostic, as it's unlikely that the user wrote the __builtin explicitly.
-  if (IsChkVariant) {
-    FunctionName = FunctionName.drop_front(std::strlen("__builtin___"));
-    FunctionName = FunctionName.drop_back(std::strlen("_chk"));
-  } else if (FunctionName.startswith("__builtin_")) {
-    FunctionName = FunctionName.drop_front(std::strlen("__builtin_"));
-  }
+  StringRef FunctionName = GetFunctionName();
 
   SmallString<16> DestinationStr;
   SmallString<16> SourceStr;
   DestinationSize->toString(DestinationStr, /*Radix=*/10);
   SourceSize->toString(SourceStr, /*Radix=*/10);
   DiagRuntimeBehavior(TheCall->getBeginLoc(), TheCall,
                       PDiag(DiagID)
                           << FunctionName << DestinationStr << SourceStr);

clang/test/Sema/warn-fortify-scanf.c

+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify
+
+typedef struct _FILE FILE;
+extern int scanf(const char *format, ...);
+extern int fscanf(FILE *f, const char *format, ...);
+extern int sscanf(const char *input, const char *format, ...);
+
+void call_scanf() {
+  char buf10[10];
+  char buf20[20];
+  char buf30[30];
+  scanf("%4s %5s %10s", buf20, buf30, buf10); // expected-warning {{'scanf' may overflow; destination buffer in argument 4 has size 10, but the corresponding field width plus NUL byte is 11}}
+  scanf("%4s %5s %11s", buf20, buf30, buf10); // expected-warning {{'scanf' may overflow; destination buffer in argument 4 has size 10, but the corresponding field width plus NUL byte is 12}}
+  scanf("%4s %5s %9s", buf20, buf30, buf10);
+  scanf("%20s %5s %9s", buf20, buf30, buf10); // expected-warning {{'scanf' may overflow; destination buffer in argument 2 has size 20, but the corresponding field width plus NUL byte is 21}}
+  scanf("%21s %5s %9s", buf20, buf30, buf10); // expected-warning {{'scanf' may overflow; destination buffer in argument 2 has size 20, but the corresponding field width plus NUL byte is 22}}
+  scanf("%19s %5s %9s", buf20, buf30, buf10);
+  scanf("%19s %29s %9s", buf20, buf30, buf10);
+}
+
+void call_sscanf() {
+  char buf10[10];
+  char buf20[20];
+  char buf30[30];
+  sscanf("a b c", "%4s %5s %10s", buf20, buf30, buf10); // expected-warning {{'sscanf' may overflow; destination buffer in argument 5 has size 10, but the corresponding field width plus NUL byte is 11}}
+  sscanf("a b c", "%4s %5s %11s", buf20, buf30, buf10); // expected-warning {{'sscanf' may overflow; destination buffer in argument 5 has size 10, but the corresponding field width plus NUL byte is 12}}
+  sscanf("a b c", "%4s %5s %9s", buf20, buf30, buf10);
+  sscanf("a b c", "%20s %5s %9s", buf20, buf30, buf10); // expected-warning {{'sscanf' may overflow; destination buffer in argument 3 has size 20, but the corresponding field width plus NUL byte is 21}}
+  sscanf("a b c", "%21s %5s %9s", buf20, buf30, buf10); // expected-warning {{'sscanf' may overflow; destination buffer in argument 3 has size 20, but the corresponding field width plus NUL byte is 22}}
+  sscanf("a b c", "%19s %5s %9s", buf20, buf30, buf10);
+  sscanf("a b c", "%19s %29s %9s", buf20, buf30, buf10);
+}
+
+void call_fscanf() {
+  char buf10[10];
+  char buf20[20];
+  char buf30[30];
+  fscanf(0, "%4s %5s %10s", buf20, buf30, buf10); // expected-warning {{'fscanf' may overflow; destination buffer in argument 5 has size 10, but the corresponding field width plus NUL byte is 11}}
+  fscanf(0, "%4s %5s %11s", buf20, buf30, buf10); // expected-warning {{'fscanf' may overflow; destination buffer in argument 5 has size 10, but the corresponding field width plus NUL byte is 12}}
+  fscanf(0, "%4s %5s %9s", buf20, buf30, buf10);
+  fscanf(0, "%20s %5s %9s", buf20, buf30, buf10); // expected-warning {{'fscanf' may overflow; destination buffer in argument 3 has size 20, but the corresponding field width plus NUL byte is 21}}
+  fscanf(0, "%21s %5s %9s", buf20, buf30, buf10); // expected-warning {{'fscanf' may overflow; destination buffer in argument 3 has size 20, but the corresponding field width plus NUL byte is 22}}
+  fscanf(0, "%19s %5s %9s", buf20, buf30, buf10);
+  fscanf(0, "%19s %29s %9s", buf20, buf30, buf10);
+}