Skip to content

C++: Support IRGuards with no implicit boolean conversion #16364

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Apr 30, 2024
Merged

C++: Support IRGuards with no implicit boolean conversion #16364

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
806d428
C++: Add test in C file with pointer type guard.
MathiasVP Apr 30, 2024
d736426
C++: Support guards without implicit boolean conversions.
MathiasVP Apr 30, 2024
bb6cc92
C++: Support guards without implicit boolean conversions.
MathiasVP Apr 30, 2024
2939c89
C++: Accept test changes.
MathiasVP Apr 30, 2024
32fe084
Update cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll
MathiasVP Apr 30, 2024
c32c810
C++: Add a test with a 'short' type.
MathiasVP Apr 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -790,6 +790,27 @@ private predicate simple_comparison_eq(Instruction test, Operand op, int k, Abst
exists(switch.getSuccessor(case)) and
case.getValue().toInt() = k
)
or
// There's no implicit CompareInstruction in files compiled as C since C
// doesn't have implicit boolean conversions. So instead we check whether
// there's a branch on a value of pointer or integer type.
exists(ConditionalBranchInstruction branch, IRType type |
not test instanceof CompareInstruction and
type = test.getResultIRType() and
(type instanceof IRAddressType or type instanceof IRIntegerType) and
test = branch.getCondition() and
op.getDef() = test
|
// We'd like to also include a case such as:
// ```
// k = 1 and
// value.(BooleanValue).getValue() = true
// ```
// but all we know is that the value is non-zero in the true branch.
// So we can only conclude something in the false branch.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the consequences if we included common values for true (namely 1 and -1)? Would any results be wrong, or just incomplete?

Copy link
Contributor Author

@MathiasVP MathiasVP Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Results would be wrong. For example, consider this example:

void foo(char* p) {
  if(p) { // assume we uncommented the above so that we inferred that `p == 1` is implied by this
    if(p == 1) { // I know this is a weird comparison to do on a pointer, but oh well.
       // We would conclude that this branch was always taken
    }
  }
}

k = 0 and
value.(BooleanValue).getValue() = false
)
}

