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