serge-sans-paille updated this revision to Diff 234262.
serge-sans-paille added a comment.
Take remarks into account:
- support sprintf when the buffer has a known static size
- use llvm::Optional
- some extra minor tweaks.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71566/new/
https://reviews.llvm.org/D71566
Files:
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Sema/SemaChecking.cpp
clang/test/Sema/warn-fortify-source.c
Index: clang/test/Sema/warn-fortify-source.c
===================================================================
--- clang/test/Sema/warn-fortify-source.c
+++ clang/test/Sema/warn-fortify-source.c
@@ -11,6 +11,8 @@
extern "C" {
#endif
+extern int sprintf(char *str, const char *format, ...);
+
#if defined(USE_PASS_OBJECT_SIZE)
void *memcpy(void *dst, const void *src, size_t c);
static void *memcpy(void *dst __attribute__((pass_object_size(1))), const void *src, size_t c) __attribute__((overloadable)) __asm__("merp");
@@ -96,6 +98,37 @@
__builtin_vsnprintf(buf, 11, "merp", list); // expected-warning {{'vsnprintf' size argument is too large; destination buffer has size 10, but size argument is 11}}
}
+void call_sprintf_chk(char *buf) {
+ __builtin___sprintf_chk(buf, 1, 2, "%d", 9);
+ __builtin___sprintf_chk(buf, 1, 1, "%d", 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, 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, 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, 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}}
+ __builtin___sprintf_chk(buf, 1, 2, "%f", 9.f);
+ __builtin___sprintf_chk(buf, 1, 1, "%f", 9.f); // expected-warning {{'sprintf' will always overflow; destination buffer has size 1, but format string expands to at least 2}}
+}
+
+void call_sprintf() {
+ char buf[6];
+ 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, "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, "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, "%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}}
+ sprintf(buf, "1234%f", 9.f);
+ sprintf(buf, "12345%f", 9.f); // expected-warning {{'sprintf' will always overflow; destination buffer has size 6, but format string expands to at least 7}}
+}
+
#ifdef __cplusplus
template <class> struct S {
void mf() const {
Index: clang/lib/Sema/SemaChecking.cpp
===================================================================
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -309,13 +309,70 @@
return false;
}
+namespace {
+
+class EstimateSizeFormatHandler
+ : public analyze_format_string::FormatStringHandler {
+ size_t Size;
+
+public:
+ EstimateSizeFormatHandler(const StringLiteral *fexpr)
+ : Size(fexpr->getLength() + 1 /* null byte always written by sprintf */) {
+ }
+
+ bool HandlePrintfSpecifier(const analyze_printf::PrintfSpecifier &FS,
+ const char *startSpecifier,
+ unsigned specifierLen) override {
+ const analyze_format_string::OptionalAmount &FW = FS.getFieldWidth();
+ if (FW.getHowSpecified() == analyze_format_string::OptionalAmount::Constant)
+ Size += FW.getConstantAmount();
+ else
+ Size += FS.getConversionSpecifier().isAnyIntArg() ||
+ FS.getConversionSpecifier().isDoubleArg();
+ Size += FS.hasPlusPrefix() || FS.hasSpacePrefix();
+ if (FS.hasAlternativeForm()) {
+ switch (FS.getConversionSpecifier().getKind()) {
+ default:
+ break;
+ case analyze_format_string::ConversionSpecifier::oArg: // leading 0
+ case analyze_format_string::ConversionSpecifier::aArg: // force a .
+ case analyze_format_string::ConversionSpecifier::AArg: // force a .
+ case analyze_format_string::ConversionSpecifier::eArg: // force a .
+ case analyze_format_string::ConversionSpecifier::EArg: // force a .
+ case analyze_format_string::ConversionSpecifier::fArg: // force a .
+ case analyze_format_string::ConversionSpecifier::FArg: // force a .
+ case analyze_format_string::ConversionSpecifier::gArg: // force a .
+ case analyze_format_string::ConversionSpecifier::GArg: // force a .
+ Size += 1;
+ break;
+ case analyze_format_string::ConversionSpecifier::xArg: // leading 0x
+ case analyze_format_string::ConversionSpecifier::XArg: // leading 0x
+ Size += 2;
+ break;
+ }
+ }
+ switch (FS.getConversionSpecifier().getKind()) {
+ case analyze_format_string::ConversionSpecifier::pArg: // leading 0x
+ Size += 3;
+ break;
+ default:
+ break;
+ }
+ Size -= specifierLen;
+ return true;
+ }
+
+ size_t getSizeLowerBound() const { return Size; }
+};
+
+} // namespace
+
/// Check a call to BuiltinID for buffer overflows. If BuiltinID is a
/// __builtin_*_chk function, then use the object size argument specified in the
/// source. Otherwise, infer the object size using __builtin_object_size.
void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD,
CallExpr *TheCall) {
// FIXME: There are some more useful checks we could be doing here:
- // - Analyze the format string of sprintf to see how much of buffer is used.
// - Evaluate strlen of strcpy arguments, use as object size.
if (TheCall->isValueDependent() || TheCall->isTypeDependent() ||
@@ -326,12 +383,51 @@
if (!BuiltinID)
return;
+ const TargetInfo &TI = getASTContext().getTargetInfo();
+ unsigned SizeTypeWidth = TI.getTypeWidth(TI.getSizeType());
+
unsigned DiagID = 0;
bool IsChkVariant = false;
+ Optional<llvm::APSInt> UsedSize;
unsigned SizeIndex, ObjectIndex;
switch (BuiltinID) {
default:
return;
+ case Builtin::BIsprintf:
+ case Builtin::BI__builtin___sprintf_chk: {
+ size_t FormatIndex = BuiltinID == Builtin::BIsprintf ? 1 : 3;
+ auto *FormatExpr = TheCall->getArg(FormatIndex)->IgnoreParenImpCasts();
+
+ if (auto *Format = dyn_cast<StringLiteral>(FormatExpr)) {
+ EstimateSizeFormatHandler H(Format);
+ StringRef FormatStrRef = Format->getString();
+ const char *FormatBytes = FormatStrRef.data();
+ const ConstantArrayType *T =
+ Context.getAsConstantArrayType(Format->getType());
+ assert(T && "String literal not of constant array type!");
+ size_t TypeSize = T->getSize().getZExtValue();
+
+ // In case there's a null byte somewhere.
+ size_t StrLen =
+ std::min(std::max(TypeSize, size_t(1)) - 1, FormatStrRef.size());
+ if (!analyze_format_string::ParsePrintfString(
+ H, FormatBytes, FormatBytes + StrLen, getLangOpts(),
+ Context.getTargetInfo(), false)) {
+ DiagID = diag::warn_fortify_source_format_overflow;
+ UsedSize = llvm::APSInt::getUnsigned(H.getSizeLowerBound())
+ .extOrTrunc(SizeTypeWidth);
+ if (BuiltinID == Builtin::BI__builtin___sprintf_chk) {
+ IsChkVariant = true;
+ ObjectIndex = 2;
+ } else {
+ IsChkVariant = false;
+ ObjectIndex = 0;
+ }
+ break;
+ }
+ }
+ return;
+ }
case Builtin::BI__builtin___memcpy_chk:
case Builtin::BI__builtin___memmove_chk:
case Builtin::BI__builtin___memset_chk:
@@ -421,19 +517,19 @@
if (!ObjArg->tryEvaluateObjectSize(Result, getASTContext(), BOSType))
return;
// Get the object size in the target's size_t width.
- const TargetInfo &TI = getASTContext().getTargetInfo();
- unsigned SizeTypeWidth = TI.getTypeWidth(TI.getSizeType());
ObjectSize = llvm::APSInt::getUnsigned(Result).extOrTrunc(SizeTypeWidth);
}
// Evaluate the number of bytes of the object that this call will use.
- Expr::EvalResult Result;
- Expr *UsedSizeArg = TheCall->getArg(SizeIndex);
- if (!UsedSizeArg->EvaluateAsInt(Result, getASTContext()))
- return;
- llvm::APSInt UsedSize = Result.Val.getInt();
-
- if (UsedSize.ule(ObjectSize))
+ if (!UsedSize) {
+ Expr::EvalResult Result;
+ Expr *UsedSizeArg = TheCall->getArg(SizeIndex);
+ if (!UsedSizeArg->EvaluateAsInt(Result, getASTContext()))
+ return;
+ UsedSize = Result.Val.getInt().extOrTrunc(SizeTypeWidth);
+ }
+ assert(UsedSize && "Found a size estimate");
+ if (UsedSize.getValue().ule(ObjectSize))
return;
StringRef FunctionName = getASTContext().BuiltinInfo.getName(BuiltinID);
@@ -449,7 +545,7 @@
DiagRuntimeBehavior(TheCall->getBeginLoc(), TheCall,
PDiag(DiagID)
<< FunctionName << ObjectSize.toString(/*Radix=*/10)
- << UsedSize.toString(/*Radix=*/10));
+ << UsedSize.getValue().toString(/*Radix=*/10));
}
static bool SemaBuiltinSEHScopeCheck(Sema &SemaRef, CallExpr *TheCall,
@@ -8867,6 +8963,7 @@
S.Context.getTargetInfo(),
Type == Sema::FST_FreeBSDKPrintf))
H.DoneProcessing();
+
} else if (Type == Sema::FST_Scanf) {
CheckScanfHandler H(S, FExpr, OrigFormatExpr, Type, firstDataArg,
numDataArgs, Str, HasVAListArg, Args, format_idx,
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -741,6 +741,12 @@
"'%0' size argument is too large; destination buffer has size %1,"
" but size argument is %2">, InGroup<FortifySource>;
+def warn_fortify_source_format_overflow : Warning<
+ "'%0' will always overflow; destination buffer has size %1,"
+ " but format string expands to at least %2">,
+ InGroup<FortifySource>;
+
+
/// main()
// static main() is not an error in C, just in C++.
def warn_static_main : Warning<"'main' should not be declared static">,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