private predicate complex_eq(
Expand Down
68 changes: 68 additions & 0 deletions cpp/ql/test/library-tests/controlflow/guards-ir/tests.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,24 @@ astGuardsCompare
| 109 | y < 0+0 when ... < ... is true |
| 109 | y >= 0+0 when ... < ... is false |
| 109 | y >= 0+0 when ... \|\| ... is false |
| 126 | 1 != 0 when 1 is true |
| 126 | 1 != 0 when ... && ... is true |
| 126 | 1 == 0 when 1 is false |
| 126 | call to test3_condition != 0 when ... && ... is true |
| 126 | call to test3_condition != 0 when call to test3_condition is true |
| 126 | call to test3_condition == 0 when call to test3_condition is false |
| 131 | b != 0 when b is true |
| 131 | b == 0 when b is false |
| 137 | 0 != 0 when 0 is true |
| 137 | 0 == 0 when 0 is false |
| 146 | ! ... != 0 when ! ... is true |
| 146 | ! ... == 0 when ! ... is false |
| 152 | x != 0 when ... && ... is true |
| 152 | x != 0 when x is true |
| 152 | x == 0 when x is false |
| 152 | y != 0 when ... && ... is true |
| 152 | y != 0 when y is true |
| 152 | y == 0 when y is false |
| 156 | ... + ... != x+0 when ... == ... is false |
| 156 | ... + ... == x+0 when ... == ... is true |
| 156 | x != ... + ...+0 when ... == ... is false |
Expand Down Expand Up @@ -186,6 +204,8 @@ astGuardsCompare
| 175 | call to foo != 0+0 when ... == ... is false |
| 175 | call to foo == 0 when ... == ... is true |
| 175 | call to foo == 0+0 when ... == ... is true |
| 181 | x != 0 when x is true |
| 181 | x == 0 when x is false |
astGuardsControl
| test.c:7:9:7:13 | ... > ... | false | 10 | 11 |
| test.c:7:9:7:13 | ... > ... | true | 7 | 9 |
Expand Down Expand Up @@ -487,8 +507,27 @@ astGuardsEnsure_const
| test.c:109:9:109:14 | ... == ... | test.c:109:9:109:9 | x | != | 0 | 109 | 109 |
| test.c:109:9:109:14 | ... == ... | test.c:109:9:109:9 | x | != | 0 | 113 | 113 |
| test.c:109:9:109:23 | ... \|\| ... | test.c:109:9:109:9 | x | != | 0 | 113 | 113 |
| test.c:126:7:126:7 | 1 | test.c:126:7:126:7 | 1 | != | 0 | 126 | 126 |
| test.c:126:7:126:7 | 1 | test.c:126:7:126:7 | 1 | != | 0 | 126 | 128 |
| test.c:126:7:126:7 | 1 | test.c:126:7:126:7 | 1 | != | 0 | 131 | 131 |
| test.c:126:7:126:7 | 1 | test.c:126:7:126:7 | 1 | != | 0 | 131 | 132 |
| test.c:126:7:126:7 | 1 | test.c:126:7:126:7 | 1 | != | 0 | 134 | 123 |
| test.c:126:7:126:28 | ... && ... | test.c:126:7:126:7 | 1 | != | 0 | 126 | 128 |
| test.c:126:7:126:28 | ... && ... | test.c:126:12:126:26 | call to test3_condition | != | 0 | 126 | 128 |
| test.c:126:12:126:26 | call to test3_condition | test.c:126:12:126:26 | call to test3_condition | != | 0 | 126 | 128 |
| test.c:131:7:131:7 | b | test.c:131:7:131:7 | b | != | 0 | 131 | 132 |
| test.c:137:7:137:7 | 0 | test.c:137:7:137:7 | 0 | == | 0 | 142 | 136 |
| test.c:146:7:146:8 | ! ... | test.c:146:7:146:8 | ! ... | != | 0 | 146 | 147 |
| test.c:152:10:152:10 | x | test.c:152:10:152:10 | x | != | 0 | 151 | 152 |
| test.c:152:10:152:10 | x | test.c:152:10:152:10 | x | != | 0 | 152 | 152 |
| test.c:152:10:152:15 | ... && ... | test.c:152:10:152:10 | x | != | 0 | 151 | 152 |
| test.c:152:10:152:15 | ... && ... | test.c:152:15:152:15 | y | != | 0 | 151 | 152 |
| test.c:152:15:152:15 | y | test.c:152:15:152:15 | y | != | 0 | 151 | 152 |
| test.c:175:13:175:32 | ... == ... | test.c:175:13:175:15 | call to foo | != | 0 | 175 | 175 |
| test.c:175:13:175:32 | ... == ... | test.c:175:13:175:15 | call to foo | == | 0 | 175 | 175 |
| test.c:181:9:181:9 | x | test.c:181:9:181:9 | x | != | 0 | 181 | 182 |
| test.c:181:9:181:9 | x | test.c:181:9:181:9 | x | != | 0 | 186 | 180 |
| test.c:181:9:181:9 | x | test.c:181:9:181:9 | x | == | 0 | 183 | 184 |
| test.cpp:18:8:18:10 | call to get | test.cpp:18:8:18:10 | call to get | != | 0 | 19 | 19 |
| test.cpp:31:7:31:13 | ... == ... | test.cpp:31:7:31:7 | x | != | -1 | 30 | 30 |
| test.cpp:31:7:31:13 | ... == ... | test.cpp:31:7:31:7 | x | != | -1 | 34 | 34 |
Expand Down Expand Up @@ -640,6 +679,20 @@ irGuardsCompare
| 109 | y < 0+0 when CompareLT: ... < ... is true |
| 109 | y >= 0 when CompareLT: ... < ... is false |
| 109 | y >= 0+0 when CompareLT: ... < ... is false |
| 126 | 1 != 0 when Constant: 1 is true |
| 126 | 1 == 0 when Constant: 1 is false |
| 126 | call to test3_condition != 0 when Call: call to test3_condition is true |
| 126 | call to test3_condition == 0 when Call: call to test3_condition is false |
| 131 | b != 0 when Load: b is true |
| 131 | b == 0 when Load: b is false |
| 137 | 0 != 0 when Constant: 0 is true |
| 137 | 0 == 0 when Constant: 0 is false |
| 146 | ! ... != 0 when LogicalNot: ! ... is true |
| 146 | ! ... == 0 when LogicalNot: ! ... is false |
| 152 | x != 0 when Load: x is true |
| 152 | x == 0 when Load: x is false |
| 152 | y != 0 when Load: y is true |
| 152 | y == 0 when Load: y is false |
| 156 | ... + ... != x+0 when CompareEQ: ... == ... is false |
| 156 | ... + ... == x+0 when CompareEQ: ... == ... is true |
| 156 | x != ... + ...+0 when CompareEQ: ... == ... is false |
Expand Down Expand Up @@ -678,6 +731,8 @@ irGuardsCompare
| 175 | call to foo != 0+0 when CompareEQ: ... == ... is false |
| 175 | call to foo == 0 when CompareEQ: ... == ... is true |
| 175 | call to foo == 0+0 when CompareEQ: ... == ... is true |
| 181 | x != 0 when Load: x is true |
| 181 | x == 0 when Load: x is false |
irGuardsControl
| test.c:7:9:7:13 | CompareGT: ... > ... | false | 11 | 11 |
| test.c:7:9:7:13 | CompareGT: ... > ... | true | 8 | 8 |
Expand Down Expand Up @@ -999,8 +1054,21 @@ irGuardsEnsure_const
| test.c:109:9:109:14 | CompareEQ: ... == ... | test.c:109:9:109:9 | Load: x | != | 0 | 109 | 109 |
| test.c:109:9:109:14 | CompareEQ: ... == ... | test.c:109:9:109:9 | Load: x | != | 0 | 113 | 113 |
| test.c:109:19:109:23 | CompareLT: ... < ... | test.c:109:19:109:19 | Load: y | >= | 0 | 113 | 113 |
| test.c:126:7:126:7 | Constant: 1 | test.c:126:7:126:7 | Constant: 1 | != | 0 | 126 | 126 |
| test.c:126:7:126:7 | Constant: 1 | test.c:126:7:126:7 | Constant: 1 | != | 0 | 127 | 127 |
| test.c:126:7:126:7 | Constant: 1 | test.c:126:7:126:7 | Constant: 1 | != | 0 | 131 | 131 |
| test.c:126:7:126:7 | Constant: 1 | test.c:126:7:126:7 | Constant: 1 | != | 0 | 132 | 132 |
| test.c:126:7:126:7 | Constant: 1 | test.c:126:7:126:7 | Constant: 1 | != | 0 | 134 | 134 |
| test.c:126:12:126:26 | Call: call to test3_condition | test.c:126:12:126:26 | Call: call to test3_condition | != | 0 | 127 | 127 |
| test.c:131:7:131:7 | Load: b | test.c:131:7:131:7 | Load: b | != | 0 | 132 | 132 |
| test.c:137:7:137:7 | Constant: 0 | test.c:137:7:137:7 | Constant: 0 | == | 0 | 142 | 142 |
| test.c:146:7:146:8 | LogicalNot: ! ... | test.c:146:7:146:8 | LogicalNot: ! ... | != | 0 | 147 | 147 |
| test.c:152:10:152:10 | Load: x | test.c:152:10:152:10 | Load: x | != | 0 | 152 | 152 |
| test.c:152:15:152:15 | Load: y | test.c:152:15:152:15 | Load: y | != | 0 | 152 | 152 |
| test.c:175:13:175:32 | CompareEQ: ... == ... | test.c:175:13:175:15 | Call: call to foo | != | 0 | 175 | 175 |
| test.c:175:13:175:32 | CompareEQ: ... == ... | test.c:175:13:175:15 | Call: call to foo | == | 0 | 175 | 175 |
| test.c:181:9:181:9 | Load: x | test.c:181:9:181:9 | Load: x | != | 0 | 182 | 182 |
| test.c:181:9:181:9 | Load: x | test.c:181:9:181:9 | Load: x | == | 0 | 184 | 184 |
| test.cpp:18:8:18:12 | CompareNE: (bool)... | test.cpp:18:8:18:10 | Call: call to get | != | 0 | 19 | 19 |
| test.cpp:31:7:31:13 | CompareEQ: ... == ... | test.cpp:31:7:31:7 | Load: x | != | -1 | 34 | 34 |
| test.cpp:31:7:31:13 | CompareEQ: ... == ... | test.cpp:31:7:31:7 | Load: x | == | -1 | 30 | 30 |
Expand Down
6 changes: 6 additions & 0 deletions cpp/ql/test/library-tests/controlflow/guards/Guards.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@
| test.c:137:7:137:7 | 0 |
| test.c:146:7:146:8 | ! ... |
| test.c:146:8:146:8 | x |
| test.c:152:8:152:8 | p |
| test.c:158:8:158:9 | ! ... |
| test.c:158:9:158:9 | p |
| test.c:164:8:164:8 | s |
| test.c:170:8:170:9 | ! ... |
| test.c:170:9:170:9 | s |
| test.cpp:18:8:18:10 | call to get |
| test.cpp:31:7:31:13 | ... == ... |
| test.cpp:42:13:42:20 | call to getABool |
Expand Down
20 changes: 20 additions & 0 deletions cpp/ql/test/library-tests/controlflow/guards/GuardsCompare.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,3 +149,23 @@
| 111 | 0.0 == i+0 when ... != ... is false |
| 111 | i != 0.0+0 when ... != ... is true |
| 111 | i == 0.0+0 when ... != ... is false |
| 126 | 1 != 0 when 1 is true |
| 126 | 1 != 0 when ... && ... is true |
| 126 | 1 == 0 when 1 is false |
| 126 | call to test3_condition != 0 when ... && ... is true |
| 126 | call to test3_condition != 0 when call to test3_condition is true |
| 126 | call to test3_condition == 0 when call to test3_condition is false |
| 131 | b != 0 when b is true |
| 131 | b == 0 when b is false |
| 137 | 0 != 0 when 0 is true |
| 137 | 0 == 0 when 0 is false |
| 146 | ! ... != 0 when ! ... is true |
| 146 | ! ... == 0 when ! ... is false |
| 152 | p != 0 when p is true |
| 152 | p == 0 when p is false |
| 158 | ! ... != 0 when ! ... is true |
| 158 | ! ... == 0 when ! ... is false |
| 164 | s != 0 when s is true |
| 164 | s == 0 when s is false |
| 170 | ! ... != 0 when ! ... is true |
| 170 | ! ... == 0 when ! ... is false |
6 changes: 6 additions & 0 deletions cpp/ql/test/library-tests/controlflow/guards/GuardsControl.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,12 @@
| test.c:137:7:137:7 | 0 | false | 142 | 136 |
| test.c:146:7:146:8 | ! ... | true | 146 | 147 |
| test.c:146:8:146:8 | x | false | 146 | 147 |
| test.c:152:8:152:8 | p | true | 152 | 154 |
| test.c:158:8:158:9 | ! ... | true | 158 | 160 |
| test.c:158:9:158:9 | p | false | 158 | 160 |
| test.c:164:8:164:8 | s | true | 164 | 166 |
| test.c:170:8:170:9 | ! ... | true | 170 | 172 |
| test.c:170:9:170:9 | s | false | 170 | 172 |
| test.cpp:18:8:18:10 | call to get | true | 19 | 19 |
| test.cpp:31:7:31:13 | ... == ... | false | 30 | 30 |
| test.cpp:31:7:31:13 | ... == ... | false | 34 | 34 |
Expand Down
15 changes: 15 additions & 0 deletions cpp/ql/test/library-tests/controlflow/guards/GuardsEnsure.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,21 @@ unary
| test.c:109:9:109:23 | ... \|\| ... | test.c:109:9:109:9 | x | != | 0 | 113 | 113 |
| test.c:109:9:109:23 | ... \|\| ... | test.c:109:19:109:19 | y | >= | 0 | 113 | 113 |
| test.c:109:19:109:23 | ... < ... | test.c:109:19:109:19 | y | >= | 0 | 113 | 113 |
| test.c:126:7:126:7 | 1 | test.c:126:7:126:7 | 1 | != | 0 | 126 | 126 |
| test.c:126:7:126:7 | 1 | test.c:126:7:126:7 | 1 | != | 0 | 126 | 128 |
| test.c:126:7:126:7 | 1 | test.c:126:7:126:7 | 1 | != | 0 | 131 | 131 |
| test.c:126:7:126:7 | 1 | test.c:126:7:126:7 | 1 | != | 0 | 131 | 132 |
| test.c:126:7:126:7 | 1 | test.c:126:7:126:7 | 1 | != | 0 | 134 | 123 |
| test.c:126:7:126:28 | ... && ... | test.c:126:7:126:7 | 1 | != | 0 | 126 | 128 |
| test.c:126:7:126:28 | ... && ... | test.c:126:12:126:26 | call to test3_condition | != | 0 | 126 | 128 |
| test.c:126:12:126:26 | call to test3_condition | test.c:126:12:126:26 | call to test3_condition | != | 0 | 126 | 128 |
| test.c:131:7:131:7 | b | test.c:131:7:131:7 | b | != | 0 | 131 | 132 |
| test.c:137:7:137:7 | 0 | test.c:137:7:137:7 | 0 | == | 0 | 142 | 136 |
| test.c:146:7:146:8 | ! ... | test.c:146:7:146:8 | ! ... | != | 0 | 146 | 147 |
| test.c:152:8:152:8 | p | test.c:152:8:152:8 | p | != | 0 | 152 | 154 |
| test.c:158:8:158:9 | ! ... | test.c:158:8:158:9 | ! ... | != | 0 | 158 | 160 |
| test.c:164:8:164:8 | s | test.c:164:8:164:8 | s | != | 0 | 164 | 166 |
| test.c:170:8:170:9 | ! ... | test.c:170:8:170:9 | ! ... | != | 0 | 170 | 172 |
| test.cpp:18:8:18:10 | call to get | test.cpp:18:8:18:10 | call to get | != | 0 | 19 | 19 |
| test.cpp:31:7:31:13 | ... == ... | test.cpp:31:7:31:7 | x | != | -1 | 30 | 30 |
| test.cpp:31:7:31:13 | ... == ... | test.cpp:31:7:31:7 | x | != | -1 | 34 | 34 |
Expand Down
24 changes: 24 additions & 0 deletions cpp/ql/test/library-tests/controlflow/guards/test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,3 +147,27 @@ void test5(int x) {
test3();
}
}

void test6(char* p) {
if(p) {

}
}

void test7(char* p) {
if(!p) {

}
}

void test8(short s) {
if(s) {

}
}

void test9(short s) {
if(!s) {

}
}