Thanks for all the feedback and tips, I have hopefully addressed the comments
from the first patch by:
- reorganised the patch so that it only uses the printf specifier expression
checker,
- I have also removed the CheckSprintfHandler class and moved the functionality
into CheckPrintfHandler,
- used a switch statement on ConversionSpecifier::Kind to check each specifier,
- used Expr functionality to evaluate constant integer expressions,
- added platform specific checks for pointer formatting,
- I have created a specific error message which reports the size needed.
cheers,
http://reviews.llvm.org/D9665
Files:
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Sema/Sema.h
lib/Sema/SemaChecking.cpp
test/Sema/sprintf-chk.cpp
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -482,6 +482,11 @@
"%0 will always overflow destination buffer">,
InGroup<DiagGroup<"builtin-memcpy-chk-size">>;
+def warn_sprintf_chk_overflow : Warning<
+ "call will always overflow destination buffer as it requires at least %0 "
+ "bytes whereas %1 are provided">,
+ InGroup<Format>;
+
/// main()
// static main() is not an error in C, just in C++.
def warn_static_main : Warning<"'main' should not be declared static">,
Index: include/clang/Sema/Sema.h
===================================================================
--- include/clang/Sema/Sema.h
+++ include/clang/Sema/Sema.h
@@ -8740,6 +8740,7 @@
enum FormatStringType {
FST_Scanf,
FST_Printf,
+ FST_Sprintf, // extra check on top of Printf
FST_NSString,
FST_Strftime,
FST_Strfmon,
@@ -8767,13 +8768,15 @@
bool IsCXXMember,
VariadicCallType CallType,
SourceLocation Loc, SourceRange Range,
- llvm::SmallBitVector &CheckedVarArgs);
+ llvm::SmallBitVector &CheckedVarArgs,
+ bool isSprintf);
bool CheckFormatArguments(ArrayRef<const Expr *> Args,
bool HasVAListArg, unsigned format_idx,
unsigned firstDataArg, FormatStringType Type,
VariadicCallType CallType,
SourceLocation Loc, SourceRange range,
- llvm::SmallBitVector &CheckedVarArgs);
+ llvm::SmallBitVector &CheckedVarArgs,
+ bool isSprintf);
void CheckAbsoluteValueFunction(const CallExpr *Call,
const FunctionDecl *FDecl,
Index: lib/Sema/SemaChecking.cpp
===================================================================
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -1313,12 +1313,16 @@
// Printf and scanf checking.
llvm::SmallBitVector CheckedVarArgs;
if (FDecl) {
+ bool isSprintf = false;
+ if (auto *CastFDecl = dyn_cast<FunctionDecl>(FDecl))
+ if (CastFDecl->getBuiltinID() == Builtin::BI__builtin___sprintf_chk)
+ isSprintf = true;
for (const auto *I : FDecl->specific_attrs<FormatAttr>()) {
// Only create vector if there are format attributes.
CheckedVarArgs.resize(Args.size());
CheckFormatArguments(I, Args, IsMemberFunction, CallType, Loc, Range,
- CheckedVarArgs);
+ CheckedVarArgs, isSprintf);
}
}
@@ -3050,27 +3054,33 @@
bool IsCXXMember,
VariadicCallType CallType,
SourceLocation Loc, SourceRange Range,
- llvm::SmallBitVector &CheckedVarArgs) {
+ llvm::SmallBitVector &CheckedVarArgs,
+ bool isSprintf) {
FormatStringInfo FSI;
if (getFormatStringInfo(Format, IsCXXMember, &FSI))
return CheckFormatArguments(Args, FSI.HasVAListArg, FSI.FormatIdx,
FSI.FirstDataArg, GetFormatStringType(Format),
- CallType, Loc, Range, CheckedVarArgs);
+ CallType, Loc, Range, CheckedVarArgs,
+ isSprintf);
return false;
}
bool Sema::CheckFormatArguments(ArrayRef<const Expr *> Args,
bool HasVAListArg, unsigned format_idx,
unsigned firstDataArg, FormatStringType Type,
VariadicCallType CallType,
SourceLocation Loc, SourceRange Range,
- llvm::SmallBitVector &CheckedVarArgs) {
+ llvm::SmallBitVector &CheckedVarArgs,
+ bool isSprintf) {
// CHECK: printf/scanf-like function is called with no format string.
if (format_idx >= Args.size()) {
Diag(Loc, diag::warn_missing_format_string) << Range;
return false;
}
+ if (isSprintf)
+ Type = FST_Sprintf;
+
const Expr *OrigFormatExpr = Args[format_idx]->IgnoreParenCasts();
// CHECK: format string is not a string literal.
@@ -3523,7 +3533,7 @@
}
}
-//===--- CHECK: Printf format string checking ------------------------------===//
+//===--- CHECK: Printf format string checking -----------------------------===//
namespace {
class CheckPrintfHandler : public CheckFormatHandler {
@@ -3536,12 +3546,15 @@
ArrayRef<const Expr *> Args,
unsigned formatIdx, bool inFunctionCall,
Sema::VariadicCallType CallType,
- llvm::SmallBitVector &CheckedVarArgs)
+ llvm::SmallBitVector &CheckedVarArgs,
+ bool isSprintf)
: CheckFormatHandler(s, fexpr, origFormatExpr, firstDataArg,
numDataArgs, beg, hasVAListArg, Args,
formatIdx, inFunctionCall, CallType, CheckedVarArgs),
- ObjCContext(isObjC)
- {}
+ ObjCContext(isObjC), isSprintf(isSprintf)
+ {
+ SprintfRequiredStorage = 1;
+ }
bool HandleInvalidPrintfConversionSpecifier(
@@ -3557,6 +3570,10 @@
unsigned SpecifierLen,
const Expr *E);
+ bool HandleSprintfSpecifier(const analyze_printf::PrintfSpecifier &FS,
+ const char *startSpecifier,
+ unsigned specifierLen);
+
bool HandleAmount(const analyze_format_string::OptionalAmount &Amt, unsigned k,
const char *startSpecifier, unsigned specifierLen);
void HandleInvalidAmount(const analyze_printf::PrintfSpecifier &FS,
@@ -3572,8 +3589,10 @@
const char *startSpecifier, unsigned specifierLen);
bool checkForCStrMembers(const analyze_printf::ArgType &AT,
const Expr *E);
-
-};
+private:
+ bool isSprintf;
+ uint64_t SprintfRequiredStorage;
+};
}
bool CheckPrintfHandler::HandleInvalidPrintfConversionSpecifier(
@@ -3916,7 +3935,171 @@
if (!Arg)
return true;
- return checkFormatExpr(FS, startSpecifier, specifierLen, Arg);
+
+ if (checkFormatExpr(FS, startSpecifier, specifierLen, Arg)) {
+ if (isSprintf)
+ return HandleSprintfSpecifier(FS, startSpecifier, specifierLen);
+ else
+ return true;
+ }
+ return false;
+}
+
+static const ConstantArrayType *getConstArrayType(Sema &S, const Expr *Array) {
+ return S.Context.getAsConstantArrayType(Array->getType());
+}
+
+// Analyses the data arguments to sprintf to warn the user of memory overflows.
+// Mainly handles integer and string literals with their specifiers and flags
+// with little support for floating point values. None specifier text is also
+// not considered towards the storage space required for the string.
+bool CheckPrintfHandler::HandleSprintfSpecifier(
+ const analyze_printf::PrintfSpecifier &FS,
+ const char *startSpecifier, unsigned specifierLen) {
+
+ using namespace analyze_format_string;
+ using namespace analyze_printf;
+
+ const PrintfConversionSpecifier &CS = FS.getConversionSpecifier();
+ ConversionSpecifier::Kind CSKind = CS.getKind();
+ OptionalAmount Precision = FS.getPrecision();
+ OptionalAmount Width = FS.getFieldWidth();
+ // Offset the argument index to the data items to be printed.
+ unsigned Idx = 4 + FS.getArgIndex();
+ const Expr* Data = Args[Idx]->IgnoreImplicit();
+ uint64_t StorageBufSize = 0;
+ uint64_t RequiredStorage = 0;
+
+ if (auto *StorageBufTy = getConstArrayType(S, Args[0]->IgnoreImplicit()))
+ StorageBufSize = StorageBufTy->getSize().getZExtValue();
+ else
+ return true;
+
+ switch (CSKind) {
+ default:
+ // We can't handle eArg, EArg, fArg, FArg, gArg, GArg, nArg
+ break;
+ case ConversionSpecifier::aArg:
+ case ConversionSpecifier::AArg: {
+ if (isa<FloatingLiteral>(Data)) {
+ llvm::APFloat Value = cast<FloatingLiteral>(Data)->getValue();
+ char TempBuf[32];
+ unsigned HexDigits = 0;
+
+ if (Precision.getHowSpecified() == OptionalAmount::Constant)
+ HexDigits = Precision.getConstantAmount();
+
+ // Get the number of digits used to print the value.
+ unsigned NumDigits =
+ Value.convertToHexString(TempBuf, HexDigits, false,
+ llvm::APFloat::rmNearestTiesToEven);
+ RequiredStorage += NumDigits;
+ }
+ break;
+ }
+ case ConversionSpecifier::cArg:
+ case ConversionSpecifier::CArg:
+ ++RequiredStorage;
+ break;
+ case ConversionSpecifier::dArg:
+ case ConversionSpecifier::DArg:
+ case ConversionSpecifier::iArg:
+ case ConversionSpecifier::uArg:
+ case ConversionSpecifier::UArg: {
+ if (Data->isEvaluatable(S.getASTContext())) {
+ llvm::APSInt Result = llvm::APSInt(64);
+ if (CSKind == ConversionSpecifier::uArg ||
+ CSKind == ConversionSpecifier::UArg)
+ Result = llvm::APSInt(64, false);
+ if (Data->EvaluateAsInt(Result, S.getASTContext())) {
+ std::string DataStr = Result.toString(10, true);
+ RequiredStorage += DataStr.length();
+ }
+ }
+ break;
+ }
+ case ConversionSpecifier::pArg: {
+ // When using the %p specifier, all platforms appear to use hex digits, this
+ // is prepended with '0x' on BSD, Linux and OSX.
+ const TargetInfo &TI = S.Context.getTargetInfo();
+ RequiredStorage += TI.getPointerWidth(0) / 4;
+ if (TI.getTriple().isOSDarwin() ||
+ TI.getTriple().isOSLinux() ||
+ TI.getTriple().isOSNetBSD() ||
+ TI.getTriple().isOSOpenBSD() ||
+ TI.getTriple().isOSFreeBSD() ||
+ TI.getTriple().isOSDragonFly())
+ RequiredStorage += 2;
+ break;
+ }
+ case ConversionSpecifier::sArg:
+ case ConversionSpecifier::SArg:
+ case ConversionSpecifier::ZArg:
+ if (auto SL = dyn_cast<StringLiteral>(Data))
+ RequiredStorage += SL->getByteLength();
+ else if (auto DataBuf = getConstArrayType(S, Data)) {
+ uint64_t BufSize = DataBuf->getSize().getZExtValue();
+ RequiredStorage += BufSize;
+ }
+ break;
+ case ConversionSpecifier::oArg:
+ case ConversionSpecifier::OArg:
+ case ConversionSpecifier::xArg:
+ case ConversionSpecifier::XArg: {
+ unsigned Radix = 16;
+ if (CSKind == ConversionSpecifier::oArg ||
+ CSKind == ConversionSpecifier::OArg)
+ Radix = 8;
+ if (isa<IntegerLiteral>(Data)) {
+ // Create a real string to use its size.
+ std::string DataStr =
+ cast<IntegerLiteral>(Data)->getValue().toString(Radix, true);
+ RequiredStorage += DataStr.length();
+ // 0x will can be prepended for oct and hex values.
+ if (FS.hasAlternativeForm())
+ RequiredStorage += 2;
+ }
+ break;
+ }
+ }
+
+ // An extra will be required for +/-
+ if (FS.hasPlusPrefix() || FS.hasSpacePrefix())
+ ++RequiredStorage;
+
+ // The calculated sizes could be smaller than sizes specified by the user
+ // in their format string.
+ if (CS.isAnyIntArg()) {
+ // For integers, the precision modifier specifies how many digits to be
+ // written so this may overwrite the calculated size.
+ if (Precision.getHowSpecified() == OptionalAmount::Constant) {
+ if (RequiredStorage <= Precision.getConstantAmount())
+ RequiredStorage = Precision.getConstantAmount();
+ }
+ }
+
+ // Width specifies the minimum number of characters to be printed.
+ if (Width.getHowSpecified() == OptionalAmount::Constant) {
+ if (RequiredStorage <= Width.getConstantAmount())
+ RequiredStorage = Width.getConstantAmount();
+ }
+
+ // Update the total required.
+ SprintfRequiredStorage += RequiredStorage;
+
+ if (StorageBufSize < SprintfRequiredStorage) {
+ std::string Limit = llvm::APInt(64, StorageBufSize).toString(10, false);
+ std::string Required =
+ llvm::APInt(64, SprintfRequiredStorage).toString(10, false);
+ S.Diag(Args[0]->getLocStart(), diag::warn_sprintf_chk_overflow)
+ << getSpecifierRange(startSpecifier, specifierLen)
+ << Data->getSourceRange()
+ << Required
+ << Limit;
+ return false;
+ }
+
+ return true;
}
static bool requiresParensToAddCast(const Expr *E) {
@@ -4493,11 +4676,14 @@
}
if (Type == FST_Printf || Type == FST_NSString ||
- Type == FST_FreeBSDKPrintf || Type == FST_OSTrace) {
+ Type == FST_FreeBSDKPrintf || Type == FST_OSTrace ||
+ Type == FST_Sprintf) {
CheckPrintfHandler H(*this, FExpr, OrigFormatExpr, firstDataArg,
- numDataArgs, (Type == FST_NSString || Type == FST_OSTrace),
+ numDataArgs,
+ (Type == FST_NSString || Type == FST_OSTrace),
Str, HasVAListArg, Args, format_idx,
- inFunctionCall, CallType, CheckedVarArgs);
+ inFunctionCall, CallType, CheckedVarArgs,
+ Type == FST_Sprintf);
if (!analyze_format_string::ParsePrintfString(H, Str, Str + StrLen,
getLangOpts(),
Index: test/Sema/sprintf-chk.cpp
===================================================================
--- /dev/null
+++ test/Sema/sprintf-chk.cpp
@@ -0,0 +1,106 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -fsyntax-only %s
+void test() {
+
+ char bufA[4];
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%d", 100);
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%i", 100);
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%u", 100);
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%x", 100);
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%X", 100);
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%o", 100);
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%s", "foo");
+ char addrBuf[19];
+ __builtin___sprintf_chk(addrBuf, 0, __builtin_object_size(addrBuf, 0), "%p",
+ bufA);
+
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%d%d", 10, 10);
+ // expected-warning@-1{{call will always overflow destination buffer as it requires at least 5 bytes whereas 4 are provided}}
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%i%i", 10, 10);
+ // expected-warning@-1{{call will always overflow destination buffer}}
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%u%u", 10, 10);
+ // expected-warning@-1{{call will always overflow destination buffer}}
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%x%x", 100, 100);
+ // expected-warning@-1{{call will always overflow destination buffer}}
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%X%X", 100, 100);
+ // expected-warning@-1{{call will always overflow destination buffer}}
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%o%o", 100, 100);
+ // expected-warning@-1{{call will always overflow destination buffer}}
+
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%d", 1000);
+ // expected-warning@-1{{call will always overflow destination buffer}}
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%i", 1000);
+ // expected-warning@-1{{call will always overflow destination buffer}}
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%u", 1000);
+ // expected-warning@-1{{call will always overflow destination buffer}}
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%x", 10000);
+ // expected-warning@-1{{call will always overflow destination buffer}}
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%X", 10000);
+ // expected-warning@-1{{call will always overflow destination buffer}}
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%o", 1000);
+ // expected-warning@-1{{call will always overflow destination buffer}}
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%a", 10.0);
+ // expected-warning@-1{{call will always overflow destination buffer}}
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%A", 10.0);
+ // expected-warning@-1{{call will always overflow destination buffer}}
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%s", "foobar");
+ // expected-warning@-1{{call will always overflow destination buffer}}
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%p", bufA);
+ // expected-warning@-1{{call will always overflow destination buffer}}
+
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%+d", 100);
+ // expected-warning@-1{{call will always overflow destination buffer}}
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%+d", 10);
+
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%3d", 100);
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%3i", 100);
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%3u", 100);
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%3x", 100);
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%3X", 100);
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%3o", 100);
+
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%4d", 100);
+ // expected-warning@-1{{call will always overflow destination buffer}}
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%4i", 100);
+ // expected-warning@-1{{call will always overflow destination buffer}}
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%4u", 100);
+ // expected-warning@-1{{call will always overflow destination buffer}}
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%4x", 100);
+ // expected-warning@-1{{call will always overflow destination buffer}}
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%4X", 100);
+ // expected-warning@-1{{call will always overflow destination buffer}}
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%4o", 100);
+ // expected-warning@-1{{call will always overflow destination buffer}}
+
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%#x", 15);
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%#X", 15);
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%#o", 7);
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%#x", 100);
+ // expected-warning@-1{{call will always overflow destination buffer}}
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%#X", 100);
+ // expected-warning@-1{{call will always overflow destination buffer}}
+ __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%#o", 100);
+ // expected-warning@-1{{call will always overflow destination buffer}}
+
+
+ char bufB[7];
+ __builtin___sprintf_chk(bufB, 0, __builtin_object_size(bufB, 0), "%a", 0.5);
+ __builtin___sprintf_chk(bufB, 0, __builtin_object_size(bufB, 0), "%A", 0.5);
+ __builtin___sprintf_chk(bufB, 0, __builtin_object_size(bufB, 0), "%a", 10.0);
+ // expected-warning@-1{{call will always overflow destination buffer}}
+ __builtin___sprintf_chk(bufB, 0, __builtin_object_size(bufB, 0), "%A", 10.0);
+ // expected-warning@-1{{call will always overflow destination buffer}}
+ __builtin___sprintf_chk(bufB, 0, __builtin_object_size(bufB, 0), "%1.1a", 0.5);
+ __builtin___sprintf_chk(bufB, 0, __builtin_object_size(bufB, 0), "%1.1A", 0.5);
+ __builtin___sprintf_chk(bufB, 0, __builtin_object_size(bufB, 0), "%1.2a", 0.5);
+ // expected-warning@-1{{call will always overflow destination buffer}}
+ __builtin___sprintf_chk(bufB, 0, __builtin_object_size(bufB, 0), "%1.2A", 0.5);
+ // expected-warning@-1{{call will always overflow destination buffer}}
+
+ char bufC[5];
+ __builtin___sprintf_chk(bufC, 0, __builtin_object_size(bufC, 0), "%s%s",
+ "foo", "bar");
+ // expected-warning@-2{{call will always overflow destination buffer}}
+ const char bar[] = "foobar";
+ __builtin___sprintf_chk(bufC, 0, __builtin_object_size(bufC, 0), "%s", bar);
+ // expected-warning@-1{{call will always overflow destination buffer}}
+}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
