diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 74272ba523f1..bd8276fc0f92 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -4655,9 +4655,6 @@ def warn_format_invalid_positional_specifier : Warning< def warn_format_mix_positional_nonpositional_args : Warning< "cannot mix positional and non-positional arguments in format string">, InGroup; -def warn_null_arg : Warning< - "null passed to a callee which requires a non-null argument">, - InGroup; def warn_static_array_too_small : Warning< "array argument is too small; contains %0 elements, callee requires at least %1">, InGroup>; @@ -4689,7 +4686,12 @@ def warn_printf_ignored_flag: Warning< def warn_scanf_scanlist_incomplete : Warning< "no closing ']' for '%%[' in scanf format string">, InGroup; - +def note_format_string_defined : Note<"format string is defined here">; + +def warn_null_arg : Warning< + "null passed to a callee which requires a non-null argument">, + InGroup; + // CHECK: returning address/reference of stack memory def warn_ret_stack_addr : Warning< "address of stack memory associated with local variable %0 returned">, diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index cba8134fca4d..f1b0befcbf40 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -6185,12 +6185,13 @@ private: bool SemaCheckStringLiteral(const Expr *E, const CallExpr *TheCall, bool HasVAListArg, unsigned format_idx, - unsigned firstDataArg, bool isPrintf); + unsigned firstDataArg, bool isPrintf, + bool inFunctionCall = true); void CheckFormatString(const StringLiteral *FExpr, const Expr *OrigFormatExpr, const CallExpr *TheCall, bool HasVAListArg, unsigned format_idx, unsigned firstDataArg, - bool isPrintf); + bool isPrintf, bool inFunctionCall); void CheckNonNullArguments(const NonNullAttr *NonNull, const Expr * const *ExprArgs, diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp index 6e81c3c9661d..715abb9c0a2a 100644 --- a/clang/lib/Sema/SemaChecking.cpp +++ b/clang/lib/Sema/SemaChecking.cpp @@ -1215,7 +1215,7 @@ bool Sema::SemaBuiltinLongjmp(CallExpr *TheCall) { bool Sema::SemaCheckStringLiteral(const Expr *E, const CallExpr *TheCall, bool HasVAListArg, unsigned format_idx, unsigned firstDataArg, - bool isPrintf) { + bool isPrintf, bool inFunctionCall) { tryAgain: if (E->isTypeDependent() || E->isValueDependent()) return false; @@ -1227,9 +1227,11 @@ bool Sema::SemaCheckStringLiteral(const Expr *E, const CallExpr *TheCall, case Stmt::ConditionalOperatorClass: { const AbstractConditionalOperator *C = cast(E); return SemaCheckStringLiteral(C->getTrueExpr(), TheCall, HasVAListArg, - format_idx, firstDataArg, isPrintf) + format_idx, firstDataArg, isPrintf, + inFunctionCall) && SemaCheckStringLiteral(C->getFalseExpr(), TheCall, HasVAListArg, - format_idx, firstDataArg, isPrintf); + format_idx, firstDataArg, isPrintf, + inFunctionCall); } case Stmt::IntegerLiteralClass: @@ -1277,7 +1279,7 @@ bool Sema::SemaCheckStringLiteral(const Expr *E, const CallExpr *TheCall, if (const Expr *Init = VD->getAnyInitializer()) return SemaCheckStringLiteral(Init, TheCall, HasVAListArg, format_idx, firstDataArg, - isPrintf); + isPrintf, /*inFunctionCall*/false); } // For vprintf* functions (i.e., HasVAListArg==true), we add a @@ -1317,7 +1319,8 @@ bool Sema::SemaCheckStringLiteral(const Expr *E, const CallExpr *TheCall, const Expr *Arg = CE->getArg(ArgIndex - 1); return SemaCheckStringLiteral(Arg, TheCall, HasVAListArg, - format_idx, firstDataArg, isPrintf); + format_idx, firstDataArg, isPrintf, + inFunctionCall); } } } @@ -1336,7 +1339,7 @@ bool Sema::SemaCheckStringLiteral(const Expr *E, const CallExpr *TheCall, if (StrE) { CheckFormatString(StrE, E, TheCall, HasVAListArg, format_idx, - firstDataArg, isPrintf); + firstDataArg, isPrintf, inFunctionCall); return true; } @@ -1440,19 +1443,22 @@ protected: llvm::BitVector CoveredArgs; bool usesPositionalArgs; bool atFirstArg; + bool inFunctionCall; public: CheckFormatHandler(Sema &s, const StringLiteral *fexpr, const Expr *origFormatExpr, unsigned firstDataArg, unsigned numDataArgs, bool isObjCLiteral, const char *beg, bool hasVAListArg, - const CallExpr *theCall, unsigned formatIdx) + const CallExpr *theCall, unsigned formatIdx, + bool inFunctionCall) : S(s), FExpr(fexpr), OrigFormatExpr(origFormatExpr), FirstDataArg(firstDataArg), NumDataArgs(numDataArgs), IsObjCLiteral(isObjCLiteral), Beg(beg), HasVAListArg(hasVAListArg), TheCall(theCall), FormatIdx(formatIdx), - usesPositionalArgs(false), atFirstArg(true) { + usesPositionalArgs(false), atFirstArg(true), + inFunctionCall(inFunctionCall) { CoveredArgs.resize(numDataArgs); CoveredArgs.reset(); } @@ -1470,11 +1476,23 @@ public: void HandleNullChar(const char *nullCharacter); + template + static void EmitFormatDiagnostic(Sema &S, bool inFunctionCall, + const Expr *ArgumentExpr, + PartialDiagnostic PDiag, + SourceLocation StringLoc, + bool IsStringLocation, Range StringRange, + FixItHint Fixit = FixItHint()); + protected: bool HandleInvalidConversionSpecifier(unsigned argIndex, SourceLocation Loc, const char *startSpec, unsigned specifierLen, const char *csStart, unsigned csLen); + + void HandlePositionalNonpositionalArgs(SourceLocation Loc, + const char *startSpec, + unsigned specifierLen); SourceRange getFormatStringRange(); CharSourceRange getSpecifierRange(const char *startSpecifier, @@ -1487,6 +1505,14 @@ protected: const analyze_format_string::ConversionSpecifier &CS, const char *startSpecifier, unsigned specifierLen, unsigned argIndex); + + template + void EmitFormatDiagnostic(PartialDiagnostic PDiag, SourceLocation StringLoc, + bool IsStringLocation, Range StringRange, + FixItHint Fixit = FixItHint()); + + void CheckPositionalAndNonpositionalArgs( + const analyze_format_string::FormatSpecifier *FS); }; } @@ -1511,32 +1537,36 @@ SourceLocation CheckFormatHandler::getLocationOfByte(const char *x) { void CheckFormatHandler::HandleIncompleteSpecifier(const char *startSpecifier, unsigned specifierLen){ - SourceLocation Loc = getLocationOfByte(startSpecifier); - S.Diag(Loc, diag::warn_printf_incomplete_specifier) - << getSpecifierRange(startSpecifier, specifierLen); + EmitFormatDiagnostic(S.PDiag(diag::warn_printf_incomplete_specifier), + getLocationOfByte(startSpecifier), + /*IsStringLocation*/true, + getSpecifierRange(startSpecifier, specifierLen)); } void CheckFormatHandler::HandleInvalidPosition(const char *startPos, unsigned posLen, analyze_format_string::PositionContext p) { - SourceLocation Loc = getLocationOfByte(startPos); - S.Diag(Loc, diag::warn_format_invalid_positional_specifier) - << (unsigned) p << getSpecifierRange(startPos, posLen); + EmitFormatDiagnostic(S.PDiag(diag::warn_format_invalid_positional_specifier) + << (unsigned) p, + getLocationOfByte(startPos), /*IsStringLocation*/true, + getSpecifierRange(startPos, posLen)); } void CheckFormatHandler::HandleZeroPosition(const char *startPos, unsigned posLen) { - SourceLocation Loc = getLocationOfByte(startPos); - S.Diag(Loc, diag::warn_format_zero_positional_specifier) - << getSpecifierRange(startPos, posLen); + EmitFormatDiagnostic(S.PDiag(diag::warn_format_zero_positional_specifier), + getLocationOfByte(startPos), + /*IsStringLocation*/true, + getSpecifierRange(startPos, posLen)); } void CheckFormatHandler::HandleNullChar(const char *nullCharacter) { if (!IsObjCLiteral) { // The presence of a null character is likely an error. - S.Diag(getLocationOfByte(nullCharacter), - diag::warn_printf_format_string_contains_null_char) - << getFormatStringRange(); + EmitFormatDiagnostic( + S.PDiag(diag::warn_printf_format_string_contains_null_char), + getLocationOfByte(nullCharacter), /*IsStringLocation*/true, + getFormatStringRange()); } } @@ -1553,9 +1583,9 @@ void CheckFormatHandler::DoneProcessing() { signed notCoveredArg = CoveredArgs.find_first(); if (notCoveredArg >= 0) { assert((unsigned)notCoveredArg < NumDataArgs); - S.Diag(getDataArg((unsigned) notCoveredArg)->getLocStart(), - diag::warn_printf_data_arg_not_used) - << getFormatStringRange(); + EmitFormatDiagnostic(S.PDiag(diag::warn_printf_data_arg_not_used), + getDataArg((unsigned) notCoveredArg)->getLocStart(), + /*IsStringLocation*/false, getFormatStringRange()); } } } @@ -1583,13 +1613,23 @@ CheckFormatHandler::HandleInvalidConversionSpecifier(unsigned argIndex, keepGoing = false; } - S.Diag(Loc, diag::warn_format_invalid_conversion) - << StringRef(csStart, csLen) - << getSpecifierRange(startSpec, specifierLen); + EmitFormatDiagnostic(S.PDiag(diag::warn_format_invalid_conversion) + << StringRef(csStart, csLen), + Loc, /*IsStringLocation*/true, + getSpecifierRange(startSpec, specifierLen)); return keepGoing; } +void +CheckFormatHandler::HandlePositionalNonpositionalArgs(SourceLocation Loc, + const char *startSpec, + unsigned specifierLen) { + EmitFormatDiagnostic( + S.PDiag(diag::warn_format_mix_positional_nonpositional_args), + Loc, /*isStringLoc*/true, getSpecifierRange(startSpec, specifierLen)); +} + bool CheckFormatHandler::CheckNumArgs( const analyze_format_string::FormatSpecifier &FS, @@ -1597,23 +1637,74 @@ CheckFormatHandler::CheckNumArgs( const char *startSpecifier, unsigned specifierLen, unsigned argIndex) { if (argIndex >= NumDataArgs) { - if (FS.usesPositionalArg()) { - S.Diag(getLocationOfByte(CS.getStart()), - diag::warn_printf_positional_arg_exceeds_data_args) - << (argIndex+1) << NumDataArgs - << getSpecifierRange(startSpecifier, specifierLen); - } - else { - S.Diag(getLocationOfByte(CS.getStart()), - diag::warn_printf_insufficient_data_args) - << getSpecifierRange(startSpecifier, specifierLen); - } - + PartialDiagnostic PDiag = FS.usesPositionalArg() + ? (S.PDiag(diag::warn_printf_positional_arg_exceeds_data_args) + << (argIndex+1) << NumDataArgs) + : S.PDiag(diag::warn_printf_insufficient_data_args); + EmitFormatDiagnostic( + PDiag, getLocationOfByte(CS.getStart()), /*IsStringLocation*/true, + getSpecifierRange(startSpecifier, specifierLen)); return false; } return true; } +template +void CheckFormatHandler::EmitFormatDiagnostic(PartialDiagnostic PDiag, + SourceLocation Loc, + bool IsStringLocation, + Range StringRange, + FixItHint FixIt) { + EmitFormatDiagnostic(S, inFunctionCall, TheCall->getArg(FormatIdx), PDiag, + Loc, IsStringLocation, StringRange, FixIt); +} + +/// \brief If the format string is not within the funcion call, emit a note +/// so that the function call and string are in diagnostic messages. +/// +/// \param inFunctionCall if true, the format string is within the function +/// call and only one diagnostic message will be produced. Otherwise, an +/// extra note will be emitted pointing to location of the format string. +/// +/// \param ArgumentExpr the expression that is passed as the format string +/// argument in the function call. Used for getting locations when two +/// diagnostics are emitted. +/// +/// \param PDiag the callee should already have provided any strings for the +/// diagnostic message. This function only adds locations and fixits +/// to diagnostics. +/// +/// \param Loc primary location for diagnostic. If two diagnostics are +/// required, one will be at Loc and a new SourceLocation will be created for +/// the other one. +/// +/// \param IsStringLocation if true, Loc points to the format string should be +/// used for the note. Otherwise, Loc points to the argument list and will +/// be used with PDiag. +/// +/// \param StringRange some or all of the string to highlight. This is +/// templated so it can accept either a CharSourceRange or a SourceRange. +/// +/// \param Fixit optional fix it hint for the format string. +template +void CheckFormatHandler::EmitFormatDiagnostic(Sema &S, bool InFunctionCall, + const Expr *ArgumentExpr, + PartialDiagnostic PDiag, + SourceLocation Loc, + bool IsStringLocation, + Range StringRange, + FixItHint FixIt) { + if (InFunctionCall) + S.Diag(Loc, PDiag) << StringRange << FixIt; + else { + S.Diag(IsStringLocation ? ArgumentExpr->getExprLoc() : Loc, PDiag) + << ArgumentExpr->getSourceRange(); + S.Diag(IsStringLocation ? Loc : StringRange.getBegin(), + diag::note_format_string_defined) + << StringRange << FixIt; + } +} + //===--- CHECK: Printf format string checking ------------------------------===// namespace { @@ -1623,10 +1714,11 @@ public: const Expr *origFormatExpr, unsigned firstDataArg, unsigned numDataArgs, bool isObjCLiteral, const char *beg, bool hasVAListArg, - const CallExpr *theCall, unsigned formatIdx) + const CallExpr *theCall, unsigned formatIdx, + bool inFunctionCall) : CheckFormatHandler(s, fexpr, origFormatExpr, firstDataArg, numDataArgs, isObjCLiteral, beg, hasVAListArg, - theCall, formatIdx) {} + theCall, formatIdx, inFunctionCall) {} bool HandleInvalidPrintfConversionSpecifier( @@ -1676,9 +1768,11 @@ bool CheckPrintfHandler::HandleAmount( if (!HasVAListArg) { unsigned argIndex = Amt.getArgIndex(); if (argIndex >= NumDataArgs) { - S.Diag(getLocationOfByte(Amt.getStart()), - diag::warn_printf_asterisk_missing_arg) - << k << getSpecifierRange(startSpecifier, specifierLen); + EmitFormatDiagnostic(S.PDiag(diag::warn_printf_asterisk_missing_arg) + << k, + getLocationOfByte(Amt.getStart()), + /*IsStringLocation*/true, + getSpecifierRange(startSpecifier, specifierLen)); // Don't do any more checking. We will just emit // spurious errors. return false; @@ -1696,12 +1790,12 @@ bool CheckPrintfHandler::HandleAmount( assert(ATR.isValid()); if (!ATR.matchesType(S.Context, T)) { - S.Diag(getLocationOfByte(Amt.getStart()), - diag::warn_printf_asterisk_wrong_type) - << k - << ATR.getRepresentativeType(S.Context) << T - << getSpecifierRange(startSpecifier, specifierLen) - << Arg->getSourceRange(); + EmitFormatDiagnostic(S.PDiag(diag::warn_printf_asterisk_wrong_type) + << k << ATR.getRepresentativeType(S.Context) + << T << Arg->getSourceRange(), + getLocationOfByte(Amt.getStart()), + /*IsStringLocation*/true, + getSpecifierRange(startSpecifier, specifierLen)); // Don't do any more checking. We will just emit // spurious errors. return false; @@ -1719,25 +1813,19 @@ void CheckPrintfHandler::HandleInvalidAmount( unsigned specifierLen) { const analyze_printf::PrintfConversionSpecifier &CS = FS.getConversionSpecifier(); - switch (Amt.getHowSpecified()) { - case analyze_printf::OptionalAmount::Constant: - S.Diag(getLocationOfByte(Amt.getStart()), - diag::warn_printf_nonsensical_optional_amount) - << type - << CS.toString() - << getSpecifierRange(startSpecifier, specifierLen) - << FixItHint::CreateRemoval(getSpecifierRange(Amt.getStart(), - Amt.getConstantLength())); - break; - default: - S.Diag(getLocationOfByte(Amt.getStart()), - diag::warn_printf_nonsensical_optional_amount) - << type - << CS.toString() - << getSpecifierRange(startSpecifier, specifierLen); - break; - } + FixItHint fixit = + Amt.getHowSpecified() == analyze_printf::OptionalAmount::Constant + ? FixItHint::CreateRemoval(getSpecifierRange(Amt.getStart(), + Amt.getConstantLength())) + : FixItHint(); + + EmitFormatDiagnostic(S.PDiag(diag::warn_printf_nonsensical_optional_amount) + << type << CS.toString(), + getLocationOfByte(Amt.getStart()), + /*IsStringLocation*/true, + getSpecifierRange(startSpecifier, specifierLen), + fixit); } void CheckPrintfHandler::HandleFlag(const analyze_printf::PrintfSpecifier &FS, @@ -1747,11 +1835,13 @@ void CheckPrintfHandler::HandleFlag(const analyze_printf::PrintfSpecifier &FS, // Warn about pointless flag with a fixit removal. const analyze_printf::PrintfConversionSpecifier &CS = FS.getConversionSpecifier(); - S.Diag(getLocationOfByte(flag.getPosition()), - diag::warn_printf_nonsensical_flag) - << flag.toString() << CS.toString() - << getSpecifierRange(startSpecifier, specifierLen) - << FixItHint::CreateRemoval(getSpecifierRange(flag.getPosition(), 1)); + EmitFormatDiagnostic(S.PDiag(diag::warn_printf_nonsensical_flag) + << flag.toString() << CS.toString(), + getLocationOfByte(flag.getPosition()), + /*IsStringLocation*/true, + getSpecifierRange(startSpecifier, specifierLen), + FixItHint::CreateRemoval( + getSpecifierRange(flag.getPosition(), 1))); } void CheckPrintfHandler::HandleIgnoredFlag( @@ -1761,12 +1851,13 @@ void CheckPrintfHandler::HandleIgnoredFlag( const char *startSpecifier, unsigned specifierLen) { // Warn about ignored flag with a fixit removal. - S.Diag(getLocationOfByte(ignoredFlag.getPosition()), - diag::warn_printf_ignored_flag) - << ignoredFlag.toString() << flag.toString() - << getSpecifierRange(startSpecifier, specifierLen) - << FixItHint::CreateRemoval(getSpecifierRange( - ignoredFlag.getPosition(), 1)); + EmitFormatDiagnostic(S.PDiag(diag::warn_printf_ignored_flag) + << ignoredFlag.toString() << flag.toString(), + getLocationOfByte(ignoredFlag.getPosition()), + /*IsStringLocation*/true, + getSpecifierRange(startSpecifier, specifierLen), + FixItHint::CreateRemoval( + getSpecifierRange(ignoredFlag.getPosition(), 1))); } bool @@ -1785,10 +1876,8 @@ CheckPrintfHandler::HandlePrintfSpecifier(const analyze_printf::PrintfSpecifier usesPositionalArgs = FS.usesPositionalArg(); } else if (usesPositionalArgs != FS.usesPositionalArg()) { - // Cannot mix-and-match positional and non-positional arguments. - S.Diag(getLocationOfByte(CS.getStart()), - diag::warn_format_mix_positional_nonpositional_args) - << getSpecifierRange(startSpecifier, specifierLen); + HandlePositionalNonpositionalArgs(getLocationOfByte(CS.getStart()), + startSpecifier, specifierLen); return false; } } @@ -1864,18 +1953,22 @@ CheckPrintfHandler::HandlePrintfSpecifier(const analyze_printf::PrintfSpecifier // Check the length modifier is valid with the given conversion specifier. const LengthModifier &LM = FS.getLengthModifier(); if (!FS.hasValidLengthModifier()) - S.Diag(getLocationOfByte(LM.getStart()), - diag::warn_format_nonsensical_length) - << LM.toString() << CS.toString() - << getSpecifierRange(startSpecifier, specifierLen) - << FixItHint::CreateRemoval(getSpecifierRange(LM.getStart(), - LM.getLength())); + EmitFormatDiagnostic(S.PDiag(diag::warn_format_nonsensical_length) + << LM.toString() << CS.toString(), + getLocationOfByte(LM.getStart()), + /*IsStringLocation*/true, + getSpecifierRange(startSpecifier, specifierLen), + FixItHint::CreateRemoval( + getSpecifierRange(LM.getStart(), + LM.getLength()))); // Are we using '%n'? if (CS.getKind() == ConversionSpecifier::nArg) { // Issue a warning about this being a possible security issue. - S.Diag(getLocationOfByte(CS.getStart()), diag::warn_printf_write_back) - << getSpecifierRange(startSpecifier, specifierLen); + EmitFormatDiagnostic(S.PDiag(diag::warn_printf_write_back), + getLocationOfByte(CS.getStart()), + /*IsStringLocation*/true, + getSpecifierRange(startSpecifier, specifierLen)); // Continue checking the other format specifiers. return true; } @@ -1916,14 +2009,16 @@ CheckPrintfHandler::HandlePrintfSpecifier(const analyze_printf::PrintfSpecifier // FIXME: getRepresentativeType() perhaps should return a string // instead of a QualType to better handle when the representative // type is 'wint_t' (which is defined in the system headers). - S.Diag(getLocationOfByte(CS.getStart()), - diag::warn_printf_conversion_argument_type_mismatch) - << ATR.getRepresentativeType(S.Context) << Ex->getType() - << getSpecifierRange(startSpecifier, specifierLen) - << Ex->getSourceRange() - << FixItHint::CreateReplacement( - getSpecifierRange(startSpecifier, specifierLen), - os.str()); + EmitFormatDiagnostic( + S.PDiag(diag::warn_printf_conversion_argument_type_mismatch) + << ATR.getRepresentativeType(S.Context) << Ex->getType() + << Ex->getSourceRange(), + getLocationOfByte(CS.getStart()), + /*IsStringLocation*/true, + getSpecifierRange(startSpecifier, specifierLen), + FixItHint::CreateReplacement( + getSpecifierRange(startSpecifier, specifierLen), + os.str())); } else { S.Diag(getLocationOfByte(CS.getStart()), @@ -1946,10 +2041,11 @@ public: const Expr *origFormatExpr, unsigned firstDataArg, unsigned numDataArgs, bool isObjCLiteral, const char *beg, bool hasVAListArg, - const CallExpr *theCall, unsigned formatIdx) + const CallExpr *theCall, unsigned formatIdx, + bool inFunctionCall) : CheckFormatHandler(s, fexpr, origFormatExpr, firstDataArg, numDataArgs, isObjCLiteral, beg, hasVAListArg, - theCall, formatIdx) {} + theCall, formatIdx, inFunctionCall) {} bool HandleScanfSpecifier(const analyze_scanf::ScanfSpecifier &FS, const char *startSpecifier, @@ -1966,8 +2062,9 @@ public: void CheckScanfHandler::HandleIncompleteScanList(const char *start, const char *end) { - S.Diag(getLocationOfByte(end), diag::warn_scanf_scanlist_incomplete) - << getSpecifierRange(start, end - start); + EmitFormatDiagnostic(S.PDiag(diag::warn_scanf_scanlist_incomplete), + getLocationOfByte(end), /*IsStringLocation*/true, + getSpecifierRange(start, end - start)); } bool CheckScanfHandler::HandleInvalidScanfConversionSpecifier( @@ -2002,10 +2099,8 @@ bool CheckScanfHandler::HandleScanfSpecifier( usesPositionalArgs = FS.usesPositionalArg(); } else if (usesPositionalArgs != FS.usesPositionalArg()) { - // Cannot mix-and-match positional and non-positional arguments. - S.Diag(getLocationOfByte(CS.getStart()), - diag::warn_format_mix_positional_nonpositional_args) - << getSpecifierRange(startSpecifier, specifierLen); + HandlePositionalNonpositionalArgs(getLocationOfByte(CS.getStart()), + startSpecifier, specifierLen); return false; } } @@ -2016,9 +2111,10 @@ bool CheckScanfHandler::HandleScanfSpecifier( if (Amt.getConstantAmount() == 0) { const CharSourceRange &R = getSpecifierRange(Amt.getStart(), Amt.getConstantLength()); - S.Diag(getLocationOfByte(Amt.getStart()), - diag::warn_scanf_nonzero_width) - << R << FixItHint::CreateRemoval(R); + EmitFormatDiagnostic(S.PDiag(diag::warn_scanf_nonzero_width), + getLocationOfByte(Amt.getStart()), + /*IsStringLocation*/true, R, + FixItHint::CreateRemoval(R)); } } @@ -2064,13 +2160,14 @@ void Sema::CheckFormatString(const StringLiteral *FExpr, const Expr *OrigFormatExpr, const CallExpr *TheCall, bool HasVAListArg, unsigned format_idx, unsigned firstDataArg, - bool isPrintf) { + bool isPrintf, bool inFunctionCall) { // CHECK: is the format string a wide literal? if (!FExpr->isAscii()) { - Diag(FExpr->getLocStart(), - diag::warn_format_string_is_wide_literal) - << OrigFormatExpr->getSourceRange(); + CheckFormatHandler::EmitFormatDiagnostic( + *this, inFunctionCall, TheCall->getArg(format_idx), + PDiag(diag::warn_format_string_is_wide_literal), FExpr->getLocStart(), + /*IsStringLocation*/true, OrigFormatExpr->getSourceRange()); return; } @@ -2082,15 +2179,18 @@ void Sema::CheckFormatString(const StringLiteral *FExpr, // CHECK: empty format string? if (StrLen == 0 && numDataArgs > 0) { - Diag(FExpr->getLocStart(), diag::warn_empty_format_string) - << OrigFormatExpr->getSourceRange(); + CheckFormatHandler::EmitFormatDiagnostic( + *this, inFunctionCall, TheCall->getArg(format_idx), + PDiag(diag::warn_empty_format_string), FExpr->getLocStart(), + /*IsStringLocation*/true, OrigFormatExpr->getSourceRange()); return; } if (isPrintf) { CheckPrintfHandler H(*this, FExpr, OrigFormatExpr, firstDataArg, numDataArgs, isa(OrigFormatExpr), - Str, HasVAListArg, TheCall, format_idx); + Str, HasVAListArg, TheCall, format_idx, + inFunctionCall); if (!analyze_format_string::ParsePrintfString(H, Str, Str + StrLen)) H.DoneProcessing(); @@ -2098,7 +2198,8 @@ void Sema::CheckFormatString(const StringLiteral *FExpr, else { CheckScanfHandler H(*this, FExpr, OrigFormatExpr, firstDataArg, numDataArgs, isa(OrigFormatExpr), - Str, HasVAListArg, TheCall, format_idx); + Str, HasVAListArg, TheCall, format_idx, + inFunctionCall); if (!analyze_format_string::ParseScanfString(H, Str, Str + StrLen)) H.DoneProcessing(); diff --git a/clang/test/Sema/format-strings-no-fixit.c b/clang/test/Sema/format-strings-no-fixit.c new file mode 100644 index 000000000000..701e945f6902 --- /dev/null +++ b/clang/test/Sema/format-strings-no-fixit.c @@ -0,0 +1,65 @@ +// RUN: cp %s %t +// RUN: %clang_cc1 -fsyntax-only -fixit %t +// RUN: %clang_cc1 -E -o - %t | FileCheck %s + +/* This is a test of the various code modification hints that are + provided as part of warning or extension diagnostics. Only + warnings for format strings within the function call will be + fixed by -fixit. Other format strings will be left alone. */ + +int printf(char const *, ...); +int scanf(char const *, ...); + +void pr9751() { + const char kFormat1[] = "%s"; + printf(kFormat1, 5); + printf("%s", 5); + + const char kFormat2[] = "%.3p"; + void *p; + printf(kFormat2, p); + printf("%.3p", p); + + const char kFormat3[] = "%0s"; + printf(kFormat3, "a"); + printf("%0s", "a"); + + const char kFormat4[] = "%hhs"; + printf(kFormat4, "a"); + printf("%hhs", "a"); + + const char kFormat5[] = "%-0d"; + printf(kFormat5, 5); + printf("%-0d", 5); + + const char kFormat6[] = "%00d"; + int *i; + scanf(kFormat6, i); + scanf("%00d", i); +} + +// CHECK: const char kFormat1[] = "%s"; +// CHECK: printf(kFormat1, 5); +// CHECK: printf("%d", 5); + +// CHECK: const char kFormat2[] = "%.3p"; +// CHECK: void *p; +// CHECK: printf(kFormat2, p); +// CHECK: printf("%p", p); + +// CHECK: const char kFormat3[] = "%0s"; +// CHECK: printf(kFormat3, "a"); +// CHECK: printf("%s", "a"); + +// CHECK: const char kFormat4[] = "%hhs"; +// CHECK: printf(kFormat4, "a"); +// CHECK: printf("%s", "a"); + +// CHECK: const char kFormat5[] = "%-0d"; +// CHECK: printf(kFormat5, 5); +// CHECK: printf("%-d", 5); + +// CHECK: const char kFormat6[] = "%00d"; +// CHECK: int *i; +// CHECK: scanf(kFormat6, i); +// CHECK: scanf("%d", i); diff --git a/clang/test/Sema/format-strings-scanf.c b/clang/test/Sema/format-strings-scanf.c index 42b6c03ffbff..ee559daef615 100644 --- a/clang/test/Sema/format-strings-scanf.c +++ b/clang/test/Sema/format-strings-scanf.c @@ -32,3 +32,15 @@ void bad_length_modifiers(char *s, void *p, wchar_t *ws, long double *ld) { scanf("%ls", ws); // no-warning scanf("%#.2Lf", ld); // expected-warning{{invalid conversion specifier '#'}} } + +// Test that the scanf call site is where the warning is attached. If the +// format string is somewhere else, point to it in a note. +void pr9751() { + int *i; + const char kFormat1[] = "%00d"; // expected-note{{format string is defined here}}} + scanf(kFormat1, i); // expected-warning{{zero field width in scanf format string is unused}} + scanf("%00d", i); // expected-warning{{zero field width in scanf format string is unused}} + const char kFormat2[] = "%["; // expected-note{{format string is defined here}}} + scanf(kFormat2, &i); // expected-warning{{no closing ']' for '%[' in scanf format string}} + scanf("%[", &i); // expected-warning{{no closing ']' for '%[' in scanf format string}} +} diff --git a/clang/test/Sema/format-strings.c b/clang/test/Sema/format-strings.c index 4f2ad99a5727..c4bbc836d458 100644 --- a/clang/test/Sema/format-strings.c +++ b/clang/test/Sema/format-strings.c @@ -393,3 +393,79 @@ void test_suppress_invalid_specifier() { #pragma clang diagnostic pop } +// Make sure warnings are on for next test. +#pragma GCC diagnostic warning "-Wformat" +#pragma GCC diagnostic warning "-Wformat-security" + +// Test that the printf call site is where the warning is attached. If the +// format string is somewhere else, point to it in a note. +void pr9751() { + const char kFormat1[] = "%d %d \n"; // expected-note{{format string is defined here}}} + printf(kFormat1, 0); // expected-warning{{more '%' conversions than data arguments}} + printf("%d %s\n", 0); // expected-warning{{more '%' conversions than data arguments}} + + const char kFormat2[] = "%18$s\n"; // expected-note{{format string is defined here}} + printf(kFormat2, 1, "foo"); // expected-warning{{data argument position '18' exceeds the number of data arguments (2)}} + printf("%18$s\n", 1, "foo"); // expected-warning{{data argument position '18' exceeds the number of data arguments (2)}} + + const char kFormat3[] = "%n"; // expected-note{{format string is defined here}} + printf(kFormat3, "as"); // expected-warning{{use of '%n' in format string discouraged}} + printf("%n", "as"); // expected-warning{{use of '%n' in format string discouraged}} + + const char kFormat4[] = "%y"; // expected-note{{format string is defined here}} + printf(kFormat4, 5); // expected-warning{{invalid conversion specifier 'y'}} + printf("%y", 5); // expected-warning{{invalid conversion specifier 'y'}} + + const char kFormat5[] = "%."; // expected-note{{format string is defined here}} + printf(kFormat5, 5); // expected-warning{{incomplete format specifier}} + printf("%.", 5); // expected-warning{{incomplete format specifier}} + + const char kFormat6[] = "%s"; // expected-note{{format string is defined here}} + printf(kFormat6, 5); // expected-warning{{conversion specifies type 'char *' but the argument has type 'int'}} + printf("%s", 5); // expected-warning{{conversion specifies type 'char *' but the argument has type 'int'}} + + const char kFormat7[] = "%0$"; // expected-note{{format string is defined here}} + printf(kFormat7, 5); // expected-warning{{position arguments in format strings start counting at 1 (not 0)}} + printf("%0$", 5); // expected-warning{{position arguments in format strings start counting at 1 (not 0)}} + + const char kFormat8[] = "%1$d %d"; // expected-note{{format string is defined here}} + printf(kFormat8, 4, 4); // expected-warning{{cannot mix positional and non-positional arguments in format string}} + printf("%1$d %d", 4, 4); // expected-warning{{cannot mix positional and non-positional arguments in format string}} + + const char kFormat9[] = ""; // expected-note{{format string is defined here}} + printf(kFormat9, 4, 4); // expected-warning{{format string is empty}} + printf("", 4, 4); // expected-warning{{format string is empty}} + + const char kFormat10[] = "\0%d"; // expected-note{{format string is defined here}} + printf(kFormat10, 4); // expected-warning{{format string contains '\0' within the string body}} + printf("\0%d", 4); // expected-warning{{format string contains '\0' within the string body}} + + const char kFormat11[] = "%*d"; // expected-note{{format string is defined here}} + printf(kFormat11); // expected-warning{{'*' specified field width is missing a matching 'int' argument}} + printf("%*d"); // expected-warning{{'*' specified field width is missing a matching 'int' argument}} + + const char kFormat12[] = "%*d"; // expected-note{{format string is defined here}} + printf(kFormat12, 4.4); // expected-warning{{field width should have type 'int', but argument has type 'double'}} + printf("%*d", 4.4); // expected-warning{{field width should have type 'int', but argument has type 'double'}} + + const char kFormat13[] = "%.3p"; // expected-note{{format string is defined here}} + void *p; + printf(kFormat13, p); // expected-warning{{precision used with 'p' conversion specifier, resulting in undefined behavior}} + printf("%.3p", p); // expected-warning{{precision used with 'p' conversion specifier, resulting in undefined behavior}} + + const char kFormat14[] = "%0s"; // expected-note{{format string is defined here}} + printf(kFormat14, "a"); // expected-warning{{flag '0' results in undefined behavior with 's' conversion specifier}} + printf("%0s", "a"); // expected-warning{{flag '0' results in undefined behavior with 's' conversion specifier}} + + const char kFormat15[] = "%hhs"; // expected-note{{format string is defined here}} + printf(kFormat15, "a"); // expected-warning{{length modifier 'hh' results in undefined behavior or no effect with 's' conversion specifier}} + printf("%hhs", "a"); // expected-warning{{length modifier 'hh' results in undefined behavior or no effect with 's' conversion specifier}} + + const char kFormat16[] = "%-0d"; // expected-note{{format string is defined here}} + printf(kFormat16, 5); // expected-warning{{flag '0' is ignored when flag '-' is present}} + printf("%-0d", 5); // expected-warning{{flag '0' is ignored when flag '-' is present}} + + // Make sure that the "format string is defined here" note is not emitted + // when the original string is within the argument expression. + printf(1 ? "yes %d" : "no %d"); // expected-warning 2{{more '%' conversions than data arguments}} +}