From d23d3edf356d787bf989594312c7b18207fb414d Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Tue, 13 Feb 2024 11:05:36 -0800 Subject: [PATCH 1/5] Extract customizations into own module --- .../cpp/exceptions/ExceptionFlow.qll | 67 +----------------- .../ExceptionFlowCustomizations.qll | 68 +++++++++++++++++++ 2 files changed, 69 insertions(+), 66 deletions(-) create mode 100644 cpp/common/src/codingstandards/cpp/exceptions/ExceptionFlowCustomizations.qll diff --git a/cpp/common/src/codingstandards/cpp/exceptions/ExceptionFlow.qll b/cpp/common/src/codingstandards/cpp/exceptions/ExceptionFlow.qll index 5a4e7fee6e..d62bc8c02a 100644 --- a/cpp/common/src/codingstandards/cpp/exceptions/ExceptionFlow.qll +++ b/cpp/common/src/codingstandards/cpp/exceptions/ExceptionFlow.qll @@ -5,6 +5,7 @@ import cpp import codingstandards.cpp.standardlibrary.Exceptions import codingstandards.cpp.exceptions.ExceptionSpecifications +import codingstandards.cpp.exceptions.ExceptionFlowCustomizations import ThirdPartyExceptions /* @@ -271,72 +272,6 @@ ExceptionType getAFunctionThrownType(Function f, ThrowingExpr throwingExpr) { ) } -/** A `ThrowingExpr` which is the origin of a exceptions in the program. */ -abstract class OriginThrowingExpr extends ThrowingExpr { } - -/** An expression which directly throws. */ -class DirectThrowExprThrowingExpr extends DirectThrowExpr, OriginThrowingExpr { - override ExceptionType getAnExceptionType() { result = getExceptionType() } -} - -/** An `typeid` expression which may throw `std::bad_typeid`. */ -class TypeIdThrowingExpr extends TypeidOperator, OriginThrowingExpr { - override ExceptionType getAnExceptionType() { result instanceof StdBadTypeId } -} - -/** An `new[]` expression which may throw `std::bad_array_new_length`. */ -class NewThrowingExpr extends NewArrayExpr, OriginThrowingExpr { - NewThrowingExpr() { - // If the extent is known to be below 0 at runtime - getExtent().getValue().toInt() < 0 - or - // initializer has more elements than the array size - getExtent().getValue().toInt() < getInitializer().(ArrayAggregateLiteral).getArraySize() - } - - override ExceptionType getAnExceptionType() { result instanceof StdBadArrayNewLength } -} - -/** A `ReThrowExpr` which throws a previously caught exception. */ -class ReThrowExprThrowingExpr extends ReThrowExpr, ThrowingExpr { - predicate rethrows(CatchBlock cb, ExceptionType et, ThrowingExpr te) { - // Find the nearest CatchBlock - cb = getNearestCatch(this.getEnclosingStmt()) and - // Find an `ExceptionType` which is caught by this catch block, and `ThrowingExpr` which throws that exception type - catches(cb, te, et) - } - - override ExceptionType getAnExceptionType() { rethrows(_, result, _) } - - CatchBlock getCatchBlock() { rethrows(result, _, _) } -} - -/** An expression which calls a function which may throw an exception. */ -class FunctionCallThrowingExpr extends FunctionCall, ThrowingExpr { - override ExceptionType getAnExceptionType() { - exists(Function target | - target = getTarget() and - result = getAFunctionThrownType(target, _) and - // [expect.spec] states that throwing an exception type that is prohibited - // by the specification will result in the program terminating, unless - // a custom `unexpected_handler` is registered that throws an exception type - // which is compatible with the dynamic exception specification, or the - // dynamic exception specification lists `std::bad_exception`, in which case - // a `std::bad_exception` is thrown. - // As dynamic exception specifications and the `unexpected_handler` are both - // deprecated in C++14 and removed in C++17, we assume a default - // `std::unexpected` handler that calls `std::terminate` and therefore - // do not propagate such exceptions to the call sites for the function. - not ( - hasDynamicExceptionSpecification(target) and - not result = getAHandledExceptionType(target.getAThrownType()) - or - isNoExceptTrue(target) - ) - ) - } -} - module ExceptionPathGraph { /** * A function for which we want path information. diff --git a/cpp/common/src/codingstandards/cpp/exceptions/ExceptionFlowCustomizations.qll b/cpp/common/src/codingstandards/cpp/exceptions/ExceptionFlowCustomizations.qll new file mode 100644 index 0000000000..f8e9a02f7a --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/exceptions/ExceptionFlowCustomizations.qll @@ -0,0 +1,68 @@ +import cpp +private import codingstandards.cpp.exceptions.ExceptionFlow + +/** A `ThrowingExpr` which is the origin of a exceptions in the program. */ +abstract class OriginThrowingExpr extends ThrowingExpr { } + +/** An expression which directly throws. */ +class DirectThrowExprThrowingExpr extends DirectThrowExpr, OriginThrowingExpr { + override ExceptionType getAnExceptionType() { result = getExceptionType() } +} + +/** A `ReThrowExpr` which throws a previously caught exception. */ +class ReThrowExprThrowingExpr extends ReThrowExpr, ThrowingExpr { + predicate rethrows(CatchBlock cb, ExceptionType et, ThrowingExpr te) { + // Find the nearest CatchBlock + cb = getNearestCatch(this.getEnclosingStmt()) and + // Find an `ExceptionType` which is caught by this catch block, and `ThrowingExpr` which throws that exception type + catches(cb, te, et) + } + + override ExceptionType getAnExceptionType() { rethrows(_, result, _) } + + CatchBlock getCatchBlock() { rethrows(result, _, _) } +} + +/** An expression which calls a function which may throw an exception. */ +class FunctionCallThrowingExpr extends FunctionCall, ThrowingExpr { + override ExceptionType getAnExceptionType() { + exists(Function target | + target = getTarget() and + result = getAFunctionThrownType(target, _) and + // [expect.spec] states that throwing an exception type that is prohibited + // by the specification will result in the program terminating, unless + // a custom `unexpected_handler` is registered that throws an exception type + // which is compatible with the dynamic exception specification, or the + // dynamic exception specification lists `std::bad_exception`, in which case + // a `std::bad_exception` is thrown. + // As dynamic exception specifications and the `unexpected_handler` are both + // deprecated in C++14 and removed in C++17, we assume a default + // `std::unexpected` handler that calls `std::terminate` and therefore + // do not propagate such exceptions to the call sites for the function. + not ( + hasDynamicExceptionSpecification(target) and + not result = getAHandledExceptionType(target.getAThrownType()) + or + isNoExceptTrue(target) + ) + ) + } +} + +/** An `typeid` expression which may throw `std::bad_typeid`. */ +private class TypeIdThrowingExpr extends TypeidOperator, OriginThrowingExpr { + override ExceptionType getAnExceptionType() { result instanceof StdBadTypeId } +} + +/** An `new[]` expression which may throw `std::bad_array_new_length`. */ +private class NewThrowingExpr extends NewArrayExpr, OriginThrowingExpr { + NewThrowingExpr() { + // If the extent is known to be below 0 at runtime + getExtent().getValue().toInt() < 0 + or + // initializer has more elements than the array size + getExtent().getValue().toInt() < getInitializer().(ArrayAggregateLiteral).getArraySize() + } + + override ExceptionType getAnExceptionType() { result instanceof StdBadArrayNewLength } +} From 476e91084288a735f4e5b9c71a5a7014b2b52443 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Tue, 13 Feb 2024 12:26:35 -0800 Subject: [PATCH 2/5] Add test case of FP --- cpp/autosar/test/rules/A15-4-4/test.cpp | 11 +++++++++++ cpp/common/test/includes/standard-library/stdexcept.h | 2 ++ cpp/common/test/includes/standard-library/string | 2 ++ 3 files changed, 15 insertions(+) diff --git a/cpp/autosar/test/rules/A15-4-4/test.cpp b/cpp/autosar/test/rules/A15-4-4/test.cpp index 7d8597a75f..2ca6a32e0c 100644 --- a/cpp/autosar/test/rules/A15-4-4/test.cpp +++ b/cpp/autosar/test/rules/A15-4-4/test.cpp @@ -43,4 +43,15 @@ void test_swap_wrapper() noexcept { int a = 0; int b = 1; swap_wrapper(&a, &b); +} + +#include +#include + +std::string test_fp_reported_in_424(const std::string &s1, const std::string &s2) { + std::string s3; + s3.reserve(s1.size() + s2.size()); + s3.append(s1.c_str(), s1.size()); + s3.append(s2.c_str(), s2.size()); + return s3; } \ No newline at end of file diff --git a/cpp/common/test/includes/standard-library/stdexcept.h b/cpp/common/test/includes/standard-library/stdexcept.h index cb9af14db2..d341738bfc 100644 --- a/cpp/common/test/includes/standard-library/stdexcept.h +++ b/cpp/common/test/includes/standard-library/stdexcept.h @@ -28,5 +28,7 @@ class nested_exception { template [[noreturn]] void throw_with_nested(T &&t); template void rethrow_if_nested(E const &e); +class length_error : public logic_error{}; +class out_of_range: public logic_error{}; } // namespace std #endif // _GHLIBCPP_STDEXCEPT \ No newline at end of file diff --git a/cpp/common/test/includes/standard-library/string b/cpp/common/test/includes/standard-library/string index ca267f6191..a3f22f5e80 100644 --- a/cpp/common/test/includes/standard-library/string +++ b/cpp/common/test/includes/standard-library/string @@ -166,6 +166,8 @@ public: int compare(const charT *s) const; int compare(size_type pos1, size_type n1, const charT *s) const; int compare(size_type pos1, size_type n1, const charT *s, size_type n2) const; + + void reserve(size_type new_cap = 0); }; template From 1c1f6307951850fb5218d93024348fd5a47985ad Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Tue, 13 Feb 2024 12:27:28 -0800 Subject: [PATCH 3/5] Add model of external functions that may throw This allows us to extend the queries reasoning about exceptions that maybe thrown with information that is not directly retrievable from the source. For example, standard library functions known to throw exceptions that are not specified in their signatures. --- .../ExceptionFlowCustomizations.qll | 45 +++++++++++++++++++ cpp/common/src/ext/stdc++.model.yml | 7 +++ cpp/common/src/qlpack.yml | 2 + 3 files changed, 54 insertions(+) create mode 100644 cpp/common/src/ext/stdc++.model.yml diff --git a/cpp/common/src/codingstandards/cpp/exceptions/ExceptionFlowCustomizations.qll b/cpp/common/src/codingstandards/cpp/exceptions/ExceptionFlowCustomizations.qll index f8e9a02f7a..90f67c3075 100644 --- a/cpp/common/src/codingstandards/cpp/exceptions/ExceptionFlowCustomizations.qll +++ b/cpp/common/src/codingstandards/cpp/exceptions/ExceptionFlowCustomizations.qll @@ -1,9 +1,50 @@ +/* + * A library customize models that model the flow of exceptions through the program. + */ + import cpp private import codingstandards.cpp.exceptions.ExceptionFlow /** A `ThrowingExpr` which is the origin of a exceptions in the program. */ abstract class OriginThrowingExpr extends ThrowingExpr { } +/** + * A `FunctionCall` to an external function without an exception specification that * + * may throw an exception. + */ +abstract class ExternalUnderspecifiedFunctionCallThrowingExpr extends FunctionCall, ThrowingExpr { } + +/** + * An extensible predicate that describes functions that when called may throw an exception. + */ +extensible predicate throwingFunctionModel( + string functionNamespaceQualifier, string functionTypeQualifier, string functionName, + string exceptionNamespaceQualifier, string exceptionType +); + +/** + * A `FunctionCall` that may throw an exception of type `ExceptionType` as provded by + * the extensible predicate `throwingFunctionModel`. + */ +private class ExternalFunctionCallThrowingExpr extends FunctionCall, ThrowingExpr { + ExceptionType exceptionType; + + ExternalFunctionCallThrowingExpr() { + exists( + string functionNamespaceQualifier, string functionTypeQualifier, string functionName, + string exceptionNamespaceQualifier, string exceptionTypeSpec + | + throwingFunctionModel(functionNamespaceQualifier, functionTypeQualifier, functionName, + exceptionNamespaceQualifier, exceptionTypeSpec) and + this.getTarget() + .hasQualifiedName(functionNamespaceQualifier, functionTypeQualifier, functionName) and + exceptionType.(Class).hasQualifiedName(exceptionNamespaceQualifier, exceptionTypeSpec) + ) + } + + override ExceptionType getAnExceptionType() { result = exceptionType } +} + /** An expression which directly throws. */ class DirectThrowExprThrowingExpr extends DirectThrowExpr, OriginThrowingExpr { override ExceptionType getAnExceptionType() { result = getExceptionType() } @@ -46,6 +87,10 @@ class FunctionCallThrowingExpr extends FunctionCall, ThrowingExpr { isNoExceptTrue(target) ) ) + or + result = this.(ExternalUnderspecifiedFunctionCallThrowingExpr).getAnExceptionType() + or + result = this.(ExternalFunctionCallThrowingExpr).getAnExceptionType() } } diff --git a/cpp/common/src/ext/stdc++.model.yml b/cpp/common/src/ext/stdc++.model.yml new file mode 100644 index 0000000000..37919dceea --- /dev/null +++ b/cpp/common/src/ext/stdc++.model.yml @@ -0,0 +1,7 @@ +extensions: + - addsTo: + pack: codeql/common-cpp-coding-standards + extensible: throwingFunctionModel + data: + - ["std", "basic_string", "append", "std", "out_of_range"] + - ["std", "basic_string", "reserve", "std", "length_error"] \ No newline at end of file diff --git a/cpp/common/src/qlpack.yml b/cpp/common/src/qlpack.yml index ebb90b8fa1..bb698cc9b0 100644 --- a/cpp/common/src/qlpack.yml +++ b/cpp/common/src/qlpack.yml @@ -3,3 +3,5 @@ version: 2.22.0-dev license: MIT dependencies: codeql/cpp-all: 0.9.3 +dataExtensions: + - ext/*.model.yml \ No newline at end of file From 3413966ce8ee2961a368830b9c364a5bda5ff194 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Tue, 13 Feb 2024 12:38:59 -0800 Subject: [PATCH 4/5] Add changenote --- change_notes/2024-02-13-fix-fp-a15-4-4.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 change_notes/2024-02-13-fix-fp-a15-4-4.md diff --git a/change_notes/2024-02-13-fix-fp-a15-4-4.md b/change_notes/2024-02-13-fix-fp-a15-4-4.md new file mode 100644 index 0000000000..1afb29fd6a --- /dev/null +++ b/change_notes/2024-02-13-fix-fp-a15-4-4.md @@ -0,0 +1,2 @@ +-`A15-4-4` - `MissingNoExcept.ql`: + - Fix FP reported in #424. Exclude functions calling `std::string::reserve` or `std::string::append` that may throw even if their signatures don't specify it. \ No newline at end of file From b3ff4522b9d99cb969e1e26c7b55126de125326d Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Tue, 13 Feb 2024 12:42:32 -0800 Subject: [PATCH 5/5] Add test case annotation --- cpp/autosar/test/rules/A15-4-4/test.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/cpp/autosar/test/rules/A15-4-4/test.cpp b/cpp/autosar/test/rules/A15-4-4/test.cpp index 2ca6a32e0c..1f9d0d5a85 100644 --- a/cpp/autosar/test/rules/A15-4-4/test.cpp +++ b/cpp/autosar/test/rules/A15-4-4/test.cpp @@ -45,10 +45,12 @@ void test_swap_wrapper() noexcept { swap_wrapper(&a, &b); } -#include #include +#include -std::string test_fp_reported_in_424(const std::string &s1, const std::string &s2) { +std::string test_fp_reported_in_424( + const std::string &s1, + const std::string &s2) { // COMPLIANT - `reserve` and `append` may throw. std::string s3; s3.reserve(s1.size() + s2.size()); s3.append(s1.c_str(), s1.size());