Skip to content

Commit

Permalink
[clang][Sema] Make format size estimator aware of %p's existence in f…
Browse files Browse the repository at this point in the history
…ormat string (#65969)

This change introduces `-Wformat-overflow` and `-Wformat-truncation` warning flags that were formerly diagnosed from `-Wfortify-source` warning group.
This also introduces `-Wformat-overflow-non-kprintf` and `-Wformat-truncation-non-kprintf`, both of which will be used when the format string contains `%p` format string.

The rationale for this is that Linux kernel has its own extension for `%p` format specifier, and we need some way to suppress false positives in kernel codebase.
The approach of this patch aims NOT to affect non-kernel codebases.
Note that GCC stops format size estimation upon `%p` format specifier.

As requested in #64871
  • Loading branch information
hazohelet committed Sep 25, 2023
1 parent ff7e854 commit 56c3b8e
Show file tree
Hide file tree
Showing 7 changed files with 225 additions and 24 deletions.
12 changes: 11 additions & 1 deletion clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -185,9 +185,19 @@ Improvements to Clang's diagnostics
- 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
- Clang's ``-Wformat-truncation`` now diagnoses ``snprintf`` call that is known to
result in string truncation.
(`#64871: <https://github.com/llvm/llvm-project/issues/64871>`_).
Existing warnings that similarly warn about the overflow in ``sprintf``
now falls under its own warning group ```-Wformat-overflow`` so that it can
be disabled separately from ``Wfortify-source``.
These two new warning groups have subgroups ``-Wformat-truncation-non-kprintf``
and ``-Wformat-overflow-non-kprintf``, respectively. These subgroups are used when
the format string contains ``%p`` format specifier.
Because Linux kernel's codebase has format extensions for ``%p``, kernel developers
are encouraged to disable these two subgroups by setting ``-Wno-format-truncation-non-kprintf``
and ``-Wno-format-overflow-non-kprintf`` in order to avoid false positives on
the kernel codebase.
Also clang no longer emits false positive warnings about the output length of
``%g`` format specifier and about ``%o, %x, %X`` with ``#`` flag.
- Clang now emits ``-Wcast-qual`` for functional-style cast expressions.
Expand Down
10 changes: 8 additions & 2 deletions clang/include/clang/Basic/DiagnosticGroups.td
Original file line number Diff line number Diff line change
Expand Up @@ -961,10 +961,16 @@ def FormatNonStandard : DiagGroup<"format-non-iso">;
def FormatY2K : DiagGroup<"format-y2k">;
def FormatPedantic : DiagGroup<"format-pedantic">;
def FormatTypeConfusion : DiagGroup<"format-type-confusion">;

def FormatOverflowNonKprintf: DiagGroup<"format-overflow-non-kprintf">;
def FormatOverflow: DiagGroup<"format-overflow", [FormatOverflowNonKprintf]>;
def FormatTruncationNonKprintf: DiagGroup<"format-truncation-non-kprintf">;
def FormatTruncation: DiagGroup<"format-truncation", [FormatTruncationNonKprintf]>;

def Format : DiagGroup<"format",
[FormatExtraArgs, FormatZeroLength, NonNull,
FormatSecurity, FormatY2K, FormatInvalidSpecifier,
FormatInsufficientArgs]>,
FormatInsufficientArgs, FormatOverflow, FormatTruncation]>,
DiagCategory<"Format String Issue">;
def FormatNonLiteral : DiagGroup<"format-nonliteral">;
def Format2 : DiagGroup<"format=2",
Expand Down Expand Up @@ -1400,7 +1406,7 @@ def CrossTU : DiagGroup<"ctu">;

def CTADMaybeUnsupported : DiagGroup<"ctad-maybe-unsupported">;

def FortifySource : DiagGroup<"fortify-source">;
def FortifySource : DiagGroup<"fortify-source", [FormatOverflow, FormatTruncation]>;

def MaxTokens : DiagGroup<"max-tokens"> {
code Documentation = [{
Expand Down
26 changes: 20 additions & 6 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -854,15 +854,29 @@ 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<
def subst_format_overflow : TextSubstitution<
"'%0' will always overflow; destination buffer has size %1,"
" but format string expands to at least %2">,
InGroup<FortifySource>;
" but format string expands to at least %2">;

def warn_format_overflow : Warning<
"%sub{subst_format_overflow}0,1,2">,
InGroup<FormatOverflow>;

def warn_format_overflow_non_kprintf : Warning<
"%sub{subst_format_overflow}0,1,2">,
InGroup<FormatOverflowNonKprintf>;

def warn_fortify_source_format_truncation: Warning<
def subst_format_truncation: TextSubstitution<
"'%0' will always be truncated; specified size is %1,"
" but format string expands to at least %2">,
InGroup<FortifySource>;
" but format string expands to at least %2">;

def warn_format_truncation: Warning<
"%sub{subst_format_truncation}0,1,2">,
InGroup<FormatTruncation>;

def warn_format_truncation_non_kprintf: Warning<
"%sub{subst_format_truncation}0,1,2">,
InGroup<FormatTruncationNonKprintf>;

def warn_fortify_scanf_overflow : Warning<
"'%0' may overflow; destination buffer in argument %1 has size "
Expand Down
27 changes: 22 additions & 5 deletions clang/lib/Sema/SemaChecking.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -854,6 +854,9 @@ class ScanfDiagnosticFormatHandler
class EstimateSizeFormatHandler
: public analyze_format_string::FormatStringHandler {
size_t Size;
/// Whether the format string contains Linux kernel's format specifier
/// extension.
bool IsKernelCompatible = true;

public:
EstimateSizeFormatHandler(StringRef Format)
Expand Down Expand Up @@ -933,6 +936,10 @@ class EstimateSizeFormatHandler

// Just a pointer in the form '0xddd'.
case analyze_format_string::ConversionSpecifier::pArg:
// Linux kernel has its own extesion for `%p` specifier.
// Kernel Document:
// https://docs.kernel.org/core-api/printk-formats.html#pointer-types
IsKernelCompatible = false;
Size += std::max(FieldWidth, 2 /* leading 0x */ + Precision);
break;

Expand Down Expand Up @@ -990,6 +997,7 @@ class EstimateSizeFormatHandler
}

size_t getSizeLowerBound() const { return Size; }
bool isKernelCompatible() const { return IsKernelCompatible; }

private:
static size_t computeFieldWidth(const analyze_printf::PrintfSpecifier &FS) {
Expand Down Expand Up @@ -1259,7 +1267,9 @@ void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD,
if (!analyze_format_string::ParsePrintfString(
H, FormatBytes, FormatBytes + StrLen, getLangOpts(),
Context.getTargetInfo(), false)) {
DiagID = diag::warn_fortify_source_format_overflow;
DiagID = H.isKernelCompatible()
? diag::warn_format_overflow
: diag::warn_format_overflow_non_kprintf;
SourceSize = llvm::APSInt::getUnsigned(H.getSizeLowerBound())
.extOrTrunc(SizeTypeWidth);
if (BuiltinID == Builtin::BI__builtin___sprintf_chk) {
Expand Down Expand Up @@ -1350,10 +1360,17 @@ void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD,
llvm::APSInt::getUnsigned(H.getSizeLowerBound())
.extOrTrunc(SizeTypeWidth);
if (FormatSize > *SourceSize && *SourceSize != 0) {
DiagID = diag::warn_fortify_source_format_truncation;
DestinationSize = SourceSize;
SourceSize = FormatSize;
break;
unsigned TruncationDiagID =
H.isKernelCompatible() ? diag::warn_format_truncation
: diag::warn_format_truncation_non_kprintf;
SmallString<16> SpecifiedSizeStr;
SmallString<16> FormatSizeStr;
SourceSize->toString(SpecifiedSizeStr, /*Radix=*/10);
FormatSize.toString(FormatSizeStr, /*Radix=*/10);
DiagRuntimeBehavior(TheCall->getBeginLoc(), TheCall,
PDiag(TruncationDiagID)
<< GetFunctionName() << SpecifiedSizeStr
<< FormatSizeStr);
}
}
}
Expand Down
4 changes: 4 additions & 0 deletions clang/test/Misc/warning-wall.c
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ CHECK-NEXT: -Wformat-security
CHECK-NEXT: -Wformat-y2k
CHECK-NEXT: -Wformat-invalid-specifier
CHECK-NEXT: -Wformat-insufficient-args
CHECK-NEXT: -Wformat-overflow
CHECK-NEXT: -Wformat-overflow-non-kprintf
CHECK-NEXT: -Wformat-truncation
CHECK-NEXT: -Wformat-truncation-non-kprintf
CHECK-NEXT: -Wfor-loop-analysis
CHECK-NEXT: -Wframe-address
CHECK-NEXT: -Wimplicit
Expand Down

0 comments on commit 56c3b8e

Please sign in to comment.