From fbe1aabfb7364806d28b7b50ff6631556d4103d8 Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Fri, 22 Mar 2024 10:25:18 -0400 Subject: [PATCH 01/11] CTR55-CPP: improve end check logic for iterators --- change_notes/2024-03-22-fix-fp-ctr55-cpp.md | 2 + ...GenericCppLibraryFunctionsDoNotOverflow.ql | 11 +--- .../DoNotUseAnAdditiveOperatorOnAnIterator.ql | 51 +++++++++++++------ cpp/cert/test/rules/CTR55-CPP/test.cpp | 15 ++++++ .../src/codingstandards/cpp/Iterators.qll | 27 ++++++++-- 5 files changed, 78 insertions(+), 28 deletions(-) create mode 100644 change_notes/2024-03-22-fix-fp-ctr55-cpp.md diff --git a/change_notes/2024-03-22-fix-fp-ctr55-cpp.md b/change_notes/2024-03-22-fix-fp-ctr55-cpp.md new file mode 100644 index 0000000000..9b30304d29 --- /dev/null +++ b/change_notes/2024-03-22-fix-fp-ctr55-cpp.md @@ -0,0 +1,2 @@ +- `CTR55-CPP` - `DoNotUseAnAdditiveOperatorOnAnIterator.ql`: + - Address reported FP in #374. Improve logic on valid end checks on iterators. \ No newline at end of file diff --git a/cpp/cert/src/rules/CTR52-CPP/GuaranteeGenericCppLibraryFunctionsDoNotOverflow.ql b/cpp/cert/src/rules/CTR52-CPP/GuaranteeGenericCppLibraryFunctionsDoNotOverflow.ql index 720880dbe4..81211c382a 100644 --- a/cpp/cert/src/rules/CTR52-CPP/GuaranteeGenericCppLibraryFunctionsDoNotOverflow.ql +++ b/cpp/cert/src/rules/CTR52-CPP/GuaranteeGenericCppLibraryFunctionsDoNotOverflow.ql @@ -80,16 +80,7 @@ where iteratorCreationCall = outputContainer.getAnIteratorFunctionCall() and iteratorCreationCall = c.getOutputIteratorSource() | - // Guarded by a bounds check that ensures our destination is larger than "some" value - exists( - GuardCondition guard, ContainerAccessWithoutRangeCheck::ContainerSizeCall sizeCall, - boolean branch - | - globalValueNumber(sizeCall.getQualifier()) = - globalValueNumber(iteratorCreationCall.getQualifier()) and - guard.controls(c.getBasicBlock(), branch) and - relOpWithSwapAndNegate(guard, sizeCall, _, Greater(), _, branch) - ) + size_compare_bounds_checked(iteratorCreationCall, c) or // Container created with sufficient size for the input exists(ContainerAccessWithoutRangeCheck::ContainerConstructorCall outputIteratorConstructor | diff --git a/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql b/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql index 8f2aec6e7d..878757ed0a 100644 --- a/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql +++ b/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql @@ -15,23 +15,44 @@ import cpp import codingstandards.cpp.cert import codingstandards.cpp.Iterators -from ContainerIteratorAccess it +/** + * any `.size()` check above our access + */ +predicate size_checked_above(ContainerIteratorAccess it, IteratorSource source) { + exists(STLContainer c, FunctionCall guardCall | + c.getACallToSize() = guardCall and + guardCall = it.getAPredecessor*() and + //make sure its the same container providing its size as giving the iterator + globalValueNumber(guardCall.getQualifier()) = globalValueNumber(source.getQualifier()) and + // and the size call we match must be after the assignment call + source.getASuccessor*() = guardCall + ) +} + +/** + * some loop check exists like: `iterator != end` + * where a relevant`.end()` call flowed into end + */ +predicate valid_end_bound_check(ContainerIteratorAccess it, IteratorSource source) { + exists(STLContainer c, Loop l, ContainerIteratorAccess otherAccess, IteratorSource end | + end = c.getAnIteratorEndFunctionCall() and + //flow exists between end() and the loop condition + DataFlow::localFlow(DataFlow::exprNode(end), DataFlow::exprNode(l.getCondition().getAChild())) and + l.getCondition().getAChild() = otherAccess and + //make sure its the same iterator being checked as incremented + otherAccess.getOwningContainer() = it.getOwningContainer() and + //make sure its the same container providing its end as giving the iterator + globalValueNumber(end.getQualifier()) = globalValueNumber(source.getQualifier()) + ) +} + +from ContainerIteratorAccess it, IteratorSource source where not isExcluded(it, IteratorsPackage::doNotUseAnAdditiveOperatorOnAnIteratorQuery()) and it.isAdditiveOperation() and not exists(RangeBasedForStmt fs | fs.getUpdate().getAChild*() = it) and - // we get the neraby assignment - not exists(STLContainer c, FunctionCall nearbyAssigningIteratorCall, FunctionCall guardCall | - nearbyAssigningIteratorCall = it.getANearbyAssigningIteratorCall() and - // we look for calls to size or end - (guardCall = c.getACallToSize() or guardCall = c.getAnIteratorEndFunctionCall()) and - // such that the call to size is before this - // access - guardCall = it.getAPredecessor*() and - // and it uses the same qualifier as the one we were just assigned - nearbyAssigningIteratorCall.getQualifier().(VariableAccess).getTarget() = - guardCall.getQualifier().(VariableAccess).getTarget() and - // and the size call we match must be after the assignment call - nearbyAssigningIteratorCall.getASuccessor*() = guardCall - ) + source = it.getANearbyAssigningIteratorCall() and + not size_compare_bounds_checked(source, it) and + not valid_end_bound_check(it, source) and + not size_checked_above(it, source) select it, "Increment of iterator may overflow since its bounds are not checked." diff --git a/cpp/cert/test/rules/CTR55-CPP/test.cpp b/cpp/cert/test/rules/CTR55-CPP/test.cpp index d80e8cfab9..7f12beae61 100644 --- a/cpp/cert/test/rules/CTR55-CPP/test.cpp +++ b/cpp/cert/test/rules/CTR55-CPP/test.cpp @@ -26,4 +26,19 @@ void f1(std::vector &v) { for (auto i = v.begin();; ++i) { // NON_COMPLIANT } +} + +void test_fp_reported_in_374(std::vector &v) { + { + auto end = v.end(); + for (auto i = v.begin(); i != end; ++i) { // COMPLIANT + } + } + + { + auto end2 = v.end(); + end2++; // NON_COMPLIANT[FALSE_NEGATIVE] + for (auto i = v.begin(); i != end2; ++i) { // NON_COMPLIANT[FALSE_NEGATIVE] + } + } } \ No newline at end of file diff --git a/cpp/common/src/codingstandards/cpp/Iterators.qll b/cpp/common/src/codingstandards/cpp/Iterators.qll index 593da544ea..6bf35c35f2 100644 --- a/cpp/common/src/codingstandards/cpp/Iterators.qll +++ b/cpp/common/src/codingstandards/cpp/Iterators.qll @@ -6,6 +6,10 @@ import cpp import codingstandards.cpp.dataflow.DataFlow import codingstandards.cpp.dataflow.TaintTracking import codingstandards.cpp.StdNamespace +import codingstandards.cpp.rules.containeraccesswithoutrangecheck.ContainerAccessWithoutRangeCheck as ContainerAccessWithoutRangeCheck +import semmle.code.cpp.controlflow.Guards +import semmle.code.cpp.valuenumbering.GlobalValueNumbering +import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils abstract class ContainerAccess extends VariableAccess { abstract Variable getOwningContainer(); @@ -63,9 +67,11 @@ class ContainerIteratorAccess extends ContainerAccess { ) } - // get a function call to cbegin/begin that - // assigns its value to the iterator represented by this - // access + /** + * gets a function call to cbegin/begin that + * assigns its value to the iterator represented by this + * access + */ FunctionCall getANearbyAssigningIteratorCall() { // the underlying container for this variable is one wherein // there is an assigned value of cbegin/cend @@ -462,3 +468,18 @@ ControlFlowNode getANonInvalidatedSuccessor(ContainerInvalidationOperation op) { not result instanceof ContainerInvalidationOperation ) } + +/** + * Guarded by a bounds check that ensures our destination is larger than "some" value + */ +predicate size_compare_bounds_checked(IteratorSource iteratorCreationCall, Expr guarded) { + exists( + GuardCondition guard, ContainerAccessWithoutRangeCheck::ContainerSizeCall sizeCall, + boolean branch + | + globalValueNumber(sizeCall.getQualifier()) = + globalValueNumber(iteratorCreationCall.getQualifier()) and + guard.controls(guarded.getBasicBlock(), branch) and + relOpWithSwapAndNegate(guard, sizeCall, _, Greater(), _, branch) + ) +} From adbcd3f83db47f9ec17f427ba3c6e8d219ca1195 Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Mon, 25 Mar 2024 11:24:39 -0400 Subject: [PATCH 02/11] CTR55-CPP: address review comments --- ...GenericCppLibraryFunctionsDoNotOverflow.ql | 2 +- .../DoNotUseAnAdditiveOperatorOnAnIterator.ql | 34 +++++++++++-------- .../src/codingstandards/cpp/Iterators.qll | 2 +- 3 files changed, 22 insertions(+), 16 deletions(-) diff --git a/cpp/cert/src/rules/CTR52-CPP/GuaranteeGenericCppLibraryFunctionsDoNotOverflow.ql b/cpp/cert/src/rules/CTR52-CPP/GuaranteeGenericCppLibraryFunctionsDoNotOverflow.ql index 81211c382a..dc53b7a6d0 100644 --- a/cpp/cert/src/rules/CTR52-CPP/GuaranteeGenericCppLibraryFunctionsDoNotOverflow.ql +++ b/cpp/cert/src/rules/CTR52-CPP/GuaranteeGenericCppLibraryFunctionsDoNotOverflow.ql @@ -80,7 +80,7 @@ where iteratorCreationCall = outputContainer.getAnIteratorFunctionCall() and iteratorCreationCall = c.getOutputIteratorSource() | - size_compare_bounds_checked(iteratorCreationCall, c) + sizeCompareBoundsChecked(iteratorCreationCall, c) or // Container created with sufficient size for the input exists(ContainerAccessWithoutRangeCheck::ContainerConstructorCall outputIteratorConstructor | diff --git a/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql b/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql index 878757ed0a..dfc5b2795a 100644 --- a/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql +++ b/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql @@ -14,14 +14,14 @@ import cpp import codingstandards.cpp.cert import codingstandards.cpp.Iterators +import semmle.code.cpp.controlflow.Dominance /** * any `.size()` check above our access */ -predicate size_checked_above(ContainerIteratorAccess it, IteratorSource source) { - exists(STLContainer c, FunctionCall guardCall | - c.getACallToSize() = guardCall and - guardCall = it.getAPredecessor*() and +predicate sizeCheckedAbove(ContainerIteratorAccess it, IteratorSource source) { + exists(ContainerAccessWithoutRangeCheck::ContainerSizeCall guardCall | + strictlyDominates(guardCall, it) and //make sure its the same container providing its size as giving the iterator globalValueNumber(guardCall.getQualifier()) = globalValueNumber(source.getQualifier()) and // and the size call we match must be after the assignment call @@ -30,16 +30,22 @@ predicate size_checked_above(ContainerIteratorAccess it, IteratorSource source) } /** - * some loop check exists like: `iterator != end` + * some guard exists like: `iterator != end` * where a relevant`.end()` call flowed into end */ -predicate valid_end_bound_check(ContainerIteratorAccess it, IteratorSource source) { - exists(STLContainer c, Loop l, ContainerIteratorAccess otherAccess, IteratorSource end | +predicate validEndBoundCheck(ContainerIteratorAccess it, IteratorSource source) { + exists( + STLContainer c, BasicBlock b, GuardCondition l, ContainerIteratorAccess otherAccess, + IteratorSource end + | end = c.getAnIteratorEndFunctionCall() and - //flow exists between end() and the loop condition - DataFlow::localFlow(DataFlow::exprNode(end), DataFlow::exprNode(l.getCondition().getAChild())) and - l.getCondition().getAChild() = otherAccess and - //make sure its the same iterator being checked as incremented + //guard controls the access + l.controls(b, _) and + b.contains(it) and + //guard is comprised of (anything flowing to) end check and an iterator access + DataFlow::localFlow(DataFlow::exprNode(end), DataFlow::exprNode(l.getChild(_))) and + l.getChild(_) = otherAccess and + //make sure its the same iterator being checked in the guard as accessed otherAccess.getOwningContainer() = it.getOwningContainer() and //make sure its the same container providing its end as giving the iterator globalValueNumber(end.getQualifier()) = globalValueNumber(source.getQualifier()) @@ -52,7 +58,7 @@ where it.isAdditiveOperation() and not exists(RangeBasedForStmt fs | fs.getUpdate().getAChild*() = it) and source = it.getANearbyAssigningIteratorCall() and - not size_compare_bounds_checked(source, it) and - not valid_end_bound_check(it, source) and - not size_checked_above(it, source) + not sizeCompareBoundsChecked(source, it) and + not validEndBoundCheck(it, source) and + not sizeCheckedAbove(it, source) select it, "Increment of iterator may overflow since its bounds are not checked." diff --git a/cpp/common/src/codingstandards/cpp/Iterators.qll b/cpp/common/src/codingstandards/cpp/Iterators.qll index 6bf35c35f2..1b5199a806 100644 --- a/cpp/common/src/codingstandards/cpp/Iterators.qll +++ b/cpp/common/src/codingstandards/cpp/Iterators.qll @@ -472,7 +472,7 @@ ControlFlowNode getANonInvalidatedSuccessor(ContainerInvalidationOperation op) { /** * Guarded by a bounds check that ensures our destination is larger than "some" value */ -predicate size_compare_bounds_checked(IteratorSource iteratorCreationCall, Expr guarded) { +predicate sizeCompareBoundsChecked(IteratorSource iteratorCreationCall, Expr guarded) { exists( GuardCondition guard, ContainerAccessWithoutRangeCheck::ContainerSizeCall sizeCall, boolean branch From 4f7523cae25b0511dd45227b043bf725775fb541 Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Mon, 25 Mar 2024 14:39:29 -0400 Subject: [PATCH 03/11] CTR55-CPP: generalize end check --- .../DoNotUseAnAdditiveOperatorOnAnIterator.ql | 5 ++--- .../ContainerAccessWithoutRangeCheck.qll | 10 ++++++++++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql b/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql index dfc5b2795a..163df94fb8 100644 --- a/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql +++ b/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql @@ -35,10 +35,9 @@ predicate sizeCheckedAbove(ContainerIteratorAccess it, IteratorSource source) { */ predicate validEndBoundCheck(ContainerIteratorAccess it, IteratorSource source) { exists( - STLContainer c, BasicBlock b, GuardCondition l, ContainerIteratorAccess otherAccess, - IteratorSource end + ContainerAccessWithoutRangeCheck::ContainerEndCall end, BasicBlock b, GuardCondition l, + ContainerIteratorAccess otherAccess | - end = c.getAnIteratorEndFunctionCall() and //guard controls the access l.controls(b, _) and b.contains(it) and diff --git a/cpp/common/src/codingstandards/cpp/rules/containeraccesswithoutrangecheck/ContainerAccessWithoutRangeCheck.qll b/cpp/common/src/codingstandards/cpp/rules/containeraccesswithoutrangecheck/ContainerAccessWithoutRangeCheck.qll index a3dabedd5a..0784aa9e86 100644 --- a/cpp/common/src/codingstandards/cpp/rules/containeraccesswithoutrangecheck/ContainerAccessWithoutRangeCheck.qll +++ b/cpp/common/src/codingstandards/cpp/rules/containeraccesswithoutrangecheck/ContainerAccessWithoutRangeCheck.qll @@ -85,6 +85,16 @@ class ContainerEmptyCall extends FunctionCall { } } +/** + * A call to either `end()` on a container. + */ +class ContainerEndCall extends FunctionCall { + ContainerEndCall() { + getTarget().getDeclaringType() instanceof ContainerType and + getTarget().getName() = "end" + } +} + /** * A call to either `size()` or `length()` on a container. */ From e948e3902fc2991f4a320dd50bf62b4e511fbb3a Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Mon, 25 Mar 2024 22:02:16 -0400 Subject: [PATCH 04/11] CTR55-CPP: remove unnecessary predicate --- .../rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql | 1 - 1 file changed, 1 deletion(-) diff --git a/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql b/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql index 163df94fb8..2981d8c950 100644 --- a/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql +++ b/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql @@ -57,7 +57,6 @@ where it.isAdditiveOperation() and not exists(RangeBasedForStmt fs | fs.getUpdate().getAChild*() = it) and source = it.getANearbyAssigningIteratorCall() and - not sizeCompareBoundsChecked(source, it) and not validEndBoundCheck(it, source) and not sizeCheckedAbove(it, source) select it, "Increment of iterator may overflow since its bounds are not checked." From bf97331be539fb14a79cdd88829d1faab6d4db35 Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Tue, 26 Mar 2024 17:47:14 -0400 Subject: [PATCH 05/11] CTR55-CPP: model precise end calculation checks and switch to use of size bounds check only --- change_notes/2024-03-22-fix-fp-ctr55-cpp.md | 2 +- .../DoNotUseAnAdditiveOperatorOnAnIterator.ql | 44 +++++++++++++------ ...UseAnAdditiveOperatorOnAnIterator.expected | 2 + cpp/cert/test/rules/CTR55-CPP/test.cpp | 7 +-- .../ContainerAccessWithoutRangeCheck.qll | 10 +++++ 5 files changed, 47 insertions(+), 18 deletions(-) diff --git a/change_notes/2024-03-22-fix-fp-ctr55-cpp.md b/change_notes/2024-03-22-fix-fp-ctr55-cpp.md index 9b30304d29..98e3eb6339 100644 --- a/change_notes/2024-03-22-fix-fp-ctr55-cpp.md +++ b/change_notes/2024-03-22-fix-fp-ctr55-cpp.md @@ -1,2 +1,2 @@ - `CTR55-CPP` - `DoNotUseAnAdditiveOperatorOnAnIterator.ql`: - - Address reported FP in #374. Improve logic on valid end checks on iterators. \ No newline at end of file + - Address reported FP in #374. Improve logic on valid end checks and size checks on iterators. \ No newline at end of file diff --git a/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql b/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql index 2981d8c950..6410b17839 100644 --- a/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql +++ b/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql @@ -17,37 +17,53 @@ import codingstandards.cpp.Iterators import semmle.code.cpp.controlflow.Dominance /** - * any `.size()` check above our access + * something like: + * `end = begin() + size()` */ -predicate sizeCheckedAbove(ContainerIteratorAccess it, IteratorSource source) { - exists(ContainerAccessWithoutRangeCheck::ContainerSizeCall guardCall | - strictlyDominates(guardCall, it) and - //make sure its the same container providing its size as giving the iterator - globalValueNumber(guardCall.getQualifier()) = globalValueNumber(source.getQualifier()) and - // and the size call we match must be after the assignment call - source.getASuccessor*() = guardCall +Expr calculatedEndCheck(AdditiveOperatorFunctionCall calc) { + exists( + ContainerAccessWithoutRangeCheck::ContainerSizeCall size, + ContainerAccessWithoutRangeCheck::ContainerBeginCall begin + | + calc.getTarget().hasName("operator+") and + DataFlow::localFlow(DataFlow::exprNode(size), DataFlow::exprNode(calc.getAChild*())) and + DataFlow::localFlow(DataFlow::exprNode(begin), DataFlow::exprNode(calc.getAChild*())) and + //make sure its the same container providing its size as giving the begin + globalValueNumber(begin.getQualifier()) = globalValueNumber(size.getQualifier()) and + result = begin.getQualifier() ) } +Expr validEndCheck(FunctionCall end) { + end instanceof ContainerAccessWithoutRangeCheck::ContainerEndCall and + result = end.getQualifier() + or + result = calculatedEndCheck(end) +} + /** * some guard exists like: `iterator != end` * where a relevant`.end()` call flowed into end */ predicate validEndBoundCheck(ContainerIteratorAccess it, IteratorSource source) { exists( - ContainerAccessWithoutRangeCheck::ContainerEndCall end, BasicBlock b, GuardCondition l, - ContainerIteratorAccess otherAccess + FunctionCall end, BasicBlock b, GuardCondition l, ContainerIteratorAccess otherAccess, + Expr qualifierToCheck | + //sufficient end guard + qualifierToCheck = validEndCheck(end) and //guard controls the access l.controls(b, _) and b.contains(it) and - //guard is comprised of (anything flowing to) end check and an iterator access + //guard is comprised of end check and an iterator access DataFlow::localFlow(DataFlow::exprNode(end), DataFlow::exprNode(l.getChild(_))) and l.getChild(_) = otherAccess and //make sure its the same iterator being checked in the guard as accessed otherAccess.getOwningContainer() = it.getOwningContainer() and - //make sure its the same container providing its end as giving the iterator - globalValueNumber(end.getQualifier()) = globalValueNumber(source.getQualifier()) + //if its the end call itself (or its parts), make sure its the same container providing its end as giving the iterator + globalValueNumber(qualifierToCheck) = globalValueNumber(source.getQualifier()) and + // and the guard call we match must be after the assignment call (to avoid valid guards protecting new iterator accesses further down) + source.getASuccessor*() = l ) } @@ -58,5 +74,5 @@ where not exists(RangeBasedForStmt fs | fs.getUpdate().getAChild*() = it) and source = it.getANearbyAssigningIteratorCall() and not validEndBoundCheck(it, source) and - not sizeCheckedAbove(it, source) + not sizeCompareBoundsChecked(source, it) select it, "Increment of iterator may overflow since its bounds are not checked." diff --git a/cpp/cert/test/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.expected b/cpp/cert/test/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.expected index 06560517dd..7aa45f734a 100644 --- a/cpp/cert/test/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.expected +++ b/cpp/cert/test/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.expected @@ -1,4 +1,6 @@ | test.cpp:8:7:8:7 | i | Increment of iterator may overflow since its bounds are not checked. | | test.cpp:9:9:9:9 | i | Increment of iterator may overflow since its bounds are not checked. | | test.cpp:10:9:10:9 | i | Increment of iterator may overflow since its bounds are not checked. | +| test.cpp:22:18:22:18 | i | Increment of iterator may overflow since its bounds are not checked. | | test.cpp:27:31:27:31 | i | Increment of iterator may overflow since its bounds are not checked. | +| test.cpp:40:5:40:8 | end2 | Increment of iterator may overflow since its bounds are not checked. | diff --git a/cpp/cert/test/rules/CTR55-CPP/test.cpp b/cpp/cert/test/rules/CTR55-CPP/test.cpp index 7f12beae61..371b145433 100644 --- a/cpp/cert/test/rules/CTR55-CPP/test.cpp +++ b/cpp/cert/test/rules/CTR55-CPP/test.cpp @@ -20,8 +20,9 @@ void f1(std::vector &v) { } for (auto i = v.begin(), l = (i + std::min(static_cast::size_type>(10), - v.size())); - i != l; ++i) { // COMPLIANT + v.size())); // NON_COMPLIANT - technically in the + // calculation + i != l; ++i) { // COMPLIANT } for (auto i = v.begin();; ++i) { // NON_COMPLIANT @@ -37,7 +38,7 @@ void test_fp_reported_in_374(std::vector &v) { { auto end2 = v.end(); - end2++; // NON_COMPLIANT[FALSE_NEGATIVE] + end2++; // NON_COMPLIANT for (auto i = v.begin(); i != end2; ++i) { // NON_COMPLIANT[FALSE_NEGATIVE] } } diff --git a/cpp/common/src/codingstandards/cpp/rules/containeraccesswithoutrangecheck/ContainerAccessWithoutRangeCheck.qll b/cpp/common/src/codingstandards/cpp/rules/containeraccesswithoutrangecheck/ContainerAccessWithoutRangeCheck.qll index 0784aa9e86..71e18a5c05 100644 --- a/cpp/common/src/codingstandards/cpp/rules/containeraccesswithoutrangecheck/ContainerAccessWithoutRangeCheck.qll +++ b/cpp/common/src/codingstandards/cpp/rules/containeraccesswithoutrangecheck/ContainerAccessWithoutRangeCheck.qll @@ -85,6 +85,16 @@ class ContainerEmptyCall extends FunctionCall { } } +/** + * A call to either `begin()` on a container. + */ +class ContainerBeginCall extends FunctionCall { + ContainerBeginCall() { + getTarget().getDeclaringType() instanceof ContainerType and + getTarget().getName() = "begin" + } +} + /** * A call to either `end()` on a container. */ From b7aec2c9b52e38d63e8a10542d53194c0f90298e Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Wed, 27 Mar 2024 10:15:52 -0400 Subject: [PATCH 06/11] CTR55-CPP: fix test expected for reformatted test --- .../CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.expected | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/cert/test/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.expected b/cpp/cert/test/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.expected index 7aa45f734a..13cd4d0ca6 100644 --- a/cpp/cert/test/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.expected +++ b/cpp/cert/test/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.expected @@ -2,5 +2,5 @@ | test.cpp:9:9:9:9 | i | Increment of iterator may overflow since its bounds are not checked. | | test.cpp:10:9:10:9 | i | Increment of iterator may overflow since its bounds are not checked. | | test.cpp:22:18:22:18 | i | Increment of iterator may overflow since its bounds are not checked. | -| test.cpp:27:31:27:31 | i | Increment of iterator may overflow since its bounds are not checked. | -| test.cpp:40:5:40:8 | end2 | Increment of iterator may overflow since its bounds are not checked. | +| test.cpp:28:31:28:31 | i | Increment of iterator may overflow since its bounds are not checked. | +| test.cpp:41:5:41:8 | end2 | Increment of iterator may overflow since its bounds are not checked. | From 4a6661bd86e8c3bad3c945c385591da4f28814f4 Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Thu, 28 Mar 2024 10:49:06 -0400 Subject: [PATCH 07/11] Update cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql Co-authored-by: Remco Vermeulen --- .../CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql b/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql index 6410b17839..f948d8944f 100644 --- a/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql +++ b/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql @@ -17,8 +17,9 @@ import codingstandards.cpp.Iterators import semmle.code.cpp.controlflow.Dominance /** - * something like: - * `end = begin() + size()` + * Get a derived one passed the end element for `containerReference`. + * An example derivation is: + * `end = begin() + size()` */ Expr calculatedEndCheck(AdditiveOperatorFunctionCall calc) { exists( From ac38e79afa5c38e8afe95116d1ea0119e179a1db Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Mon, 1 Apr 2024 12:37:16 -0400 Subject: [PATCH 08/11] CTR55-CPP: predicate renaming, variable clarity --- .../DoNotUseAnAdditiveOperatorOnAnIterator.ql | 58 +++++++++++-------- ...UseAnAdditiveOperatorOnAnIterator.expected | 1 + cpp/cert/test/rules/CTR55-CPP/test.cpp | 8 +++ 3 files changed, 44 insertions(+), 23 deletions(-) diff --git a/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql b/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql index f948d8944f..248d45d9d7 100644 --- a/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql +++ b/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql @@ -17,54 +17,66 @@ import codingstandards.cpp.Iterators import semmle.code.cpp.controlflow.Dominance /** - * Get a derived one passed the end element for `containerReference`. + * Models a call to an iterator's `operator+` + */ +class AdditionOperatorFunctionCall extends AdditiveOperatorFunctionCall { + AdditionOperatorFunctionCall() { this.getTarget().hasName("operator+") } +} + +/** + * There exists a calculation for the reference one passed the end of some container * An example derivation is: * `end = begin() + size()` */ -Expr calculatedEndCheck(AdditiveOperatorFunctionCall calc) { +Expr getDerivedReferenceToOnePassedTheEndElement(Expr containerReference) { exists( ContainerAccessWithoutRangeCheck::ContainerSizeCall size, - ContainerAccessWithoutRangeCheck::ContainerBeginCall begin + ContainerAccessWithoutRangeCheck::ContainerBeginCall begin, AdditionOperatorFunctionCall calc | - calc.getTarget().hasName("operator+") and - DataFlow::localFlow(DataFlow::exprNode(size), DataFlow::exprNode(calc.getAChild*())) and - DataFlow::localFlow(DataFlow::exprNode(begin), DataFlow::exprNode(calc.getAChild*())) and + result = calc + | + DataFlow::localFlow(DataFlow::exprNode(size), DataFlow::exprNode(calc.getAChild+())) and + DataFlow::localFlow(DataFlow::exprNode(begin), DataFlow::exprNode(calc.getAChild+())) and //make sure its the same container providing its size as giving the begin globalValueNumber(begin.getQualifier()) = globalValueNumber(size.getQualifier()) and - result = begin.getQualifier() + containerReference = begin.getQualifier() ) } -Expr validEndCheck(FunctionCall end) { - end instanceof ContainerAccessWithoutRangeCheck::ContainerEndCall and - result = end.getQualifier() +/** + * a wrapper predicate for a couple of types of permitted end bounds checks + */ +Expr getReferenceToOnePassedTheEndElement(Expr containerReference) { + //a container end access - v.end() + result instanceof ContainerAccessWithoutRangeCheck::ContainerEndCall and + containerReference = result.(FunctionCall).getQualifier() or - result = calculatedEndCheck(end) + result = getDerivedReferenceToOnePassedTheEndElement(containerReference) } /** * some guard exists like: `iterator != end` * where a relevant`.end()` call flowed into end */ -predicate validEndBoundCheck(ContainerIteratorAccess it, IteratorSource source) { +predicate isUpperBoundEndCheckedIteratorAccess(IteratorSource source, ContainerIteratorAccess it) { exists( - FunctionCall end, BasicBlock b, GuardCondition l, ContainerIteratorAccess otherAccess, - Expr qualifierToCheck + Expr referenceToOnePassedTheEndElement, BasicBlock basicBlockOfIteratorAccess, GuardCondition upperBoundCheck, + ContainerIteratorAccess checkedIteratorAccess, Expr containerReferenceFromEndGuard | //sufficient end guard - qualifierToCheck = validEndCheck(end) and + referenceToOnePassedTheEndElement = getReferenceToOnePassedTheEndElement(containerReferenceFromEndGuard) and //guard controls the access - l.controls(b, _) and - b.contains(it) and + upperBoundCheck.controls(basicBlockOfIteratorAccess, _) and + basicBlockOfIteratorAccess.contains(it) and //guard is comprised of end check and an iterator access - DataFlow::localFlow(DataFlow::exprNode(end), DataFlow::exprNode(l.getChild(_))) and - l.getChild(_) = otherAccess and + DataFlow::localFlow(DataFlow::exprNode(referenceToOnePassedTheEndElement), DataFlow::exprNode(upperBoundCheck.getChild(_))) and + upperBoundCheck.getChild(_) = checkedIteratorAccess and //make sure its the same iterator being checked in the guard as accessed - otherAccess.getOwningContainer() = it.getOwningContainer() and + checkedIteratorAccess.getOwningContainer() = it.getOwningContainer() and //if its the end call itself (or its parts), make sure its the same container providing its end as giving the iterator - globalValueNumber(qualifierToCheck) = globalValueNumber(source.getQualifier()) and + globalValueNumber(containerReferenceFromEndGuard) = globalValueNumber(source.getQualifier()) and // and the guard call we match must be after the assignment call (to avoid valid guards protecting new iterator accesses further down) - source.getASuccessor*() = l + source.getASuccessor*() = upperBoundCheck ) } @@ -74,6 +86,6 @@ where it.isAdditiveOperation() and not exists(RangeBasedForStmt fs | fs.getUpdate().getAChild*() = it) and source = it.getANearbyAssigningIteratorCall() and - not validEndBoundCheck(it, source) and + not isUpperBoundEndCheckedIteratorAccess(source, it) and not sizeCompareBoundsChecked(source, it) select it, "Increment of iterator may overflow since its bounds are not checked." diff --git a/cpp/cert/test/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.expected b/cpp/cert/test/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.expected index 13cd4d0ca6..7c7bbbdc23 100644 --- a/cpp/cert/test/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.expected +++ b/cpp/cert/test/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.expected @@ -4,3 +4,4 @@ | test.cpp:22:18:22:18 | i | Increment of iterator may overflow since its bounds are not checked. | | test.cpp:28:31:28:31 | i | Increment of iterator may overflow since its bounds are not checked. | | test.cpp:41:5:41:8 | end2 | Increment of iterator may overflow since its bounds are not checked. | +| test.cpp:50:42:50:42 | i | Increment of iterator may overflow since its bounds are not checked. | diff --git a/cpp/cert/test/rules/CTR55-CPP/test.cpp b/cpp/cert/test/rules/CTR55-CPP/test.cpp index 371b145433..d0331bce6d 100644 --- a/cpp/cert/test/rules/CTR55-CPP/test.cpp +++ b/cpp/cert/test/rules/CTR55-CPP/test.cpp @@ -42,4 +42,12 @@ void test_fp_reported_in_374(std::vector &v) { for (auto i = v.begin(); i != end2; ++i) { // NON_COMPLIANT[FALSE_NEGATIVE] } } +} + +void test(std::vector &v, std::vector &v2) { + { + auto end = v2.end(); + for (auto i = v.begin(); i != end; ++i) { // NON_COMPLIANT - wrong check + } + } } \ No newline at end of file From 440cfe9f7dad07bdd0e0ac68a3c7ade55de27d52 Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Mon, 1 Apr 2024 14:03:21 -0400 Subject: [PATCH 09/11] CTR55-CPP: improve documentation known fn case --- .../DoNotUseAnAdditiveOperatorOnAnIterator.expected | 2 +- cpp/cert/test/rules/CTR55-CPP/test.cpp | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/cpp/cert/test/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.expected b/cpp/cert/test/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.expected index 7c7bbbdc23..e8d9425f2b 100644 --- a/cpp/cert/test/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.expected +++ b/cpp/cert/test/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.expected @@ -4,4 +4,4 @@ | test.cpp:22:18:22:18 | i | Increment of iterator may overflow since its bounds are not checked. | | test.cpp:28:31:28:31 | i | Increment of iterator may overflow since its bounds are not checked. | | test.cpp:41:5:41:8 | end2 | Increment of iterator may overflow since its bounds are not checked. | -| test.cpp:50:42:50:42 | i | Increment of iterator may overflow since its bounds are not checked. | +| test.cpp:53:42:53:42 | i | Increment of iterator may overflow since its bounds are not checked. | diff --git a/cpp/cert/test/rules/CTR55-CPP/test.cpp b/cpp/cert/test/rules/CTR55-CPP/test.cpp index d0331bce6d..78eda6c0da 100644 --- a/cpp/cert/test/rules/CTR55-CPP/test.cpp +++ b/cpp/cert/test/rules/CTR55-CPP/test.cpp @@ -38,8 +38,11 @@ void test_fp_reported_in_374(std::vector &v) { { auto end2 = v.end(); - end2++; // NON_COMPLIANT - for (auto i = v.begin(); i != end2; ++i) { // NON_COMPLIANT[FALSE_NEGATIVE] + end2++; // NON_COMPLIANT + for (auto i = v.begin(); i != end2; + ++i) { // NON_COMPLIANT[FALSE_NEGATIVE] - case of invalidations to + // check before use expected to be less frequent, can model in + // future if need be } } } From 87a623cc36529537ca935003cc66b4a755015d27 Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Mon, 1 Apr 2024 14:06:44 -0400 Subject: [PATCH 10/11] CTR55-CPP: missing reformat query --- .../DoNotUseAnAdditiveOperatorOnAnIterator.ql | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql b/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql index 248d45d9d7..ce1fb52667 100644 --- a/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql +++ b/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql @@ -60,16 +60,19 @@ Expr getReferenceToOnePassedTheEndElement(Expr containerReference) { */ predicate isUpperBoundEndCheckedIteratorAccess(IteratorSource source, ContainerIteratorAccess it) { exists( - Expr referenceToOnePassedTheEndElement, BasicBlock basicBlockOfIteratorAccess, GuardCondition upperBoundCheck, - ContainerIteratorAccess checkedIteratorAccess, Expr containerReferenceFromEndGuard + Expr referenceToOnePassedTheEndElement, BasicBlock basicBlockOfIteratorAccess, + GuardCondition upperBoundCheck, ContainerIteratorAccess checkedIteratorAccess, + Expr containerReferenceFromEndGuard | //sufficient end guard - referenceToOnePassedTheEndElement = getReferenceToOnePassedTheEndElement(containerReferenceFromEndGuard) and + referenceToOnePassedTheEndElement = + getReferenceToOnePassedTheEndElement(containerReferenceFromEndGuard) and //guard controls the access upperBoundCheck.controls(basicBlockOfIteratorAccess, _) and basicBlockOfIteratorAccess.contains(it) and //guard is comprised of end check and an iterator access - DataFlow::localFlow(DataFlow::exprNode(referenceToOnePassedTheEndElement), DataFlow::exprNode(upperBoundCheck.getChild(_))) and + DataFlow::localFlow(DataFlow::exprNode(referenceToOnePassedTheEndElement), + DataFlow::exprNode(upperBoundCheck.getChild(_))) and upperBoundCheck.getChild(_) = checkedIteratorAccess and //make sure its the same iterator being checked in the guard as accessed checkedIteratorAccess.getOwningContainer() = it.getOwningContainer() and From cee9f014f769865083d2a4c544d9323c61fdf164 Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Tue, 2 Apr 2024 12:33:37 -0400 Subject: [PATCH 11/11] CTR55-CPP: improve testfile to doc an expected fp corner case --- .../DoNotUseAnAdditiveOperatorOnAnIterator.expected | 1 + cpp/cert/test/rules/CTR55-CPP/test.cpp | 10 ++++++++++ 2 files changed, 11 insertions(+) diff --git a/cpp/cert/test/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.expected b/cpp/cert/test/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.expected index e8d9425f2b..0a06677b54 100644 --- a/cpp/cert/test/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.expected +++ b/cpp/cert/test/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.expected @@ -5,3 +5,4 @@ | test.cpp:28:31:28:31 | i | Increment of iterator may overflow since its bounds are not checked. | | test.cpp:41:5:41:8 | end2 | Increment of iterator may overflow since its bounds are not checked. | | test.cpp:53:42:53:42 | i | Increment of iterator may overflow since its bounds are not checked. | +| test.cpp:64:15:64:15 | i | Increment of iterator may overflow since its bounds are not checked. | diff --git a/cpp/cert/test/rules/CTR55-CPP/test.cpp b/cpp/cert/test/rules/CTR55-CPP/test.cpp index 78eda6c0da..e4d14ec25e 100644 --- a/cpp/cert/test/rules/CTR55-CPP/test.cpp +++ b/cpp/cert/test/rules/CTR55-CPP/test.cpp @@ -53,4 +53,14 @@ void test(std::vector &v, std::vector &v2) { for (auto i = v.begin(); i != end; ++i) { // NON_COMPLIANT - wrong check } } +} + +void test2(std::vector &v) { + auto i = v.begin(); + while (1) { + auto i2 = ((i != v.end()) != 0); + if (!i2) + break; + (void)((++i)); // COMPLIANT[FALSE_POSITIVE] + } } \ No newline at end of file