diff --git a/change_notes/2024-01-30-fix-fp-for-a8-4-8.md b/change_notes/2024-01-30-fix-fp-for-a8-4-8.md new file mode 100644 index 0000000000..a71f810b24 --- /dev/null +++ b/change_notes/2024-01-30-fix-fp-for-a8-4-8.md @@ -0,0 +1,7 @@ +- `A8-4-8` - `OutParametersUsed.ql` + - Fixes #370 - Non-member user-defined assignment operator and stream insertion/extraction parameters that are required to be out parameters are excluded. + - Broadens the definition of out parameter by considering assignment and crement operators as modifications to an out parameter candidate. +- `FIO51-CPP` - `CloseFilesWhenTheyAreNoLongerNeeded.ql`: + - Broadened definition of `IStream` and `OStream` types may result in reduced false negatives. +- `A5-1-1` - `LiteralValueUsedOutsideTypeInit.ql`: + - Broadened definition of `IStream` types may result in reduced false positives because more file stream function calls may be detected as logging operations that will be excluded from the results. diff --git a/cpp/autosar/src/rules/A8-4-8/OutputParametersUsed.ql b/cpp/autosar/src/rules/A8-4-8/OutputParametersUsed.ql index aa480a95f7..c2fc51fcdf 100644 --- a/cpp/autosar/src/rules/A8-4-8/OutputParametersUsed.ql +++ b/cpp/autosar/src/rules/A8-4-8/OutputParametersUsed.ql @@ -23,31 +23,60 @@ import codingstandards.cpp.ConstHelpers import codingstandards.cpp.Operator /** - * Non-const T& and T* `Parameter`s to `Function`s + * Holds if p is passed as a non-const reference or pointer and is modified. + * This holds for in-out or out-only parameters. */ -class NonConstReferenceOrPointerParameterCandidate extends FunctionParameter { - NonConstReferenceOrPointerParameterCandidate() { - this instanceof NonConstReferenceParameter - or - this instanceof NonConstPointerParameter - } +predicate isOutParameter(NonConstPointerorReferenceParameter p) { + any(VariableEffect ve).getTarget() = p +} + +/** + * Holds if parameter `p` is a parameter to a user defined assignment operator that + * is defined outside of a class body. + * These require an in-out parameter as the first argument. + */ +predicate isNonMemberUserAssignmentParameter(NonConstPointerorReferenceParameter p) { + p.getFunction() instanceof UserAssignmentOperator and + not p.isMember() +} + +/** + * Holds if parameter `p` is a parameter to a stream insertion operator that + * is defined outside of a class body. + * These require an in-out parameter as the first argument. + * + * e.g., `std::ostream& operator<<(std::ostream& os, const T& obj)` + */ +predicate isStreamInsertionStreamParameter(NonConstPointerorReferenceParameter p) { + exists(StreamInsertionOperator op | not op.isMember() | op.getParameter(0) = p) } -pragma[inline] -predicate isFirstAccess(VariableAccess va) { - not exists(VariableAccess otherVa | - otherVa.getTarget() = va.getTarget() or - otherVa.getQualifier().(VariableAccess).getTarget() = va.getTarget() - | - otherVa.getASuccessor() = va +/** + * Holds if parameter `p` is a parameter to a stream insertion operator that + * is defined outside of a class body. + * These require an in-out parameter as the first argument and an out parameter for the second. + * + * e.g., `std::istream& operator>>(std::istream& is, T& obj)` + */ +predicate isStreamExtractionParameter(NonConstPointerorReferenceParameter p) { + exists(StreamExtractionOperator op | not op.isMember() | + op.getParameter(0) = p + or + op.getParameter(1) = p ) } -from NonConstReferenceOrPointerParameterCandidate p, VariableEffect ve +predicate isException(NonConstPointerorReferenceParameter p) { + isNonMemberUserAssignmentParameter(p) and p.getIndex() = 0 + or + isStreamInsertionStreamParameter(p) + or + isStreamExtractionParameter(p) +} + +from NonConstPointerorReferenceParameter p where not isExcluded(p, ConstPackage::outputParametersUsedQuery()) and - ve.getTarget() = p and - isFirstAccess(ve.getAnAccess()) and - not ve instanceof AnyAssignOperation and - not ve instanceof CrementOperation -select p, "Out parameter " + p.getName() + " that is modified before being read." + isOutParameter(p) and + not isException(p) +select p, "Out parameter '" + p.getName() + "' used." diff --git a/cpp/autosar/test/rules/A8-4-8/OutputParametersUsed.expected b/cpp/autosar/test/rules/A8-4-8/OutputParametersUsed.expected index 6b0df8d0dd..221def5a42 100644 --- a/cpp/autosar/test/rules/A8-4-8/OutputParametersUsed.expected +++ b/cpp/autosar/test/rules/A8-4-8/OutputParametersUsed.expected @@ -1,5 +1,6 @@ -| test.cpp:5:22:5:24 | str | Out parameter str that is modified before being read. | -| test.cpp:16:14:16:14 | i | Out parameter i that is modified before being read. | -| test.cpp:21:14:21:14 | i | Out parameter i that is modified before being read. | -| test.cpp:29:12:29:12 | a | Out parameter a that is modified before being read. | -| test.cpp:33:12:33:12 | a | Out parameter a that is modified before being read. | +| test.cpp:5:22:5:24 | str | Out parameter 'str' used. | +| test.cpp:8:22:8:24 | str | Out parameter 'str' used. | +| test.cpp:16:14:16:14 | i | Out parameter 'i' used. | +| test.cpp:21:14:21:14 | i | Out parameter 'i' used. | +| test.cpp:29:12:29:12 | a | Out parameter 'a' used. | +| test.cpp:33:12:33:12 | a | Out parameter 'a' used. | diff --git a/cpp/autosar/test/rules/A8-4-8/test.cpp b/cpp/autosar/test/rules/A8-4-8/test.cpp index e41a61704b..fd2e5e8763 100644 --- a/cpp/autosar/test/rules/A8-4-8/test.cpp +++ b/cpp/autosar/test/rules/A8-4-8/test.cpp @@ -5,7 +5,7 @@ void f(int &i) { // COMPLIANT void f1(std::string &str) { // NON_COMPLIANT str = "replacement"; } -void f2(std::string &str) { // COMPLIANT +void f2(std::string &str) { // NON_COMPLIANT str += "suffix"; } @@ -37,3 +37,41 @@ void f7(A &a) { // NON_COMPLIANT void f8(int i) { // COMPLIANT i += 1; } + +constexpr A &operator|=( + A &lhs, + const A &rhs) noexcept { // COMPLIANT - non-member user defined assignment + // operators are considered an exception. + return lhs; +} + +enum class byte : unsigned char {}; +constexpr byte & +operator|(const byte &lhs, + const byte &rhs) noexcept { // COMPLIANT - parameters are const + // qualified references + return lhs | rhs; +} +constexpr byte &operator|=( + byte &lhs, + const byte rhs) noexcept { // COMPLIANT - non-member user defined assignment + // operators are considered an exception. + lhs = (lhs | rhs); + return lhs; +} + +#include +std::ostream &operator<<(std::ostream &os, + const byte &obj) { // COMPLIANT - insertion operators + // are considered an exception. + std::ostream other; + os = other; // simulate modification + return os; +} + +std::istream &operator>>(std::istream &is, + byte &obj) { // COMPLIANT - extraction operators are + // considered an exception. + obj = static_cast('a'); // simulate modification + return is; +} \ No newline at end of file diff --git a/cpp/cert/src/rules/FIO51-CPP/CloseFilesWhenTheyAreNoLongerNeeded.ql b/cpp/cert/src/rules/FIO51-CPP/CloseFilesWhenTheyAreNoLongerNeeded.ql index 8736348682..383fb9db1f 100644 --- a/cpp/cert/src/rules/FIO51-CPP/CloseFilesWhenTheyAreNoLongerNeeded.ql +++ b/cpp/cert/src/rules/FIO51-CPP/CloseFilesWhenTheyAreNoLongerNeeded.ql @@ -38,8 +38,10 @@ predicate filebufAccess(ControlFlowNode node, FileStreamSource fss) { //insertion or extraction operator calls node.(InsertionOperatorCall).getFStream() = fss.getAUse() or node.(ExtractionOperatorCall).getFStream() = fss.getAUse() or - //methods inherited from istream or ostream - node.(IOStreamFunctionCall).getFStream() = fss.getAUse() + // Methods inherited from istream or ostream that access the file stream. + // Exclude is_open as it is not a filebuf access + any(IOStreamFunctionCall call | node = call and not call.getTarget().hasName("is_open")) + .getFStream() = fss.getAUse() } /** diff --git a/cpp/common/src/codingstandards/cpp/Operator.qll b/cpp/common/src/codingstandards/cpp/Operator.qll index a515f2e241..9f4c5558eb 100644 --- a/cpp/common/src/codingstandards/cpp/Operator.qll +++ b/cpp/common/src/codingstandards/cpp/Operator.qll @@ -1,5 +1,7 @@ import cpp import Expr +private import semmle.code.cpp.security.FileWrite +private import codingstandards.cpp.standardlibrary.FileStreams /** * any assignment operator that also reads from the access @@ -119,7 +121,7 @@ class UserAssignmentOperator extends AssignmentOperator { } /** An assignment operator of any sort */ -class AssignmentOperator extends MemberFunction { +class AssignmentOperator extends Function { AssignmentOperator() { // operator op, where op is =, +=, -=, *=, /=, %=, ^=, &=, |=, >>=, <<= exists(string op | @@ -127,6 +129,8 @@ class AssignmentOperator extends MemberFunction { op in ["=", "+=", "-=", "*=", "/=", "%=", "^=", "&=", "|=", ">>=", "<<="] ) } + + predicate isLValueRefQualified() { this.(MemberFunction).isLValueRefQualified() } } class UserComparisonOperator extends Function { @@ -265,6 +269,38 @@ class UserOverloadedOperator extends Function { } } +/** An implementation of a stream insertion operator. */ +class StreamInsertionOperator extends Function { + StreamInsertionOperator() { + this.hasName("operator<<") and + ( + if this.isMember() + then this.getNumberOfParameters() = 1 + else ( + this.getNumberOfParameters() = 2 and + this.getParameter(0).getType() instanceof BasicOStreamClass + ) + ) and + this.getType() instanceof BasicOStreamClass + } +} + +/** An implementation of a stream extraction operator. */ +class StreamExtractionOperator extends Function { + StreamExtractionOperator() { + this.hasName("operator>>") and + ( + if this.isMember() + then this.getNumberOfParameters() = 1 + else ( + this.getNumberOfParameters() = 2 and + this.getParameter(0).getType() instanceof IStream + ) + ) and + this.getType() instanceof IStream + } +} + /** A user defined operator address of operator (`&`). */ class UnaryAddressOfOperator extends Operator { UnaryAddressOfOperator() { diff --git a/cpp/common/src/codingstandards/cpp/standardlibrary/FileStreams.qll b/cpp/common/src/codingstandards/cpp/standardlibrary/FileStreams.qll index 4d495fce3e..c4724d36c2 100644 --- a/cpp/common/src/codingstandards/cpp/standardlibrary/FileStreams.qll +++ b/cpp/common/src/codingstandards/cpp/standardlibrary/FileStreams.qll @@ -12,6 +12,7 @@ import cpp import codingstandards.cpp.dataflow.DataFlow import codingstandards.cpp.dataflow.TaintTracking +private import codingstandards.cpp.Operator /** * A `basic_fstream` like `std::fstream` @@ -23,15 +24,31 @@ class FileStream extends ClassTemplateInstantiation { /** * A `basic_istream` like `std::istream` */ -class IStream extends ClassTemplateInstantiation { - IStream() { this.getTemplate().hasQualifiedName("std", "basic_istream") } +class IStream extends Type { + IStream() { + this.(Class).getQualifiedName().matches("std::basic\\_istream%") + or + this.getUnspecifiedType() instanceof IStream + or + this.(Class).getABaseClass() instanceof IStream + or + this.(ReferenceType).getBaseType() instanceof IStream + } } /** * A `basic_ostream` like `std::ostream` */ -class OStream extends ClassTemplateInstantiation { - OStream() { this.getTemplate().hasQualifiedName("std", "basic_ostream") } +class OStream extends Type { + OStream() { + this.(Class).getQualifiedName().matches("std::basic\\_ostream%") + or + this.getUnspecifiedType() instanceof OStream + or + this.(Class).getABaseClass() instanceof OStream + or + this.(ReferenceType).getBaseType() instanceof OStream + } } /** @@ -53,7 +70,7 @@ predicate sameStreamSource(FileStreamFunctionCall a, FileStreamFunctionCall b) { * Insertion `operator<<` and Extraction `operator>>` operators. */ class InsertionOperatorCall extends FileStreamFunctionCall { - InsertionOperatorCall() { this.getTarget().(Operator).hasQualifiedName("std", "operator<<") } + InsertionOperatorCall() { this.getTarget() instanceof StreamInsertionOperator } override Expr getFStream() { result = this.getQualifier() @@ -63,7 +80,7 @@ class InsertionOperatorCall extends FileStreamFunctionCall { } class ExtractionOperatorCall extends FileStreamFunctionCall { - ExtractionOperatorCall() { this.getTarget().(Operator).hasQualifiedName("std", "operator>>") } + ExtractionOperatorCall() { this.getTarget() instanceof StreamExtractionOperator } override Expr getFStream() { result = this.getQualifier()