[clang][Sema] Add truncation warning on fortified snprintf

[hazohelet]

Summary

This patch warns on snprintf calls whose n argument is known to be smaller than the size of the formatted string like

char buf[5];
snprintf(buf, 5, "Hello");

This is a counterpart of gcc's Wformat-truncation
Fixes https://github.com/llvm/llvm-project/issues/64871

[tbaeder]

Can't say much about the rest of the patch so I'd defer to someone else. Since gcc has -Wformat-truncation, can we disable this warning in clang as well? Can you add a test for that?

clang/lib/Sema/SemaChecking.cpp

1156–1157

1348

(Really not a fan of this, it adds nothing).

[nickdesaulniers]

Thanks for the patch!

clang/lib/Sema/SemaChecking.cpp

1154

naive question, so will we create a lambda with capture even though a few cases in the switch below will never use it?

1347

Can DestinationSize be calculated later, after the if (SourceSize && ...? Seems it's not needed unless that condition is false?

[hazohelet]

Address review comments

[hazohelet]

Gcc can diagnose wider cases of overflow/truncation by specifying a higher level for it, like -Wformat-overflow=2 (https://godbolt.org/z/n5facjW1c).
The current clang counterpart only diagnoses when the format string is always larger than the buffer size. If we are going to implement more general warnings like gcc's, I think we should separate these counterparts into their own warning flags (-Wformat-overflow/truncation).

clang/lib/Sema/SemaChecking.cpp

1154

I made it a static function.

[aaron.ballman]

Thank you for working on this!

Precommit CI found an issue with libc++'s tests: https://reviews.llvm.org/harbormaster/unit/view/8606248/ -- it'd be best if those could be addressed before landing the changes.

[hazohelet]

In D158562#4614835, @aaron.ballman wrote:

Thank you for working on this!

Precommit CI found an issue with libc++'s tests: https://reviews.llvm.org/harbormaster/unit/view/8606248/ -- it'd be best if those could be addressed before landing the changes.

Thanks for the heads up! I was missing that.
The reason for the false positive is that clang doesn't know that %g specifier can result in a print of only a single digit. (Live demo: https://godbolt.org/z/8aeqh1Wsf)

[hazohelet]

Fixes EstimateSizeFormatHandler's %g handling.

[aaron.ballman]

LGTM!

clang/lib/Sema/SemaChecking.cpp

1041

clang/docs/ReleaseNotes.rst

- Clang no longer emits ``-Wmissing-variable-declarations`` for variables declared
   with the ``register`` storage class.
 - Clang's ``-Wtautological-negation-compare`` flag now diagnoses logical
   tautologies like ``x && !x`` and ``!x || x`` in expressions. This also
   makes ``-Winfinite-recursion`` diagnose more cases.
   (`#56035: <https://github.com/llvm/llvm-project/issues/56035>`_).
 - Clang constexpr evaluator now diagnoses compound assignment operators against
   uninitialized variables as a read of uninitialized object.
   (`#51536 <https://github.com/llvm/llvm-project/issues/51536>_`)
+- Clang's ``-Wfortify-source`` now diagnoses ``snprintf`` call that is known to
+  result in string truncation.
+  (`#64871: <https://github.com/llvm/llvm-project/issues/64871>`_).
+  Also clang no longer emits false positive warnings about the output length of
+  ``%g`` format specifier.
 
 Bug Fixes in This Version
 -------------------------
 - Fixed an issue where a class template specialization whose declaration is
   instantiated in one module and whose definition is instantiated in another
   module may end up with members associated with the wrong declaration of the
   class, which can result in miscompiles in some cases.
 - Fix crash on use of a variadic overloaded operator.

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_source_format_truncation: Warning<
+  "'%0' will always be truncated; specified size is %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 specifier may require size %3">,
   InGroup<FortifySource>;
 
 def err_function_start_invalid_type: Error<
   "argument must be a function">;
 

clang/lib/Sema/SemaChecking.cpp

bool HandlePrintfSpecifier(const analyze_printf::PrintfSpecifier &FS,
     case analyze_format_string::ConversionSpecifier::uArg:
     case analyze_format_string::ConversionSpecifier::UArg:
     case analyze_format_string::ConversionSpecifier::xArg:
     case analyze_format_string::ConversionSpecifier::XArg:
       Size += std::max(FieldWidth, Precision);
       break;
 
     // %g style conversion switches between %f or %e style dynamically.
-    // %f always takes less space, so default to it.
+    // %g removes trailing zeros, and does not print decimal point if there are
+    // no digits that follow it. Thus %g can print a single digit.
     case analyze_format_string::ConversionSpecifier::gArg:
     case analyze_format_string::ConversionSpecifier::GArg:
+      Size += 1;
+      break;
 
     // Floating point number in the form '[+]ddd.ddd'.
     case analyze_format_string::ConversionSpecifier::fArg:
     case analyze_format_string::ConversionSpecifier::FArg:
       Size += std::max(FieldWidth, 1 /* integer part */ +
                                        (Precision ? 1 + Precision
                                                   : 0) /* period + decimal */);
       break;
default:
       break;
     }
     return Precision;
   }
 };
 
 } // namespace
 
