[clang][Sema] Fix format size estimator's handling of %o, %x, %X with alternative form

[hazohelet]

Summary

The wrong handling of %x specifier with alternative form causes a false positive in linux kernel (https://github.com/ClangBuiltLinux/linux/issues/1923#issuecomment-1696075886)

The kernel code: https://github.com/torvalds/linux/blob/651a00bc56403161351090a9d7ddbd7095975324/drivers/media/pci/cx18/cx18-mailbox.c#L99

This patch fixes this handling, and also adds some standard wordings as comments to clarify the reason.

[serge-sans-paille]

clang/lib/Sema/SemaChecking.cpp

966

Then couldn't we increase the Size if the FieldWidth is unspecified?

[hazohelet]

clang/lib/Sema/SemaChecking.cpp

966

We cannot, unless the formatted integer is known to be nonzero.
("%#x", 0) is "0" and not "0x0"

[nickdesaulniers]

Mind adding tests for %#X and %#o as well, since you're changing the behavior of those, too?

I think it would also be good to have explicit tests for %o and %b where the value being printed was and was not 0. IIUC, we cannot diagnose those (unless a literal value is being printed)?

(Thanks for finding that one of the kernel cases was a false positive!)

[hazohelet]

Added %X and %o tests, which are also affected

[hazohelet]

In D159138#4628858, @nickdesaulniers wrote:

Mind adding tests for %#X and %#o as well, since you're changing the behavior of those, too?

I think it would also be good to have explicit tests for %o and %b where the value being printed was and was not 0. IIUC, we cannot diagnose those (unless a literal value is being printed)?

(Thanks for finding that one of the kernel cases was a false positive!)

The current implementation only sees the format string and calculates the minimum possible length without taking into accout the actual value passed to it. So currently every case assumes that the value can be 0, even when a literal is passed.
Even if the printed expression is not literal, we can use something like Expr::EvaluateAsRValue to learn about the printed value.

As for the %b, it's C23 feature and Wfortify-source doesn't seem to support it yet (%b is considered a valid specifier, but does not increase Size here). I think %b tests should be added when we support it.

[nickdesaulniers]

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

100–102

Note that GCC -Wformat-truncation can warn on some of these.

https://godbolt.org/z/jE3axWe1W

Looks like the diagnostic keeps an up and lower bounds on the estimated format string expansion.

Trunk for Clang also warns for these, so is this change a regression? Or are both GCC and Clang (trunk) incorrect?

121–123

Clang @ trunk and GCC both warn for these 3. Regression?
https://godbolt.org/z/j551hTE5E

[hazohelet]

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

100–102

Clang trunk is saying something factually incorrect because it says the output will always be truncated, when in fact __builtin_snprintf(buf, 2, "%#x", n); doesn't trigger truncation if n is zero.

GCC is correct but is more conservative than clang's ALWAYS be truncated diagnostics.
GCC's warning message is ... directive output MAY BE truncated .
GCC doesn't warn on it when n is known to be zero. (https://godbolt.org/z/E51a3Pfhr)

GCC's behavior makes sense here because the truncation does happen whenever n is not zero. If the user knows n is zero then they have no reason to use %#x specifier.
So, I think it makes sense to assume n is not zero and emit diagnostics, but it definitely needs diagnostics rewording like is likely to be truncated.

[hazohelet]

Ping

[serge-sans-paille]

clang/docs/ReleaseNotes.rst

125

??

136

looks like a leak from another patch ^^!

[nickdesaulniers]

Thanks for the patch!

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

100–102

I see; thanks for the explanation. Sorry for the code review delay; I missed this comment in my inbox.

[hazohelet]

Thanks for the review!

clang/docs/ReleaseNotes.rst

125

I'm not seeing any diff around here, so perhaps you were seeing some Phabricator bug?

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

100–102

As a future plan, I think it's generally good to follow GCC's behavior regarding this format warning considering GCC's high smartness on it.
I want to emit some warning here like GCC does because users are highly likely to expect non-zero value to be printed, but I think it's reasonable to defer it for now because it's not ideal to add a new warning flag when we are considering changing this warning's flag from Wfortify-source to Wformat-truncation

clang/docs/ReleaseNotes.rst

 
 Deprecated Compiler Flags
 -------------------------
 
 Modified Compiler Flags
 -----------------------
 
 * ``-Woverriding-t-option`` is renamed to ``-Woverriding-option``.
 * ``-Winterrupt-service-routine`` is renamed to ``-Wexcessive-regsave`` as a generalization
 
 Removed Compiler Flags
 -------------------------
 
 Attribute Changes in Clang
 --------------------------
 - On X86, a warning is now emitted if a function with ``__attribute__((no_caller_saved_registers))``
   calls a function without ``__attribute__((no_caller_saved_registers))``, and is not compiled with
   ``-mgeneral-regs-only``
 - On X86, a function with ``__attribute__((interrupt))`` can now call a function without
   ``__attribute__((no_caller_saved_registers))`` provided that it is compiled with ``-mgeneral-regs-only``
 
 - When a non-variadic function is decorated with the ``format`` attribute,
   Clang now checks that the format string would match the function's parameters'
   types after default argument promotion. As a result, it's no longer an
   automatic diagnostic to use parameters of types that the format style
   supports but that are never the result of default argument promotion, such as
   ``float``. (`#59824: <https://github.com/llvm/llvm-project/issues/59824>`_)
 
- Clang's ``-Wtautological-negation-compare`` flag now diagnoses logical
   (`#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.
+  ``%g`` format specifier and about ``%o, %x, %X`` with ``#`` flag.
 - Clang now warns on unused variables declared and initialized in condition
   expressions.
   (`#61681: <https://github.com/llvm/llvm-project/issues/61681>`_)
 
 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

clang/lib/Sema/SemaChecking.cpp

bool HandlePrintfSpecifier(const analyze_printf::PrintfSpecifier &FS,
     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.
     // %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.
+    // FIXME: If it is alternative form:
+    // For g and G conversions, trailing zeros are not removed from the result.
     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:
bool HandlePrintfSpecifier(const analyze_printf::PrintfSpecifier &FS,
     default:
       break;
     }
 
     Size += FS.hasPlusPrefix() || FS.hasSpacePrefix();
 
     if (FS.hasAlternativeForm()) {
       switch (FS.getConversionSpecifier().getKind()) {
-      default:
-        break;
-      // Force a leading '0'.
+      // For o conversion, it increases the precision, if and only if necessary,
+      // to force the first digit of the result to be a zero
+      // (if the value and precision are both 0, a single 0 is printed)
       case analyze_format_string::ConversionSpecifier::oArg:
-        Size += 1;
-        break;
-      // Force a leading '0x'.
+      // For b conversion, a nonzero result has 0b prefixed to it.
+      case analyze_format_string::ConversionSpecifier::bArg:
+      // For x (or X) conversion, a nonzero result has 0x (or 0X) prefixed to
+      // it.
       case analyze_format_string::ConversionSpecifier::xArg:
       case analyze_format_string::ConversionSpecifier::XArg:
-        Size += 2;
-        break;
-      // Force a period '.' before decimal, even if precision is 0.
+        // Note: even when the prefix is added, if
+        // (prefix_width <= FieldWidth - formatted_length) holds,
+        // the prefix does not increase the format
+        // size. e.g.(("%#3x", 0xf) is "0xf")
+
+        // If the result is zero, o, b, x, X adds nothing.
+        break;
+      // For a, A, e, E, f, F, g, and G conversions,
+      // the result of converting a floating-point number always contains a
+      // decimal-point
       case analyze_format_string::ConversionSpecifier::aArg:
       case analyze_format_string::ConversionSpecifier::AArg:
       case analyze_format_string::ConversionSpecifier::eArg:
       case analyze_format_string::ConversionSpecifier::EArg:
       case analyze_format_string::ConversionSpecifier::fArg:
       case analyze_format_string::ConversionSpecifier::FArg:
       case analyze_format_string::ConversionSpecifier::gArg:
       case analyze_format_string::ConversionSpecifier::GArg:
         Size += (Precision ? 0 : 1);
         break;
+      // For other conversions, the behavior is undefined.
+      default:
+        break;
       }
     }
     assert(SpecifierLen <= Size && "no underflow");
     Size -= SpecifierLen;
     return true;
   }
 
   size_t getSizeLowerBound() const { return Size; }

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

 }
 
 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(double d) {
+void call_snprintf(double d, int n) {
   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);
+  __builtin_snprintf(buf, 10, " %#08x", n);
+  __builtin_snprintf(buf, 2, "%#x", n);
+  __builtin_snprintf(buf, 2, "%#X", n);
+  __builtin_snprintf(buf, 2, "%#o", n);
+  __builtin_snprintf(buf, 1, "%#x", n); // expected-warning {{'snprintf' will always be truncated; specified size is 1, but format string expands to at least 2}}
+  __builtin_snprintf(buf, 1, "%#X", n); // expected-warning {{'snprintf' will always be truncated; specified size is 1, but format string expands to at least 2}}
+  __builtin_snprintf(buf, 1, "%#o", n); // expected-warning {{'snprintf' will always be truncated; specified size is 1, but format string expands to at least 2}}
 }
 
 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);
