From cc25298f673d7acebbeac2d2783f7d64661e57ba Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 13 Mar 2020 17:00:54 +0100 Subject: [PATCH 1/3] C++: Demonstrate false positives when a const variable is initialized in a parameter list --- .../PointlessComparison.cpp | 29 +++++++++++++++++++ .../PointlessComparison.expected | 3 ++ 2 files changed, 32 insertions(+) create mode 100644 cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/PointlessComparison.cpp diff --git a/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/PointlessComparison.cpp b/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/PointlessComparison.cpp new file mode 100644 index 00000000000..ea5efec3069 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/PointlessComparison.cpp @@ -0,0 +1,29 @@ +void func_with_default_arg(const int n = 0) { + if(n <= 10) {} // GOOD [FALSE POSITIVE] +} + +struct A { + const int int_member = 0; + A(int n) : int_member(n) { + if(int_member <= 10) { + + } + } +}; + +struct B { + B(const int n = 0) { + if(n <= 10) {} // GOOD [FALSE POSITIVE] + } +}; + +const volatile int volatile_const_global = 0; + +void test1() { + func_with_default_arg(100); + + A a(100); + if(a.int_member <= 10) {} + + if(volatile_const_global <= 10) {} // GOOD [FALSE POSITIVE] +} \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/PointlessComparison.expected b/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/PointlessComparison.expected index 55b06bc4b01..8631181b585 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/PointlessComparison.expected +++ b/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/PointlessComparison.expected @@ -38,5 +38,8 @@ | PointlessComparison.c:303:9:303:14 | ... >= ... | Comparison is always false because c <= 0. | | PointlessComparison.c:312:9:312:14 | ... >= ... | Comparison is always false because c <= 0. | | PointlessComparison.c:337:14:337:21 | ... >= ... | Comparison is always true because x >= 0. | +| PointlessComparison.cpp:2:8:2:14 | ... <= ... | Comparison is always true because n <= 0. | +| PointlessComparison.cpp:16:12:16:18 | ... <= ... | Comparison is always true because n <= 0. | +| PointlessComparison.cpp:28:8:28:34 | ... <= ... | Comparison is always true because volatile_const_global <= 0. | | RegressionTests.cpp:57:7:57:22 | ... <= ... | Comparison is always true because * ... <= 4294967295. | | Templates.cpp:9:10:9:24 | ... <= ... | Comparison is always true because local <= 32767. | From e1942bbee126dd2a91142b202e6dae86b7f28694 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 13 Mar 2020 17:02:35 +0100 Subject: [PATCH 2/3] C++: Fix false positives --- .../code/cpp/rangeanalysis/SimpleRangeAnalysis.qll | 13 ++++++++++++- .../PointlessComparison/PointlessComparison.cpp | 6 +++--- .../PointlessComparison.expected | 3 --- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll b/cpp/ql/src/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll index fe89d8ebbe5..e50cdcc2bf9 100644 --- a/cpp/ql/src/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll +++ b/cpp/ql/src/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll @@ -101,9 +101,20 @@ private string getValue(Expr e) { then result = e.getValue() else exists(VariableAccess access, Variable v | + /* + * It should be safe to propagate the initialization value to a variable if: + * The type of v is const, and + * The type of v is not volatile, and + * Either: + * v is a local/global variable, or + * v is a static member variable + */ + + (v instanceof StaticStorageDurationVariable or v instanceof LocalVariable) and + not v.getUnderlyingType().isVolatile() and + v.getUnderlyingType().isConst() and e = access and v = access.getTarget() and - v.getUnderlyingType().isConst() and result = getValue(v.getAnAssignedValue()) ) } diff --git a/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/PointlessComparison.cpp b/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/PointlessComparison.cpp index ea5efec3069..67fcfd7b049 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/PointlessComparison.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/PointlessComparison.cpp @@ -1,5 +1,5 @@ void func_with_default_arg(const int n = 0) { - if(n <= 10) {} // GOOD [FALSE POSITIVE] + if(n <= 10) {} } struct A { @@ -13,7 +13,7 @@ struct A { struct B { B(const int n = 0) { - if(n <= 10) {} // GOOD [FALSE POSITIVE] + if(n <= 10) {} } }; @@ -25,5 +25,5 @@ void test1() { A a(100); if(a.int_member <= 10) {} - if(volatile_const_global <= 10) {} // GOOD [FALSE POSITIVE] + if(volatile_const_global <= 10) {} } \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/PointlessComparison.expected b/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/PointlessComparison.expected index 8631181b585..55b06bc4b01 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/PointlessComparison.expected +++ b/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/PointlessComparison.expected @@ -38,8 +38,5 @@ | PointlessComparison.c:303:9:303:14 | ... >= ... | Comparison is always false because c <= 0. | | PointlessComparison.c:312:9:312:14 | ... >= ... | Comparison is always false because c <= 0. | | PointlessComparison.c:337:14:337:21 | ... >= ... | Comparison is always true because x >= 0. | -| PointlessComparison.cpp:2:8:2:14 | ... <= ... | Comparison is always true because n <= 0. | -| PointlessComparison.cpp:16:12:16:18 | ... <= ... | Comparison is always true because n <= 0. | -| PointlessComparison.cpp:28:8:28:34 | ... <= ... | Comparison is always true because volatile_const_global <= 0. | | RegressionTests.cpp:57:7:57:22 | ... <= ... | Comparison is always true because * ... <= 4294967295. | | Templates.cpp:9:10:9:24 | ... <= ... | Comparison is always true because local <= 32767. | From 09984a40682188d4faadb955daac51e54f22637a Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 13 Mar 2020 17:57:01 +0100 Subject: [PATCH 3/3] C++: The extractor already provides the getValue result when the variable is a local variable. Thus we can simplify the QL code. --- .../cpp/rangeanalysis/SimpleRangeAnalysis.qll | 23 +++++++++---------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll b/cpp/ql/src/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll index e50cdcc2bf9..22e5f5ac83e 100644 --- a/cpp/ql/src/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll +++ b/cpp/ql/src/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll @@ -93,24 +93,23 @@ private float wideningUpperBounds(ArithmeticType t) { /** * Gets the value of the expression `e`, if it is a constant. - * This predicate also handles the case of constant variables initialized in compilation units, - * which doesn't necessarily have a getValue() result from the extractor. + * This predicate also handles the case of constant variables initialized in different + * compilation units, which doesn't necessarily have a getValue() result from the extractor. */ private string getValue(Expr e) { if exists(e.getValue()) then result = e.getValue() else - exists(VariableAccess access, Variable v | - /* - * It should be safe to propagate the initialization value to a variable if: - * The type of v is const, and - * The type of v is not volatile, and - * Either: - * v is a local/global variable, or - * v is a static member variable - */ + /* + * It should be safe to propagate the initialization value to a variable if: + * The type of v is const, and + * The type of v is not volatile, and + * Either: + * v is a local/global variable, or + * v is a static member variable + */ - (v instanceof StaticStorageDurationVariable or v instanceof LocalVariable) and + exists(VariableAccess access, StaticStorageDurationVariable v | not v.getUnderlyingType().isVolatile() and v.getUnderlyingType().isConst() and e = access and