+static bool ProcessFormatStringLiteral(const Expr *FormatExpr,
+                                       StringRef &FormatStrRef, size_t &StrLen,
+                                       ASTContext &Context) {
+  if (const auto *Format = dyn_cast<StringLiteral>(FormatExpr);
+      Format && (Format->isOrdinary() || Format->isUTF8())) {
+    FormatStrRef = Format->getString();
+    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.
+    StrLen = std::min(std::max(TypeSize, size_t(1)) - 1, FormatStrRef.find(0));
+    return true;
+  }
+  return false;
+}
+
 void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD,
                                                CallExpr *TheCall) {
   if (TheCall->isValueDependent() || TheCall->isTypeDependent() ||
       isConstantEvaluated())
     return;
 
   bool UseDABAttr = false;
   const FunctionDecl *UseDecl = FD;
auto ComputeStrLenArgument =
 
     const Expr *ObjArg = TheCall->getArg(NewIndex);
     uint64_t Result;
     if (!ObjArg->tryEvaluateStrLen(Result, getASTContext()))
       return std::nullopt;
     // Add 1 for null byte.
     return llvm::APSInt::getUnsigned(Result + 1).extOrTrunc(SizeTypeWidth);
   };
 
   std::optional<llvm::APSInt> SourceSize;
   std::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) {
case Builtin::BIsscanf: {
     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->isOrdinary() && !Format->isUTF8())
+    StringRef FormatStrRef;
+    size_t StrLen;
+    if (!ProcessFormatStringLiteral(FormatExpr, FormatStrRef, StrLen, Context))
       return;
 
     auto Diagnose = [&](unsigned ArgIndex, unsigned DestSize,
                         unsigned SourceSize) {
       DiagID = diag::warn_fortify_scanf_overflow;
       unsigned Index = ArgIndex + DataIndex;
       StringRef FunctionName = GetFunctionName();
       DiagRuntimeBehavior(TheCall->getArg(Index)->getBeginLoc(), TheCall,
                           PDiag(DiagID) << FunctionName << (Index + 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->isOrdinary() && !Format->isUTF8())
-        return;
-
-      StringRef FormatStrRef = Format->getString();
+    StringRef FormatStrRef;
+    size_t StrLen;
+    if (ProcessFormatStringLiteral(FormatExpr, FormatStrRef, StrLen, Context)) {
       EstimateSizeFormatHandler H(FormatStrRef);
       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));
       if (!analyze_format_string::ParsePrintfString(
               H, FormatBytes, FormatBytes + StrLen, getLangOpts(),
               Context.getTargetInfo(), false)) {
         DiagID = diag::warn_fortify_source_format_overflow;
         SourceSize = llvm::APSInt::getUnsigned(H.getSizeLowerBound())
                          .extOrTrunc(SizeTypeWidth);
         if (BuiltinID == Builtin::BI__builtin___sprintf_chk) {
           DestinationSize = ComputeExplicitObjectSizeArgument(2);
case Builtin::BI__builtin_mempcpy: {
     break;
   }
   case Builtin::BIsnprintf:
   case Builtin::BI__builtin_snprintf:
   case Builtin::BIvsnprintf:
   case Builtin::BI__builtin_vsnprintf: {
     DiagID = diag::warn_fortify_source_size_mismatch;
     SourceSize = ComputeExplicitObjectSizeArgument(1);
-    DestinationSize = ComputeSizeArgument(0);
+    const auto *FormatExpr = TheCall->getArg(2)->IgnoreParenImpCasts();
+    StringRef FormatStrRef;
+    size_t StrLen;
+    if (SourceSize &&
+        ProcessFormatStringLiteral(FormatExpr, FormatStrRef, StrLen, Context)) {
+      EstimateSizeFormatHandler H(FormatStrRef);
+      const char *FormatBytes = FormatStrRef.data();
+      if (!analyze_format_string::ParsePrintfString(
+              H, FormatBytes, FormatBytes + StrLen, getLangOpts(),
+              Context.getTargetInfo(), /*isFreeBSDKPrintf=*/false)) {
+        llvm::APSInt FormatSize =
+            llvm::APSInt::getUnsigned(H.getSizeLowerBound())
+                .extOrTrunc(SizeTypeWidth);
+        if (FormatSize > *SourceSize && *SourceSize != 0) {
+          DiagID = diag::warn_fortify_source_format_truncation;
+          DestinationSize = SourceSize;
+          SourceSize = FormatSize;
           break;
         }
       }
+    }
+    DestinationSize = ComputeSizeArgument(0);
+  }
+  }
 
   if (!SourceSize || !DestinationSize ||
       llvm::APSInt::compareValues(*SourceSize, *DestinationSize) <= 0)
     return;
 
   StringRef FunctionName = GetFunctionName();
 
   SmallString<16> DestinationStr;

clang/test/Analysis/taint-generic.c

 }
 
 void testTaintSystemCall2(void) {
   // Test that snpintf transfers taint.
   char buffern[156];
   char addr[128];
   scanf("%s", addr);
   __builtin_snprintf(buffern, 10, "/bin/mail %s < /tmp/email", addr);
+  // expected-warning@-1 {{'snprintf' will always be truncated; specified size is 10, but format string expands to at least 24}}
   system(buffern); // expected-warning {{Untrusted data is passed to a system call}}
 }
 
 void testTaintSystemCall3(void) {
   char buffern2[156];
   int numt;
   char addr[128];
   scanf("%s %d", addr, &numt);

clang/test/Sema/format-strings.c

 
 #endif // !defined(__ANDROID__) && !defined(__Fuchsia__)
 
 void check_invalid_specifier(FILE* fp, char *buf)
 {
   printf("%s%lv%d","unix",10,20); // expected-warning {{invalid conversion specifier 'v'}} expected-warning {{data argument not used by format string}}
   fprintf(fp,"%%%l"); // expected-warning {{incomplete format specifier}}
   sprintf(buf,"%%%%%ld%d%d", 1, 2, 3); // expected-warning{{format specifies type 'long' but the argument has type 'int'}}
-  snprintf(buf, 2, "%%%%%ld%;%d", 1, 2, 3); // expected-warning{{format specifies type 'long' but the argument has type 'int'}} expected-warning {{invalid conversion specifier ';'}} expected-warning {{data argument not used by format string}}
+  snprintf(buf, 2, "%%%%%ld%;%d", 1, 2, 3); // expected-warning{{format specifies type 'long' but the argument has type 'int'}} expected-warning {{invalid conversion specifier ';'}} expected-warning {{data argument not used by format string}} \
+                                            // expected-warning{{'snprintf' will always be truncated; specified size is 2, but format string expands to at least 7}}
 }
 
 void check_null_char_string(char* b)
 {
   printf("\0this is bogus%d",1); // expected-warning {{string contains '\0'}}
   snprintf(b,10,"%%%%%d\0%d",1,2); // expected-warning {{string contains '\0'}}
   printf("%\0d",1); // expected-warning {{string contains '\0'}}
 }

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

 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_BUILTINS
 // RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify
 // RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 %s -verify -DUSE_BUILTINS
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 -Wno-fortify-source %s -verify=nofortify
+// RUN: %clang_cc1 -xc++ -triple x86_64-apple-macosx10.14.0 -Wno-fortify-source %s -verify=nofortify
 
 typedef unsigned long size_t;
 
 #ifdef __cplusplus
 extern "C" {
 #endif
 
 extern int sprintf(char *str, const char *format, ...);

 }
 
 void call_memset(void) {
   char buf[10];
   __builtin_memset(buf, 0xff, 10);
   __builtin_memset(buf, 0xff, 11); // expected-warning {{'memset' will always overflow; destination buffer has size 10, but size argument is 11}}
 }
 
-void call_snprintf(void) {
+void call_snprintf(double d) {
   char buf[10];
   __builtin_snprintf(buf, 10, "merp");
   __builtin_snprintf(buf, 11, "merp"); // expected-warning {{'snprintf' size argument is too large; destination buffer has size 10, but size argument is 11}}
+  __builtin_snprintf(buf, 0, "merp");
+  __builtin_snprintf(buf, 3, "merp"); // expected-warning {{'snprintf' will always be truncated; specified size is 3, but format string expands to at least 5}}
+  __builtin_snprintf(buf, 4, "merp"); // expected-warning {{'snprintf' will always be truncated; specified size is 4, but format string expands to at least 5}}
+  __builtin_snprintf(buf, 5, "merp");
+  __builtin_snprintf(buf, 1, "%.1000g", d); // expected-warning {{'snprintf' will always be truncated; specified size is 1, but format string expands to at least 2}}
+  __builtin_snprintf(buf, 5, "%.1000g", d);
+  __builtin_snprintf(buf, 5, "%.1000G", d);
 }
 
 void call_vsnprintf(void) {
   char buf[10];
   __builtin_va_list list;
   __builtin_vsnprintf(buf, 10, "merp", list);
   __builtin_vsnprintf(buf, 11, "merp", list); // expected-warning {{'vsnprintf' size argument is too large; destination buffer has size 10, but size argument is 11}}
+  __builtin_vsnprintf(buf, 0, "merp", list);
+  __builtin_vsnprintf(buf, 3, "merp", list); // expected-warning {{'vsnprintf' will always be truncated; specified size is 3, but format string expands to at least 5}}
+  __builtin_vsnprintf(buf, 4, "merp", list); // expected-warning {{'vsnprintf' will always be truncated; specified size is 4, but format string expands to at least 5}}
+  __builtin_vsnprintf(buf, 5, "merp", list);
+  __builtin_vsnprintf(buf, 1, "%.1000g", list); // expected-warning {{'vsnprintf' will always be truncated; specified size is 1, but format string expands to at least 2}}
+  __builtin_vsnprintf(buf, 5, "%.1000g", list);
+  __builtin_vsnprintf(buf, 5, "%.1000G", list);
 }
 
 void call_sprintf_chk(char *buf) {
   __builtin___sprintf_chk(buf, 1, 6, "hell\n");
   __builtin___sprintf_chk(buf, 1, 5, "hell\n");     // expected-warning {{'sprintf' will always overflow; destination buffer has size 5, but format string expands to at least 6}}
-  __builtin___sprintf_chk(buf, 1, 6, "hell\0 boy"); // expected-warning {{format string contains '\0' within the string body}}
-  __builtin___sprintf_chk(buf, 1, 2, "hell\0 boy"); // expected-warning {{format string contains '\0' within the string body}}
-  // expected-warning@-1 {{'sprintf' will always overflow; destination buffer has size 2, but format string expands to at least 5}}
+  __builtin___sprintf_chk(buf, 1, 6, "hell\0 boy"); // expected-warning {{format string contains '\0' within the string body}} \
+                                                    // nofortify-warning {{format string contains '\0' within the string body}}
+  __builtin___sprintf_chk(buf, 1, 2, "hell\0 boy"); // expected-warning {{format string contains '\0' within the string body}} \
+                                                    // nofortify-warning {{format string contains '\0' within the string body}}
+  // expected-warning@-2 {{'sprintf' will always overflow; destination buffer has size 2, but format string expands to at least 5}}
   __builtin___sprintf_chk(buf, 1, 6, "hello");
   __builtin___sprintf_chk(buf, 1, 5, "hello"); // expected-warning {{'sprintf' will always overflow; destination buffer has size 5, but format string expands to at least 6}}
   __builtin___sprintf_chk(buf, 1, 2, "%c", '9');
   __builtin___sprintf_chk(buf, 1, 1, "%c", '9'); // expected-warning {{'sprintf' will always overflow; destination buffer has size 1, but format string expands to at least 2}}
   __builtin___sprintf_chk(buf, 1, 2, "%d", 9);
   __builtin___sprintf_chk(buf, 1, 1, "%d", 9); // expected-warning {{'sprintf' will always overflow; destination buffer has size 1, but format string expands to at least 2}}
   __builtin___sprintf_chk(buf, 1, 2, "%i", 9);
   __builtin___sprintf_chk(buf, 1, 1, "%i", 9); // expected-warning {{'sprintf' will always overflow; destination buffer has size 1, but format string expands to at least 2}}
void call_sprintf_chk(char *buf) {
   __builtin___sprintf_chk(buf, 1, 10, "%+f", 9.f);
   __builtin___sprintf_chk(buf, 1, 9, "%+f", 9.f); // expected-warning {{'sprintf' will always overflow; destination buffer has size 9, but format string expands to at least 10}}
   __builtin___sprintf_chk(buf, 1, 12, "%e", 9.f);
   __builtin___sprintf_chk(buf, 1, 11, "%e", 9.f); // expected-warning {{'sprintf' will always overflow; destination buffer has size 11, but format string expands to at least 12}}
 }
 
 void call_sprintf(void) {
   char buf[6];
-  sprintf(buf, "hell\0 boy"); // expected-warning {{format string contains '\0' within the string body}}
-  sprintf(buf, "hello b\0y"); // expected-warning {{format string contains '\0' within the string body}}
-  // expected-warning@-1 {{'sprintf' will always overflow; destination buffer has size 6, but format string expands to at least 8}}
+  sprintf(buf, "hell\0 boy"); // expected-warning {{format string contains '\0' within the string body}} \
+                              // nofortify-warning {{format string contains '\0' within the string body}}
+  sprintf(buf, "hello b\0y"); // expected-warning {{format string contains '\0' within the string body}} \
+                              // nofortify-warning {{format string contains '\0' within the string body}}
+  // expected-warning@-2 {{'sprintf' will always overflow; destination buffer has size 6, but format string expands to at least 8}}
   sprintf(buf, "hello");
   sprintf(buf, "hello!"); // expected-warning {{'sprintf' will always overflow; destination buffer has size 6, but format string expands to at least 7}}
   sprintf(buf, "1234%%");
   sprintf(buf, "12345%%"); // expected-warning {{'sprintf' will always overflow; destination buffer has size 6, but format string expands to at least 7}}
   sprintf(buf, "1234%c", '9');
   sprintf(buf, "12345%c", '9'); // expected-warning {{'sprintf' will always overflow; destination buffer has size 6, but format string expands to at least 7}}
   sprintf(buf, "1234%d", 9);
   sprintf(buf, "12345%d", 9); // expected-warning {{'sprintf' will always overflow; destination buffer has size 6, but format string expands to at least 7}}