+  __builtin_vsnprintf(buf, 10, " %#08x", list);
+  __builtin_vsnprintf(buf, 2, "%#x", list);
+  __builtin_vsnprintf(buf, 2, "%#X", list);
+  __builtin_vsnprintf(buf, 2, "%#o", list);
+  __builtin_vsnprintf(buf, 1, "%#x", list); // expected-warning {{'vsnprintf' will always be truncated; specified size is 1, but format string expands to at least 2}}
+  __builtin_vsnprintf(buf, 1, "%#X", list); // expected-warning {{'vsnprintf' will always be truncated; specified size is 1, but format string expands to at least 2}}
+  __builtin_vsnprintf(buf, 1, "%#o", list); // expected-warning {{'vsnprintf' will always be truncated; specified size is 1, but format string expands to at least 2}}
 }
 
 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}} \
                                                     // 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}} \
void call_sprintf_chk(char *buf) {
   __builtin___sprintf_chk(buf, 1, 1, "%hd", (short)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, "%ld", 9l);
   __builtin___sprintf_chk(buf, 1, 1, "%ld", 9l); // 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, "%lld", 9ll);
   __builtin___sprintf_chk(buf, 1, 1, "%lld", 9ll); // 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, "%%");
   __builtin___sprintf_chk(buf, 1, 1, "%%"); // expected-warning {{'sprintf' will always overflow; destination buffer has size 1, but format string expands to at least 2}}
   __builtin___sprintf_chk(buf, 1, 4, "%#x", 9);
-  __builtin___sprintf_chk(buf, 1, 3, "%#x", 9); // expected-warning {{'sprintf' will always overflow; destination buffer has size 3, but format string expands to at least 4}}
+  __builtin___sprintf_chk(buf, 1, 3, "%#x", 9);
   __builtin___sprintf_chk(buf, 1, 4, "%p", (void *)9);
   __builtin___sprintf_chk(buf, 1, 3, "%p", (void *)9); // expected-warning {{'sprintf' will always overflow; destination buffer has size 3, but format string expands to at least 4}}
   __builtin___sprintf_chk(buf, 1, 3, "%+d", 9);
   __builtin___sprintf_chk(buf, 1, 2, "%+d", 9); // expected-warning {{'sprintf' will always overflow; destination buffer has size 2, but format string expands to at least 3}}
   __builtin___sprintf_chk(buf, 1, 3, "% i", 9);
   __builtin___sprintf_chk(buf, 1, 2, "% i", 9); // expected-warning {{'sprintf' will always overflow; destination buffer has size 2, but format string expands to at least 3}}
   __builtin___sprintf_chk(buf, 1, 6, "%5d", 9);
   __builtin___sprintf_chk(buf, 1, 5, "%5d", 9); // expected-warning {{'sprintf' will always overflow; destination buffer has size 5, but format string expands to at least 6}}
void call_sprintf(void) {
   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}}
   sprintf(buf, "1234%lld", 9ll);
   sprintf(buf, "12345%lld", 9ll); // expected-warning {{'sprintf' will always overflow; destination buffer has size 6, but format string expands to at least 7}}
   sprintf(buf, "12%#x", 9);
-  sprintf(buf, "123%#x", 9); // expected-warning {{'sprintf' will always overflow; destination buffer has size 6, but format string expands to at least 7}}
+  sprintf(buf, "123%#x", 9);
   sprintf(buf, "12%p", (void *)9);
   sprintf(buf, "123%p", (void *)9); // expected-warning {{'sprintf' will always overflow; destination buffer has size 6, but format string expands to at least 7}}
   sprintf(buf, "123%+d", 9);
   sprintf(buf, "1234%+d", 9); // expected-warning {{'sprintf' will always overflow; destination buffer has size 6, but format string expands to at least 7}}
   sprintf(buf, "123% i", 9);
   sprintf(buf, "1234% i", 9); // expected-warning {{'sprintf' will always overflow; destination buffer has size 6, but format string expands to at least 7}}
   sprintf(buf, "%5d", 9);
   sprintf(buf, "1%5d", 9); // expected-warning {{'sprintf' will always overflow; destination buffer has size 6, but format string expands to at least 7}}