I'll take a look. ________________________________ From: Nico Weber <tha...@chromium.org> Sent: 17 May 2019 15:09:18 To: Kristof Umann Cc: cfe-commits Subject: Re: r361011 - [analyzer] Validate checker option names and values
It looks like this change is making gcc-7 crash on these (and other http://lab.llvm.org:8011/console) bots: http://lab.llvm.org:8011/builders/clang-cmake-aarch64-quick/builds/18639 http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/33837/ [100/212] Building CXX object tools/clang/unittests/AST/CMakeFiles/ASTTests.dir/ASTImporterTest.cpp.o FAILED: tools/clang/unittests/AST/CMakeFiles/ASTTests.dir/ASTImporterTest.cpp.o ... g++-7: internal compiler error: Killed (program cc1plus) Please submit a full bug report, with preprocessed source if appropriate. See <file:///usr/share/doc/gcc-7/README.Bugs> for instructions. [101/212] Building CXX object tools/clang/unittests/ASTMatchers/CMakeFiles/ASTMatchersTests.dir/ASTMatchersTraversalTest.cpp.o FAILED: tools/clang/unittests/ASTMatchers/CMakeFiles/ASTMatchersTests.dir/ASTMatchersTraversalTest.cpp.o ... g++-7: internal compiler error: Killed (program cc1plus) Please submit a full bug report, with preprocessed source if appropriate. See <file:///usr/share/doc/gcc-7/README.Bugs> for instructions. [102/212] Building CXX object tools/clang/unittests/ASTMatchers/CMakeFiles/ASTMatchersTests.dir/ASTMatchersNarrowingTest.cpp.o FAILED: tools/clang/unittests/ASTMatchers/CMakeFiles/ASTMatchersTests.dir/ASTMatchersNarrowingTest.cpp.o ... g++-7: internal compiler error: Killed (program cc1plus) Please submit a full bug report, with preprocessed source if appropriate. See <file:///usr/share/doc/gcc-7/README.Bugs> for instructions. [103/212] Building CXX object tools/clang/unittests/ASTMatchers/CMakeFiles/ASTMatchersTests.dir/ASTMatchersNodeTest.cpp.o FAILED: tools/clang/unittests/ASTMatchers/CMakeFiles/ASTMatchersTests.dir/ASTMatchersNodeTest.cpp.o ... g++-7: internal compiler error: Killed (program cc1plus) Please submit a full bug report, with preprocessed source if appropriate. See <file:///usr/share/doc/gcc-7/README.Bugs> for instructions. [104/212] Building CXX object tools/clang/unittests/Tooling/CMakeFiles/ToolingTests.dir/CommentHandlerTest.cpp.o FAILED: tools/clang/unittests/Tooling/CMakeFiles/ToolingTests.dir/CommentHandlerTest.cpp.o ... g++-7: internal compiler error: Killed (program cc1plus) Please submit a full bug report, with preprocessed source if appropriate. See <file:///usr/share/doc/gcc-7/README.Bugs> for instructions. [105/212] Building CXX object tools/clang/unittests/Tooling/CMakeFiles/ToolingTests.dir/ExecutionTest.cpp.o FAILED: tools/clang/unittests/Tooling/CMakeFiles/ToolingTests.dir/ExecutionTest.cpp.o ... g++-7: internal compiler error: Killed (program cc1plus) Please submit a full bug report, with preprocessed source if appropriate. See <file:///usr/share/doc/gcc-7/README.Bugs> for instructions. [106/212] Building CXX object tools/clang/unittests/AST/CMakeFiles/ASTTests.dir/ASTImporterVisibilityTest.cpp.o FAILED: tools/clang/unittests/AST/CMakeFiles/ASTTests.dir/ASTImporterVisibilityTest.cpp.o ... g++-7: internal compiler error: Killed (program cc1plus) Please submit a full bug report, with preprocessed source if appropriate. See <file:///usr/share/doc/gcc-7/README.Bugs> for instructions. From: Kristof Umann via cfe-commits <cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>> Date: Fri, May 17, 2019 at 5:49 AM To: <cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>> Author: szelethus Date: Fri May 17 02:51:59 2019 New Revision: 361011 URL: http://llvm.org/viewvc/llvm-project?rev=361011&view=rev Log: [analyzer] Validate checker option names and values Validate whether the option exists, and also whether the supplied value is of the correct type. With this patch, invoking the analyzer should be, at least in the frontend mode, a lot safer. Differential Revision: https://reviews.llvm.org/D57860 Modified: cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td cfe/trunk/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h cfe/trunk/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp cfe/trunk/test/Analysis/checker-plugins.c cfe/trunk/test/Analysis/invalid-checker-option.c Modified: cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td?rev=361011&r1=361010&r2=361011&view=diff ============================================================================== --- cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td (original) +++ cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td Fri May 17 02:51:59 2019 @@ -307,6 +307,8 @@ def err_analyzer_config_multiple_values def err_analyzer_config_invalid_input : Error< "invalid input for analyzer-config option '%0', that expects %1 value">; def err_analyzer_config_unknown : Error<"unknown analyzer-config '%0'">; +def err_analyzer_checker_option_unknown : Error< + "checker '%0' has no option called '%1'">; def err_analyzer_checker_option_invalid_input : Error< "invalid input for checker option '%0', that expects %1">; Modified: cfe/trunk/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h?rev=361011&r1=361010&r2=361011&view=diff ============================================================================== --- cfe/trunk/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h (original) +++ cfe/trunk/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h Fri May 17 02:51:59 2019 @@ -107,6 +107,17 @@ public: assert((OptionType == "bool" || OptionType == "string" || OptionType == "int") && "Unknown command line option type!"); + + assert((OptionType != "bool" || + (DefaultValStr == "true" || DefaultValStr == "false")) && + "Invalid value for boolean command line option! Maybe incorrect " + "parameters to the addCheckerOption or addPackageOption method?"); + + int Tmp; + assert((OptionType != "int" || !DefaultValStr.getAsInteger(0, Tmp)) && + "Invalid value for integer command line option! Maybe incorrect " + "parameters to the addCheckerOption or addPackageOption method?"); + (void)Tmp; } }; @@ -150,6 +161,12 @@ public: return State == StateFromCmdLine::State_Disabled && ShouldRegister(LO); } + // Since each checker must have a different full name, we can identify + // CheckerInfo objects by them. + bool operator==(const CheckerInfo &Rhs) const { + return FullName == Rhs.FullName; + } + CheckerInfo(InitializationFunction Fn, ShouldRegisterFunction sfn, StringRef Name, StringRef Desc, StringRef DocsUri, bool IsHidden) Modified: cfe/trunk/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp?rev=361011&r1=361010&r2=361011&view=diff ============================================================================== --- cfe/trunk/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp (original) +++ cfe/trunk/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp Fri May 17 02:51:59 2019 @@ -9,6 +9,7 @@ #include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h" #include "clang/Basic/Diagnostic.h" #include "clang/Basic/LLVM.h" +#include "clang/Driver/DriverDiagnostic.h" #include "clang/Frontend/FrontendDiagnostic.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h" @@ -306,18 +307,61 @@ void CheckerRegistry::addDependency(Stri Dependencies.emplace_back(FullName, Dependency); } +/// Insert the checker/package option to AnalyzerOptions' config table, and +/// validate it, if the user supplied it on the command line. +static void insertAndValidate(StringRef FullName, + const CheckerRegistry::CmdLineOption &Option, + AnalyzerOptions &AnOpts, + DiagnosticsEngine &Diags) { + + std::string FullOption = (FullName + ":" + Option.OptionName).str(); + + auto It = AnOpts.Config.insert({FullOption, Option.DefaultValStr}); + + // Insertation was successful -- CmdLineOption's constructor will validate + // whether values received from plugins or TableGen files are correct. + if (It.second) + return; + + // Insertion failed, the user supplied this package/checker option on the + // command line. If the supplied value is invalid, we'll emit an error. + + StringRef SuppliedValue = It.first->getValue(); + + if (Option.OptionType == "bool") { + if (SuppliedValue != "true" && SuppliedValue != "false") { + if (AnOpts.ShouldEmitErrorsOnInvalidConfigValue) { + Diags.Report(diag::err_analyzer_checker_option_invalid_input) + << FullOption << "a boolean value"; + } + } + return; + } + + if (Option.OptionType == "int") { + int Tmp; + bool HasFailed = SuppliedValue.getAsInteger(0, Tmp); + if (HasFailed) { + if (AnOpts.ShouldEmitErrorsOnInvalidConfigValue) { + Diags.Report(diag::err_analyzer_checker_option_invalid_input) + << FullOption << "an integer value"; + } + } + return; + } +} + template <class T> static void insertOptionToCollection(StringRef FullName, T &Collection, const CheckerRegistry::CmdLineOption &Option, - AnalyzerOptions &AnOpts) { + AnalyzerOptions &AnOpts, DiagnosticsEngine &Diags) { auto It = binaryFind(Collection, FullName); assert(It != Collection.end() && "Failed to find the checker while attempting to add a command line " "option to it!"); - AnOpts.Config.insert( - {(FullName + ":" + Option.OptionName).str(), Option.DefaultValStr}); + insertAndValidate(FullName, Option, AnOpts, Diags); It->CmdLineOptions.emplace_back(Option); } @@ -326,14 +370,14 @@ void CheckerRegistry::resolveCheckerAndP for (const std::pair<StringRef, CmdLineOption> &CheckerOptEntry : CheckerOptions) { insertOptionToCollection(CheckerOptEntry.first, Checkers, - CheckerOptEntry.second, AnOpts); + CheckerOptEntry.second, AnOpts, Diags); } CheckerOptions.clear(); for (const std::pair<StringRef, CmdLineOption> &PackageOptEntry : PackageOptions) { - insertOptionToCollection(PackageOptEntry.first, Checkers, - PackageOptEntry.second, AnOpts); + insertOptionToCollection(PackageOptEntry.first, Packages, + PackageOptEntry.second, AnOpts, Diags); } PackageOptions.clear(); } @@ -388,23 +432,62 @@ void CheckerRegistry::initializeManager( } } +static void +isOptionContainedIn(const CheckerRegistry::CmdLineOptionList &OptionList, + StringRef SuppliedChecker, StringRef SuppliedOption, + const AnalyzerOptions &AnOpts, DiagnosticsEngine &Diags) { + + if (!AnOpts.ShouldEmitErrorsOnInvalidConfigValue) + return; + + using CmdLineOption = CheckerRegistry::CmdLineOption; + + auto SameOptName = [SuppliedOption](const CmdLineOption &Opt) { + return Opt.OptionName == SuppliedOption; + }; + + auto OptionIt = llvm::find_if(OptionList, SameOptName); + + if (OptionIt == OptionList.end()) { + Diags.Report(diag::err_analyzer_checker_option_unknown) + << SuppliedChecker << SuppliedOption; + return; + } +} + void CheckerRegistry::validateCheckerOptions() const { for (const auto &Config : AnOpts.Config) { - size_t Pos = Config.getKey().find(':'); - if (Pos == StringRef::npos) + + StringRef SuppliedChecker; + StringRef SuppliedOption; + std::tie(SuppliedChecker, SuppliedOption) = Config.getKey().split(':'); + + if (SuppliedOption.empty()) continue; - bool HasChecker = false; - StringRef CheckerName = Config.getKey().substr(0, Pos); - for (const auto &Checker : Checkers) { - if (Checker.FullName.startswith(CheckerName) && - (Checker.FullName.size() == Pos || Checker.FullName[Pos] == '.')) { - HasChecker = true; - break; - } + // AnalyzerOptions' config table contains the user input, so an entry could + // look like this: + // + // cor:NoFalsePositives=true + // + // Since lower_bound would look for the first element *not less* than "cor", + // it would return with an iterator to the first checker in the core, so we + // we really have to use find here, which uses operator==. + auto CheckerIt = llvm::find(Checkers, CheckerInfo(SuppliedChecker)); + if (CheckerIt != Checkers.end()) { + isOptionContainedIn(CheckerIt->CmdLineOptions, SuppliedChecker, + SuppliedOption, AnOpts, Diags); + continue; + } + + auto PackageIt = llvm::find(Packages, PackageInfo(SuppliedChecker)); + if (PackageIt != Packages.end()) { + isOptionContainedIn(PackageIt->CmdLineOptions, SuppliedChecker, + SuppliedOption, AnOpts, Diags); + continue; } - if (!HasChecker) - Diags.Report(diag::err_unknown_analyzer_checker) << CheckerName; + + Diags.Report(diag::err_unknown_analyzer_checker) << SuppliedChecker; } } Modified: cfe/trunk/test/Analysis/checker-plugins.c URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/checker-plugins.c?rev=361011&r1=361010&r2=361011&view=diff ============================================================================== --- cfe/trunk/test/Analysis/checker-plugins.c (original) +++ cfe/trunk/test/Analysis/checker-plugins.c Fri May 17 02:51:59 2019 @@ -62,3 +62,35 @@ void caller() { // RUN: 2>&1 | FileCheck %s -check-prefix=CHECK-CHECKER-OPTION-TRUE // CHECK-CHECKER-OPTION-TRUE: example.MyChecker:ExampleOption = true + +// RUN: not %clang_analyze_cc1 -verify %s \ +// RUN: -load %llvmshlibdir/CheckerOptionHandlingAnalyzerPlugin%pluginext\ +// RUN: -analyzer-checker=example.MyChecker \ +// RUN: -analyzer-config example.MyChecker:Example=true \ +// RUN: 2>&1 | FileCheck %s -check-prefix=CHECK-NON-EXISTENT-CHECKER-OPTION + +// CHECK-NON-EXISTENT-CHECKER-OPTION: (frontend): checker 'example.MyChecker' +// CHECK-NON-EXISTENT-CHECKER-OPTION-SAME: has no option called 'Example' + +// RUN: not %clang_analyze_cc1 -verify %s \ +// RUN: -load %llvmshlibdir/CheckerOptionHandlingAnalyzerPlugin%pluginext\ +// RUN: -analyzer-checker=example.MyChecker \ +// RUN: -analyzer-config-compatibility-mode=true \ +// RUN: -analyzer-config example.MyChecker:Example=true + + +// RUN: not %clang_analyze_cc1 -verify %s \ +// RUN: -load %llvmshlibdir/CheckerOptionHandlingAnalyzerPlugin%pluginext\ +// RUN: -analyzer-checker=example.MyChecker \ +// RUN: -analyzer-config example.MyChecker:ExampleOption=example \ +// RUN: 2>&1 | FileCheck %s -check-prefix=CHECK-INVALID-BOOL-VALUE + +// CHECK-INVALID-BOOL-VALUE: (frontend): invalid input for checker option +// CHECK-INVALID-BOOL-VALUE-SAME: 'example.MyChecker:ExampleOption', that +// CHECK-INVALID-BOOL-VALUE-SAME: expects a boolean value + +// RUN: not %clang_analyze_cc1 -verify %s \ +// RUN: -load %llvmshlibdir/CheckerOptionHandlingAnalyzerPlugin%pluginext\ +// RUN: -analyzer-checker=example.MyChecker \ +// RUN: -analyzer-config-compatibility-mode=true \ +// RUN: -analyzer-config example.MyChecker:ExampleOption=example Modified: cfe/trunk/test/Analysis/invalid-checker-option.c URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/invalid-checker-option.c?rev=361011&r1=361010&r2=361011&view=diff ============================================================================== --- cfe/trunk/test/Analysis/invalid-checker-option.c (original) +++ cfe/trunk/test/Analysis/invalid-checker-option.c Fri May 17 02:51:59 2019 @@ -14,6 +14,63 @@ // CHECK-NON-EXISTENT-CHECKER: (frontend): no analyzer checkers or packages // CHECK-NON-EXISTENT-CHECKER-SAME: are associated with 'RetainOneTwoThree' + +// Every other error should be avoidable in compatiblity mode. + + +// RUN: not %clang_analyze_cc1 -verify %s \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-config debug.AnalysisOrder:Everything=false \ +// RUN: 2>&1 | FileCheck %s -check-prefix=CHECK-NON-EXISTENT-CHECKER-OPTION + +// CHECK-NON-EXISTENT-CHECKER-OPTION: (frontend): checker 'debug.AnalysisOrder' +// CHECK-NON-EXISTENT-CHECKER-OPTION-SAME: has no option called 'Everything' + +// RUN: %clang_analyze_cc1 -verify %s \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-config-compatibility-mode=true \ +// RUN: -analyzer-config debug.AnalysisOrder:Everything=false + + +// RUN: not %clang_analyze_cc1 -verify %s \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-config debug.AnalysisOrder:*=nothankyou \ +// RUN: 2>&1 | FileCheck %s -check-prefix=CHECK-INVALID-BOOL-VALUE + +// CHECK-INVALID-BOOL-VALUE: (frontend): invalid input for checker option +// CHECK-INVALID-BOOL-VALUE-SAME: 'debug.AnalysisOrder:*', that expects a +// CHECK-INVALID-BOOL-VALUE-SAME: boolean value + +// RUN: %clang_analyze_cc1 -verify %s \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-config-compatibility-mode=true \ +// RUN: -analyzer-config debug.AnalysisOrder:*=nothankyou + + +// RUN: not %clang_analyze_cc1 -verify %s \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-config optin.performance.Padding:AllowedPad=surpriseme \ +// RUN: 2>&1 | FileCheck %s -check-prefix=CHECK-INVALID-INT-VALUE + +// CHECK-INVALID-INT-VALUE: (frontend): invalid input for checker option +// CHECK-INVALID-INT-VALUE-SAME: 'optin.performance.Padding:AllowedPad', that +// CHECK-INVALID-INT-VALUE-SAME: expects an integer value + +// RUN: %clang_analyze_cc1 -verify %s \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-config-compatibility-mode=true \ +// RUN: -analyzer-config optin.performance.Padding:AllowedPad=surpriseme + + +// RUN: not %clang_analyze_cc1 -verify %s \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-config nullability:NoDiagnoseCallsToSystemHeaders=sure \ +// RUN: 2>&1 | FileCheck %s -check-prefix=CHECK-PACKAGE-VALUE + +// CHECK-PACKAGE-VALUE: (frontend): invalid input for checker option +// CHECK-PACKAGE-VALUE-SAME: 'nullability:NoDiagnoseCallsToSystemHeaders', that +// CHECK-PACKAGE-VALUE-SAME: expects a boolean value + // expected-no-diagnostics int main() {} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
