Skip to content

[-Wunsafe-buffer-usage] Support safe patterns of "%.*s" in printf functions #145862

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[-Wunsafe-buffer-usage] Support safe patterns of "%.*s" in printf functions #145862

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5c6a2f2
[NFC][-Wunsafe-buffer-usage] Refactor safe pattern check of pointer a…
ziqingluo-90 Jun 24, 2025
7167dfd
[-Wunsafe-buffer-usage] Support safe patterns of "%.*s" in printf fun…
ziqingluo-90 Jun 24, 2025
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
292 changes: 176 additions & 116 deletions clang/lib/Analysis/UnsafeBufferUsage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "clang/Basic/SourceLocation.h"
#include "clang/Lex/Lexer.h"
#include "clang/Lex/Preprocessor.h"
#include "llvm/ADT/APInt.h"
#include "llvm/ADT/APSInt.h"
#include "llvm/ADT/STLFunctionalExtras.h"
#include "llvm/ADT/SmallSet.h"
Expand Down Expand Up @@ -453,22 +454,108 @@ static bool areEqualIntegers(const Expr *E1, const Expr *E2, ASTContext &Ctx) {
}
}

// Providing that `Ptr` is a pointer and `Size` is an unsigned-integral
// expression, returns true iff they follow one of the following safe
// patterns:
// 1. Ptr is `DRE.data()` and Size is `DRE.size()`, where DRE is a hardened
// container or view;
//
// 2. Ptr is `a` and Size is `n`, where `a` is of an array-of-T with constant
// size `n`;
//
// 3. Ptr is `&var` and Size is `1`; or
// Ptr is `std::addressof(...)` and Size is `1`;
//
// 4. Size is `0`;
static bool isPtrBufferSafe(const Expr *Ptr, const Expr *Size,
ASTContext &Ctx) {
// Pattern 1:
if (auto *MCEPtr = dyn_cast<CXXMemberCallExpr>(Ptr->IgnoreParenImpCasts()))
if (auto *MCESize =
dyn_cast<CXXMemberCallExpr>(Size->IgnoreParenImpCasts())) {
auto *DREOfPtr = dyn_cast<DeclRefExpr>(
MCEPtr->getImplicitObjectArgument()->IgnoreParenImpCasts());
auto *DREOfSize = dyn_cast<DeclRefExpr>(
MCESize->getImplicitObjectArgument()->IgnoreParenImpCasts());

if (!DREOfPtr || !DREOfSize)
return false; // not in safe pattern
if (DREOfPtr->getDecl() != DREOfSize->getDecl())
return false;
if (MCEPtr->getMethodDecl()->getName() != "data")
return false;
// `MCEPtr->getRecordDecl()` must be non-null as `DREOfPtr` is non-null:
if (!MCEPtr->getRecordDecl()->isInStdNamespace())
return false;

auto *ObjII = MCEPtr->getRecordDecl()->getIdentifier();

if (!ObjII)
return false;

bool AcceptSizeBytes = Ptr->getType()->getPointeeType()->isCharType();

if (!((AcceptSizeBytes &&
MCESize->getMethodDecl()->getName() == "size_bytes") ||
// Note here the pointer must be a pointer-to-char type unless there
// is explicit casting. If there is explicit casting, this branch
// is unreachable. Thus, at this branch "size" and "size_bytes" are
// equivalent as the pointer is a char pointer:
MCESize->getMethodDecl()->getName() == "size"))
return false;

return llvm::is_contained({SIZED_CONTAINER_OR_VIEW_LIST},
ObjII->getName());
}

Expr::EvalResult ER;

// Pattern 2-4:
if (Size->EvaluateAsInt(ER, Ctx)) {
// Pattern 2:
if (auto *DRE = dyn_cast<DeclRefExpr>(Ptr->IgnoreParenImpCasts())) {
if (auto *CAT = Ctx.getAsConstantArrayType(DRE->getType())) {
llvm::APSInt SizeInt = ER.Val.getInt();

return llvm::APSInt::compareValues(
SizeInt, llvm::APSInt(CAT->getSize(), true)) == 0;
}
return false;
}

// Pattern 3:
if (ER.Val.getInt().isOne()) {
if (auto *UO = dyn_cast<UnaryOperator>(Ptr->IgnoreParenImpCasts()))
return UO && UO->getOpcode() == UnaryOperator::Opcode::UO_AddrOf;
if (auto *CE = dyn_cast<CallExpr>(Ptr->IgnoreParenImpCasts())) {
auto *FnDecl = CE->getDirectCallee();

return FnDecl && FnDecl->getNameAsString() == "addressof" &&
FnDecl->isInStdNamespace();
}
return false;
}
// Pattern 4:
if (ER.Val.getInt().isZero())
return true;
}
return false;
}

