From 3f272b853f60472c71209ce48ecfb7509ca67426 Mon Sep 17 00:00:00 2001 From: Tom Care Date: Mon, 21 Jun 2010 21:21:01 +0000 Subject: [PATCH] Bug 7377: printf checking fails to flag some undefined behavior http://llvm.org/bugs/show_bug.cgi?id=7377 Updated format string highlighting and fixits to take advantage of the new CharSourceRange class. - Change HighlightRange to allow highlighting whitespace only in a CharSourceRange (for warnings about the ' ' (space) flag) - Change format specifier range helper function to allow for half-open ranges (+1 to end) - Enabled previously failing tests (FIXMEs/XFAILs removed) - Small fixes and additions to format string test cases M test/Sema/format-strings.c M test/Sema/format-strings-fixit.c M lib/Frontend/TextDiagnosticPrinter.cpp M lib/Sema/SemaChecking.cpp llvm-svn: 106480 --- clang/lib/Frontend/TextDiagnosticPrinter.cpp | 29 ++++++++++-------- clang/lib/Sema/SemaChecking.cpp | 31 ++++++++++---------- clang/test/Sema/format-strings-fixit.c | 8 ++--- clang/test/Sema/format-strings.c | 6 ++-- 4 files changed, 37 insertions(+), 37 deletions(-) diff --git a/clang/lib/Frontend/TextDiagnosticPrinter.cpp b/clang/lib/Frontend/TextDiagnosticPrinter.cpp index 3f1eb82f7efe..1b5b7e2ea863 100644 --- a/clang/lib/Frontend/TextDiagnosticPrinter.cpp +++ b/clang/lib/Frontend/TextDiagnosticPrinter.cpp @@ -123,21 +123,24 @@ void TextDiagnosticPrinter::HighlightRange(const CharSourceRange &R, assert(StartColNo <= EndColNo && "Invalid range!"); - // Pick the first non-whitespace column. - while (StartColNo < SourceLine.size() && - (SourceLine[StartColNo] == ' ' || SourceLine[StartColNo] == '\t')) - ++StartColNo; + // Check that a token range does not highlight only whitespace. + if (R.isTokenRange()) { + // Pick the first non-whitespace column. + while (StartColNo < SourceLine.size() && + (SourceLine[StartColNo] == ' ' || SourceLine[StartColNo] == '\t')) + ++StartColNo; - // Pick the last non-whitespace column. - if (EndColNo > SourceLine.size()) - EndColNo = SourceLine.size(); - while (EndColNo-1 && - (SourceLine[EndColNo-1] == ' ' || SourceLine[EndColNo-1] == '\t')) - --EndColNo; + // Pick the last non-whitespace column. + if (EndColNo > SourceLine.size()) + EndColNo = SourceLine.size(); + while (EndColNo-1 && + (SourceLine[EndColNo-1] == ' ' || SourceLine[EndColNo-1] == '\t')) + --EndColNo; - // If the start/end passed each other, then we are trying to highlight a range - // that just exists in whitespace, which must be some sort of other bug. - assert(StartColNo <= EndColNo && "Trying to highlight whitespace??"); + // If the start/end passed each other, then we are trying to highlight a range + // that just exists in whitespace, which must be some sort of other bug. + assert(StartColNo <= EndColNo && "Trying to highlight whitespace??"); + } // Fill the range with ~'s. for (unsigned i = StartColNo; i < EndColNo; ++i) diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp index 2dc2c22a4e9c..d9264870ae22 100644 --- a/clang/lib/Sema/SemaChecking.cpp +++ b/clang/lib/Sema/SemaChecking.cpp @@ -1194,8 +1194,8 @@ public: unsigned specifierLen); private: SourceRange getFormatStringRange(); - SourceRange getFormatSpecifierRange(const char *startSpecifier, - unsigned specifierLen); + CharSourceRange getFormatSpecifierRange(const char *startSpecifier, + unsigned specifierLen); SourceLocation getLocationOfByte(const char *x); bool HandleAmount(const analyze_printf::OptionalAmount &Amt, unsigned k, @@ -1220,10 +1220,15 @@ SourceRange CheckPrintfHandler::getFormatStringRange() { return OrigFormatExpr->getSourceRange(); } -SourceRange CheckPrintfHandler:: +CharSourceRange CheckPrintfHandler:: getFormatSpecifierRange(const char *startSpecifier, unsigned specifierLen) { - return SourceRange(getLocationOfByte(startSpecifier), - getLocationOfByte(startSpecifier+specifierLen-1)); + SourceLocation Start = getLocationOfByte(startSpecifier); + SourceLocation End = getLocationOfByte(startSpecifier + specifierLen - 1); + + // Advance the end SourceLocation by one due to half-open ranges. + End = End.getFileLocWithOffset(1); + + return CharSourceRange::getCharRange(Start, End); } SourceLocation CheckPrintfHandler::getLocationOfByte(const char *x) { @@ -1451,7 +1456,7 @@ CheckPrintfHandler::HandleFormatSpecifier(const analyze_printf::FormatSpecifier // Check for invalid use of field width if (!FS.hasValidFieldWidth()) { - HandleInvalidAmount(FS, FS.getFieldWidth(), /* field width */ 1, + HandleInvalidAmount(FS, FS.getFieldWidth(), /* field width */ 0, startSpecifier, specifierLen); } @@ -1466,21 +1471,17 @@ CheckPrintfHandler::HandleFormatSpecifier(const analyze_printf::FormatSpecifier HandleFlag(FS, FS.hasLeadingZeros(), startSpecifier, specifierLen); if (!FS.hasValidPlusPrefix()) HandleFlag(FS, FS.hasPlusPrefix(), startSpecifier, specifierLen); - // FIXME: the following lines are disabled due to clang assertions on - // highlights containing spaces. - // if (!FS.hasValidSpacePrefix()) - // HandleFlag(FS, FS.hasSpacePrefix(), startSpecifier, specifierLen); + if (!FS.hasValidSpacePrefix()) + HandleFlag(FS, FS.hasSpacePrefix(), startSpecifier, specifierLen); if (!FS.hasValidAlternativeForm()) HandleFlag(FS, FS.hasAlternativeForm(), startSpecifier, specifierLen); if (!FS.hasValidLeftJustified()) HandleFlag(FS, FS.isLeftJustified(), startSpecifier, specifierLen); // Check that flags are not ignored by another flag - // FIXME: the following lines are disabled due to clang assertions on - // highlights containing spaces. - //if (FS.hasSpacePrefix() && FS.hasPlusPrefix()) // ' ' ignored by '+' - // HandleIgnoredFlag(FS, FS.hasSpacePrefix(), FS.hasPlusPrefix(), - // startSpecifier, specifierLen); + if (FS.hasSpacePrefix() && FS.hasPlusPrefix()) // ' ' ignored by '+' + HandleIgnoredFlag(FS, FS.hasSpacePrefix(), FS.hasPlusPrefix(), + startSpecifier, specifierLen); if (FS.hasLeadingZeros() && FS.isLeftJustified()) // '0' ignored by '-' HandleIgnoredFlag(FS, FS.hasLeadingZeros(), FS.isLeftJustified(), startSpecifier, specifierLen); diff --git a/clang/test/Sema/format-strings-fixit.c b/clang/test/Sema/format-strings-fixit.c index f74ce4e81acf..7cd76c7c37e7 100644 --- a/clang/test/Sema/format-strings-fixit.c +++ b/clang/test/Sema/format-strings-fixit.c @@ -1,9 +1,6 @@ // RUN: cp %s %t // RUN: %clang_cc1 -pedantic -Wall -fixit %t || true // RUN: %clang_cc1 -fsyntax-only -pedantic -Wall -Werror %t -// XFAIL: * -// FIXME: Some of these tests currently fail due to a bug in the HighlightRange -// function in lib/Frontend/TextDiagnosticPrinter.cpp. /* This is a test of the various code modification hints that are provided as part of warning or extension diagnostics. All of the @@ -26,9 +23,10 @@ void test() { printf("%1d", (long double) 1.23); // Flag handling - printf("%0+s", (unsigned) 31337); // flags should stay - printf("%0f", "test"); // flag should be removed + printf("%0+s", (unsigned) 31337); // 0 flag should stay printf("%#p", (void *) 0); + printf("% +f", 1.23); // + flag should stay + printf("%0-f", 1.23); // - flag should stay // Positional arguments printf("%1$f:%2$.*3$f:%4$.*3$f\n", 1, 2, 3, 4); diff --git a/clang/test/Sema/format-strings.c b/clang/test/Sema/format-strings.c index 2f3947bc8f5d..c6dee6801e80 100644 --- a/clang/test/Sema/format-strings.c +++ b/clang/test/Sema/format-strings.c @@ -1,7 +1,4 @@ // RUN: %clang_cc1 -fsyntax-only -verify -Wformat-nonliteral %s -// XFAIL: * -// FIXME: temporarily marking as expected fail until whitespace highlighting -// is fixed. #include typedef __typeof(sizeof(int)) size_t; @@ -277,7 +274,8 @@ void bug7377_bad_length_mod_usage() { // Bad optional amount use printf("%.2c", 'a'); // expected-warning{{precision used with 'c' conversion specifier, resulting in undefined behavior}} - printf("%1n", (void *) 0); // expected-warning{{precision used with 'n' conversion specifier, resulting in undefined behavior}} expected-warning{{use of '%n' in format string discouraged (potentially insecure)}} + printf("%1n", (void *) 0); // expected-warning{{field width used with 'n' conversion specifier, resulting in undefined behavior}} expected-warning{{use of '%n' in format string discouraged (potentially insecure)}} + printf("%.9n", (void *) 0); // expected-warning{{precision used with 'n' conversion specifier, resulting in undefined behavior}} expected-warning{{use of '%n' in format string discouraged (potentially insecure)}} // Ignored flags printf("% +f", 1.23); // expected-warning{{flag ' ' is ignored when flag '+' is present}}