Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang][Sema] Make format size estimator aware of %p's existence in format string #65969

Merged
merged 4 commits into from
Sep 25, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -160,9 +160,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]>;
Comment on lines +965 to +968
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these be added to the FortifySource group in addition to adding them to Format?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. It makes sense to add this to FortifySource for backward compatibility.


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
zygoloid marked this conversation as resolved.
Show resolved Hide resolved
: 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;
zygoloid marked this conversation as resolved.
Show resolved Hide resolved
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
Loading