// Given a two-param std::span construct call, matches iff the call has the
// following forms:
// 1. `std::span<T>{new T[n], n}`, where `n` is a literal or a DRE
// 2. `std::span<T>{new T, 1}`
// 3. `std::span<T>{&var, 1}` or `std::span<T>{std::addressof(...), 1}`
// 4. `std::span<T>{a, n}`, where `a` is of an array-of-T with constant size
// `n`
// 5. `std::span<T>{any, 0}`
// 6. `std::span<T>{ (char *)f(args), args[N] * arg*[M]}`, where
// 3. `std::span<T>{ (char *)f(args), args[N] * arg*[M]}`, where
// `f` is a function with attribute `alloc_size(N, M)`;
// `args` represents the list of arguments;
// `N, M` are parameter indexes to the allocating element number and size.
// Sometimes, there is only one parameter index representing the total
// size.
// 7. `std::span<T>{x.begin(), x.end()}` where `x` is an object in the
// 4. `std::span<T>{x.begin(), x.end()}` where `x` is an object in the
// SIZED_CONTAINER_OR_VIEW_LIST.
// 5. `isPtrBufferSafe` returns true of the two arguments of the span
// constructor
static bool isSafeSpanTwoParamConstruct(const CXXConstructExpr &Node,
ASTContext &Ctx) {
assert(Node.getNumArgs() == 2 &&
Expand All @@ -495,7 +582,7 @@ static bool isSafeSpanTwoParamConstruct(const CXXConstructExpr &Node,
// Check form 5:
return true;

// Check forms 1-3:
// Check forms 1-2:
switch (Arg0->getStmtClass()) {
case Stmt::CXXNewExprClass:
if (auto Size = cast<CXXNewExpr>(Arg0)->getArraySize()) {
Expand All @@ -509,35 +596,11 @@ static bool isSafeSpanTwoParamConstruct(const CXXConstructExpr &Node,
return Arg1CV && Arg1CV->isOne();
}
break;
case Stmt::UnaryOperatorClass:
if (cast<UnaryOperator>(Arg0)->getOpcode() ==
UnaryOperator::Opcode::UO_AddrOf)
// Check form 3:
return Arg1CV && Arg1CV->isOne();
break;
case Stmt::CallExprClass:
// Check form 3:
if (const auto *CE = dyn_cast<CallExpr>(Arg0)) {
const auto FnDecl = CE->getDirectCallee();
if (FnDecl && FnDecl->getNameAsString() == "addressof" &&
FnDecl->isInStdNamespace()) {
return Arg1CV && Arg1CV->isOne();
}
}
break;
default:
break;
}

QualType Arg0Ty = Arg0->IgnoreImplicit()->getType();

if (auto *ConstArrTy = Ctx.getAsConstantArrayType(Arg0Ty)) {
const llvm::APSInt ConstArrSize = llvm::APSInt(ConstArrTy->getSize());

// Check form 4:
return Arg1CV && llvm::APSInt::compareValues(ConstArrSize, *Arg1CV) == 0;
}
// Check form 6:
// Check form 3:
if (auto CCast = dyn_cast<CStyleCastExpr>(Arg0)) {
if (!CCast->getType()->isPointerType())
return false;
Expand Down Expand Up @@ -566,7 +629,7 @@ static bool isSafeSpanTwoParamConstruct(const CXXConstructExpr &Node,
}
}
}
// Check form 7:
// Check form 4:
auto IsMethodCallToSizedObject = [](const Stmt *Node, StringRef MethodName) {
if (const auto *MC = dyn_cast<CXXMemberCallExpr>(Node)) {
const auto *MD = MC->getMethodDecl();
Expand All @@ -592,7 +655,9 @@ static bool isSafeSpanTwoParamConstruct(const CXXConstructExpr &Node,
cast<CXXMemberCallExpr>(Arg1)
->getImplicitObjectArgument()
->IgnoreParenImpCasts());
return false;

// Check 5:
return isPtrBufferSafe(Arg0, Arg1, Ctx);
}

static bool isSafeArraySubscript(const ArraySubscriptExpr &Node,
Expand Down Expand Up @@ -743,28 +808,81 @@ static bool hasUnsafeFormatOrSArg(const CallExpr *Call, const Expr *&UnsafeArg,
const CallExpr *Call;
unsigned FmtArgIdx;
const Expr *&UnsafeArg;
ASTContext &Ctx;

// Returns an `Expr` representing the precision if specified, null
// otherwise:
const Expr *
getPrecisionAsExpr(const analyze_printf::OptionalAmount &Precision,
const CallExpr *Call) {
unsigned PArgIdx = -1;

if (Precision.hasDataArgument())
PArgIdx = Precision.getPositionalArgIndex() + FmtArgIdx;
if (0 < PArgIdx && PArgIdx < Call->getNumArgs()) {
const Expr *PArg = Call->getArg(PArgIdx);

// Strip the cast if `PArg` is a cast-to-int expression:
if (auto *CE = dyn_cast<CastExpr>(PArg);
CE && CE->getType()->isSignedIntegerType())
PArg = CE->getSubExpr();
return PArg;
}
if (Precision.getHowSpecified() ==
analyze_printf::OptionalAmount::HowSpecified::Constant) {
auto SizeTy = Ctx.getSizeType();
llvm::APSInt PArgVal = llvm::APSInt(
llvm::APInt(Ctx.getTypeSize(SizeTy), Precision.getConstantAmount()),
true);

return IntegerLiteral::Create(Ctx, PArgVal, Ctx.getSizeType(), {});
}
return nullptr;
}

public:
StringFormatStringHandler(const CallExpr *Call, unsigned FmtArgIdx,
const Expr *&UnsafeArg)
: Call(Call), FmtArgIdx(FmtArgIdx), UnsafeArg(UnsafeArg) {}
const Expr *&UnsafeArg, ASTContext &Ctx)
: Call(Call), FmtArgIdx(FmtArgIdx), UnsafeArg(UnsafeArg), Ctx(Ctx) {}

bool HandlePrintfSpecifier(const analyze_printf::PrintfSpecifier &FS,
const char *startSpecifier,
unsigned specifierLen,
const TargetInfo &Target) override {
if (FS.getConversionSpecifier().getKind() ==
analyze_printf::PrintfConversionSpecifier::sArg) {
unsigned ArgIdx = FS.getPositionalArgIndex() + FmtArgIdx;

if (0 < ArgIdx && ArgIdx < Call->getNumArgs())
if (!isNullTermPointer(Call->getArg(ArgIdx))) {
UnsafeArg = Call->getArg(ArgIdx); // output
// returning false stops parsing immediately
return false;
}
}
return true; // continue parsing
if (FS.getConversionSpecifier().getKind() !=
analyze_printf::PrintfConversionSpecifier::sArg)
return true; // continue parsing

unsigned ArgIdx = FS.getPositionalArgIndex() + FmtArgIdx;

if (!(0 < ArgIdx && ArgIdx < Call->getNumArgs()))
// If the `ArgIdx` is invalid, give up.
return true; // continue parsing

const Expr *Arg = Call->getArg(ArgIdx);

if (isNullTermPointer(Arg))
// If Arg is a null-terminated pointer, it is safe anyway.
return true; // continue parsing

// Otherwise, check if the specifier has a precision and if the character
// pointer is safely bound by the precision:
auto LengthModifier = FS.getLengthModifier();
QualType ArgType = Arg->getType();
bool IsArgTypeValid = // Is ArgType a character pointer type?
ArgType->isPointerType() &&
(LengthModifier.getKind() == LengthModifier.AsWideChar
? ArgType->getPointeeType()->isWideCharType()
: ArgType->getPointeeType()->isCharType());

ArgType.dump();
if (auto *Precision = getPrecisionAsExpr(FS.getPrecision(), Call);
Precision && IsArgTypeValid)
if (isPtrBufferSafe(Arg, Precision, Ctx))
return true;
// Handle unsafe case:
UnsafeArg = Call->getArg(ArgIdx); // output
return false; // returning false stops parsing immediately
}
};

Expand All @@ -780,7 +898,7 @@ static bool hasUnsafeFormatOrSArg(const CallExpr *Call, const Expr *&UnsafeArg,
else
goto CHECK_UNSAFE_PTR;

StringFormatStringHandler Handler(Call, FmtArgIdx, UnsafeArg);
StringFormatStringHandler Handler(Call, FmtArgIdx, UnsafeArg, Ctx);

return analyze_format_string::ParsePrintfString(
Handler, FmtStr.begin(), FmtStr.end(), Ctx.getLangOpts(),
Expand Down Expand Up @@ -1052,24 +1170,11 @@ static bool hasUnsafePrintfStringArg(const CallExpr &Node, ASTContext &Ctx,
return false;
}

// This matcher requires that it is known that the callee `isNormalPrintf`.
// Then it matches if the first two arguments of the call is a pointer and an
// integer and they are not in a safe pattern.
//
// For the first two arguments: `ptr` and `size`, they are safe if in the
// following patterns:
//
// Pattern 1:
// ptr := DRE.data();
// size:= DRE.size()/DRE.size_bytes()
// And DRE is a hardened container or view.
//
// Pattern 2:
// ptr := Constant-Array-DRE;
// size:= any expression that has compile-time constant value equivalent to
// sizeof (Constant-Array-DRE)
static bool hasUnsafeSnprintfBuffer(const CallExpr &Node,
const ASTContext &Ctx) {
// This function requires that it is known that the callee `isNormalPrintf`.
// It returns true iff the first two arguments of the call is a pointer
// `Ptr` and an unsigned integer `Size` and they are NOT safe, i.e.,
// `!isPtrBufferSafe(Ptr, Size)`.
static bool hasUnsafeSnprintfBuffer(const CallExpr &Node, ASTContext &Ctx) {
const FunctionDecl *FD = Node.getDirectCallee();

assert(FD && "It should have been checked that FD is non-null.");
Expand All @@ -1085,57 +1190,12 @@ static bool hasUnsafeSnprintfBuffer(const CallExpr &Node,
QualType FirstPteTy = FirstParmTy->castAs<PointerType>()->getPointeeType();
const Expr *Buf = Node.getArg(0), *Size = Node.getArg(1);

if (FirstPteTy.isConstQualified() || !Buf->getType()->isPointerType() ||
!Size->getType()->isIntegerType())
if (FirstPteTy.isConstQualified() || !FirstPteTy->isAnyCharacterType() ||
!Buf->getType()->isPointerType() ||
!Size->getType()->isUnsignedIntegerType())
return false; // not an snprintf call

// Pattern 1:
static StringRef SizedObjs[] = {SIZED_CONTAINER_OR_VIEW_LIST};
Buf = Buf->IgnoreParenImpCasts();
Size = Size->IgnoreParenImpCasts();
if (auto *MCEPtr = dyn_cast<CXXMemberCallExpr>(Buf))
if (auto *MCESize = dyn_cast<CXXMemberCallExpr>(Size)) {
auto *DREOfPtr = dyn_cast<DeclRefExpr>(
MCEPtr->getImplicitObjectArgument()->IgnoreParenImpCasts());
auto *DREOfSize = dyn_cast<DeclRefExpr>(
MCESize->getImplicitObjectArgument()->IgnoreParenImpCasts());

if (!DREOfPtr || !DREOfSize)
return true; // not in safe pattern
if (DREOfPtr->getDecl() != DREOfSize->getDecl())
return true; // not in safe pattern
if (MCEPtr->getMethodDecl()->getName() != "data")
return true; // not in safe pattern

if (MCESize->getMethodDecl()->getName() == "size_bytes" ||
// Note here the pointer must be a pointer-to-char type unless there
// is explicit casting. If there is explicit casting, this branch
// is unreachable. Thus, at this branch "size" and "size_bytes" are
// equivalent as the pointer is a char pointer:
MCESize->getMethodDecl()->getName() == "size")
for (StringRef SizedObj : SizedObjs)
if (MCEPtr->getRecordDecl()->isInStdNamespace() &&
MCEPtr->getRecordDecl()->getCanonicalDecl()->getName() ==
SizedObj)
return false; // It is in fact safe
}

// Pattern 2:
if (auto *DRE = dyn_cast<DeclRefExpr>(Buf->IgnoreParenImpCasts())) {
if (auto *CAT = Ctx.getAsConstantArrayType(DRE->getType())) {
Expr::EvalResult ER;
// The array element type must be compatible with `char` otherwise an
// explicit cast will be needed, which will make this check unreachable.
// Therefore, the array extent is same as its' bytewise size.
if (Size->EvaluateAsInt(ER, Ctx)) {
llvm::APSInt EVal = ER.Val.getInt(); // Size must have integer type

return llvm::APSInt::compareValues(
EVal, llvm::APSInt(CAT->getSize(), true)) != 0;
}
}
}
return true; // ptr and size are not in safe pattern
return !isPtrBufferSafe(Buf, Size, Ctx);
}
} // namespace libc_func_matchers

Expand Down
Loading