hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-05-02 12:15:17 +02:00
Code Issues Packages Projects Releases Wiki Activity

C++: Respond to more review comments.

Browse Source 
- Remove post-dominance requirement. It was really just hiding good
  results.
- Fix test annotations. Turns out Clang and GCC's 'undefined behavior'
  warning didn't align with the C++ standard.
This commit is contained in:
Mathias Vorreiter Pedersen
2020-11-13 15:44:33 +01:00
parent b249777bfb
commit 0a6a22562b
8 changed files with 64 additions and 246 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
cpp/ql/src/Likely Bugs/OO/UnsafeUseOfThis.ql
View File

@@ -169,15 +169,11 @@ predicate flowThroughCallable(Instruction instr, Instruction succ) {
/** Holds if `instr` flows to `succ`. */
predicate successor(Instruction instr, Instruction succ) {
succ.getBlock().postDominates(instr.getBlock()) and
(
succ.(CopyInstruction).getSourceValue() = instr or
succ.(CheckedConvertOrNullInstruction).getUnary() = instr or
succ.(ChiInstruction).getTotal() = instr or
succ.(ConvertInstruction).getUnary() = instr or
succ.(InheritanceConversionInstruction).getUnary() = instr
)
or
succ.(CopyInstruction).getSourceValue() = instr or
succ.(CheckedConvertOrNullInstruction).getUnary() = instr or
succ.(ChiInstruction).getTotal() = instr or
succ.(ConvertInstruction).getUnary() = instr or
succ.(InheritanceConversionInstruction).getUnary() = instr or
flowThroughCallable(instr, succ)
}

37
cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/IRBlock.qll
View File

@@ -151,34 +151,6 @@ class IRBlock extends IRBlockBase {
*/
final predicate dominates(IRBlock block) { strictlyDominates(block) or this = block }
/**
* Holds if this block immediately post-dominates `block`.
*
* Block `A` immediate post-dominates block `B` if block `A` strictly post-dominates block `B` and
* block `B` is a direct successor of block `A`.
*/
final predicate immediatelyPostDominates(IRBlock block) {
blockImmediatelyPostDominates(this, block)
}
/**
* Holds if this block strictly post-dominates `block`.
*
* Block `A` strictly post-dominates block `B` if block `A` post-dominates block `B` and blocks `A`
* and `B` are not the same block.
*/
private predicate strictlyPostDominates(IRBlock block) {
blockImmediatelyPostDominates+(this, block)
}
/**
* Holds if this block is a post-dominator of `block`.
*
* Block `A` post-dominates block `B` if any control flow path from `B` to the exit block of the
* function must pass through block `A`. A block always post-dominates itself.
*/
predicate postDominates(IRBlock block) { strictlyPostDominates(block) or this = block }
/**
* Gets a block on the dominance frontier of this block.
*
@@ -308,12 +280,3 @@ private module Cached {
}
private Instruction getFirstInstruction(TIRBlock block) { block = MkIRBlock(result) }
private predicate blockFunctionExit(IRBlock exit) {
exit.getLastInstruction() instanceof ExitFunctionInstruction
}
private predicate blockPredecessor(IRBlock src, IRBlock pred) { src.getAPredecessor() = pred }
private predicate blockImmediatelyPostDominates(IRBlock postDominator, IRBlock block) =
idominance(blockFunctionExit/1, blockPredecessor/2)(_, postDominator, block)

37
cpp/ql/src/semmle/code/cpp/ir/implementation/raw/IRBlock.qll
View File

@@ -151,34 +151,6 @@ class IRBlock extends IRBlockBase {
*/
final predicate dominates(IRBlock block) { strictlyDominates(block) or this = block }
/**
* Holds if this block immediately post-dominates `block`.
*
* Block `A` immediate post-dominates block `B` if block `A` strictly post-dominates block `B` and
* block `B` is a direct successor of block `A`.
*/
final predicate immediatelyPostDominates(IRBlock block) {
blockImmediatelyPostDominates(this, block)
}
/**
* Holds if this block strictly post-dominates `block`.
*
* Block `A` strictly post-dominates block `B` if block `A` post-dominates block `B` and blocks `A`
* and `B` are not the same block.
*/
private predicate strictlyPostDominates(IRBlock block) {
blockImmediatelyPostDominates+(this, block)
}
/**
* Holds if this block is a post-dominator of `block`.
*
* Block `A` post-dominates block `B` if any control flow path from `B` to the exit block of the
* function must pass through block `A`. A block always post-dominates itself.
*/
predicate postDominates(IRBlock block) { strictlyPostDominates(block) or this = block }
/**
* Gets a block on the dominance frontier of this block.
*
@@ -308,12 +280,3 @@ private module Cached {
}
private Instruction getFirstInstruction(TIRBlock block) { block = MkIRBlock(result) }
private predicate blockFunctionExit(IRBlock exit) {
exit.getLastInstruction() instanceof ExitFunctionInstruction
}
private predicate blockPredecessor(IRBlock src, IRBlock pred) { src.getAPredecessor() = pred }
private predicate blockImmediatelyPostDominates(IRBlock postDominator, IRBlock block) =
idominance(blockFunctionExit/1, blockPredecessor/2)(_, postDominator, block)

37
cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/IRBlock.qll
View File

@@ -151,34 +151,6 @@ class IRBlock extends IRBlockBase {
*/
final predicate dominates(IRBlock block) { strictlyDominates(block) or this = block }
/**
* Holds if this block immediately post-dominates `block`.
*
* Block `A` immediate post-dominates block `B` if block `A` strictly post-dominates block `B` and
* block `B` is a direct successor of block `A`.
*/
final predicate immediatelyPostDominates(IRBlock block) {
blockImmediatelyPostDominates(this, block)
}
/**
* Holds if this block strictly post-dominates `block`.
*
* Block `A` strictly post-dominates block `B` if block `A` post-dominates block `B` and blocks `A`
* and `B` are not the same block.
*/
private predicate strictlyPostDominates(IRBlock block) {
blockImmediatelyPostDominates+(this, block)
}
/**
* Holds if this block is a post-dominator of `block`.
*
* Block `A` post-dominates block `B` if any control flow path from `B` to the exit block of the
* function must pass through block `A`. A block always post-dominates itself.
*/
predicate postDominates(IRBlock block) { strictlyPostDominates(block) or this = block }
/**
* Gets a block on the dominance frontier of this block.
*
@@ -308,12 +280,3 @@ private module Cached {
}
private Instruction getFirstInstruction(TIRBlock block) { block = MkIRBlock(result) }
private predicate blockFunctionExit(IRBlock exit) {
exit.getLastInstruction() instanceof ExitFunctionInstruction
}
private predicate blockPredecessor(IRBlock src, IRBlock pred) { src.getAPredecessor() = pred }
private predicate blockImmediatelyPostDominates(IRBlock postDominator, IRBlock block) =
idominance(blockFunctionExit/1, blockPredecessor/2)(_, postDominator, block)

107
cpp/ql/test/query-tests/Critical/UnsafeUseOfThis/UnsafeUseOfThis.expected
View File

@@ -1,55 +1,62 @@
edges
| test.cpp:7:2:7:2 | InitializeParameter: B | test.cpp:8:10:8:13 | Load: this |
| test.cpp:8:10:8:13 | Load: this | test.cpp:34:16:34:16 | InitializeParameter: x |
| test.cpp:11:10:11:10 | InitializeParameter: b | test.cpp:12:9:12:9 | Load: b |
| test.cpp:12:9:12:9 | CopyValue: (reference dereference) | test.cpp:12:9:12:9 | ConvertToNonVirtualBase: (A)... |
| test.cpp:12:9:12:9 | Load: b | test.cpp:12:9:12:9 | CopyValue: (reference dereference) |
| test.cpp:15:2:15:3 | InitializeParameter: ~B | test.cpp:16:3:16:3 | Load: this |
| test.cpp:16:3:16:3 | Load: this | file://:0:0:0:0 | ConvertToNonVirtualBase: (A *)... |
| test.cpp:21:2:21:2 | InitializeParameter: C | test.cpp:21:12:21:12 | ConvertToNonVirtualBase: call to B |
| test.cpp:21:2:21:2 | InitializeParameter: C | test.cpp:22:10:22:13 | Load: this |
| test.cpp:21:12:21:12 | ConvertToNonVirtualBase: call to B | test.cpp:7:2:7:2 | InitializeParameter: B |
| test.cpp:22:10:22:13 | ConvertToNonVirtualBase: (B *)... | test.cpp:34:16:34:16 | InitializeParameter: x |
| test.cpp:22:10:22:13 | Load: this | test.cpp:22:10:22:13 | ConvertToNonVirtualBase: (B *)... |
| test.cpp:31:5:31:5 | InitializeParameter: D | test.cpp:31:14:31:17 | Load: this |
| test.cpp:31:13:31:17 | ConvertToNonVirtualBase: (B)... | test.cpp:31:13:31:17 | CopyValue: (reference to) |
| test.cpp:31:13:31:17 | CopyValue: (reference to) | test.cpp:11:10:11:10 | InitializeParameter: b |
| test.cpp:31:13:31:17 | CopyValue: * ... | test.cpp:31:13:31:17 | ConvertToNonVirtualBase: (B)... |
| test.cpp:31:14:31:17 | Load: this | test.cpp:31:13:31:17 | CopyValue: * ... |
| test.cpp:34:16:34:16 | InitializeParameter: x | test.cpp:35:2:35:2 | Load: x |
| test.cpp:35:2:35:2 | Load: x | test.cpp:35:2:35:2 | ConvertToNonVirtualBase: (A *)... |
| test.cpp:47:2:47:2 | InitializeParameter: F | test.cpp:48:8:48:11 | Load: this |
| test.cpp:48:8:48:11 | ConvertToNonVirtualBase: (E *)... | test.cpp:48:4:48:11 | ConvertToNonVirtualBase: (A *)... |
| test.cpp:48:8:48:11 | Load: this | test.cpp:48:8:48:11 | ConvertToNonVirtualBase: (E *)... |
| test.cpp:7:3:7:3 | InitializeParameter: B | test.cpp:8:12:8:15 | Load: this |
| test.cpp:8:12:8:15 | Load: this | test.cpp:34:16:34:16 | InitializeParameter: x |
| test.cpp:11:8:11:8 | InitializeParameter: b | test.cpp:12:5:12:5 | Load: b |
| test.cpp:12:5:12:5 | CopyValue: (reference dereference) | test.cpp:12:5:12:5 | ConvertToNonVirtualBase: (A)... |
| test.cpp:12:5:12:5 | Load: b | test.cpp:12:5:12:5 | CopyValue: (reference dereference) |
| test.cpp:15:3:15:4 | InitializeParameter: ~B | test.cpp:16:5:16:5 | Load: this |
| test.cpp:16:5:16:5 | Load: this | file://:0:0:0:0 | ConvertToNonVirtualBase: (A *)... |
| test.cpp:21:3:21:3 | InitializeParameter: C | test.cpp:21:13:21:13 | ConvertToNonVirtualBase: call to B |
| test.cpp:21:3:21:3 | InitializeParameter: C | test.cpp:22:12:22:15 | Load: this |
| test.cpp:21:3:21:3 | InitializeParameter: C | test.cpp:25:7:25:10 | Load: this |
| test.cpp:21:13:21:13 | ConvertToNonVirtualBase: call to B | test.cpp:7:3:7:3 | InitializeParameter: B |
| test.cpp:22:12:22:15 | ConvertToNonVirtualBase: (B *)... | test.cpp:34:16:34:16 | InitializeParameter: x |
| test.cpp:22:12:22:15 | Load: this | test.cpp:22:12:22:15 | ConvertToNonVirtualBase: (B *)... |
| test.cpp:25:7:25:10 | ConvertToNonVirtualBase: (B *)... | test.cpp:25:7:25:10 | ConvertToNonVirtualBase: (A *)... |
| test.cpp:25:7:25:10 | Load: this | test.cpp:25:7:25:10 | ConvertToNonVirtualBase: (B *)... |
| test.cpp:31:3:31:3 | InitializeParameter: D | test.cpp:31:12:31:15 | Load: this |
| test.cpp:31:11:31:15 | ConvertToNonVirtualBase: (B)... | test.cpp:31:11:31:15 | CopyValue: (reference to) |
| test.cpp:31:11:31:15 | CopyValue: (reference to) | test.cpp:11:8:11:8 | InitializeParameter: b |
| test.cpp:31:11:31:15 | CopyValue: * ... | test.cpp:31:11:31:15 | ConvertToNonVirtualBase: (B)... |
| test.cpp:31:12:31:15 | Load: this | test.cpp:31:11:31:15 | CopyValue: * ... |
| test.cpp:34:16:34:16 | InitializeParameter: x | test.cpp:35:3:35:3 | Load: x |
| test.cpp:35:3:35:3 | Load: x | test.cpp:35:3:35:3 | ConvertToNonVirtualBase: (A *)... |
| test.cpp:47:3:47:3 | InitializeParameter: F | test.cpp:48:10:48:13 | Load: this |
| test.cpp:48:10:48:13 | ConvertToNonVirtualBase: (E *)... | test.cpp:48:6:48:13 | ConvertToNonVirtualBase: (A *)... |
| test.cpp:48:10:48:13 | Load: this | test.cpp:48:10:48:13 | ConvertToNonVirtualBase: (E *)... |
nodes
| file://:0:0:0:0 | ConvertToNonVirtualBase: (A *)... | semmle.label | ConvertToNonVirtualBase: (A *)... |
| test.cpp:7:2:7:2 | InitializeParameter: B | semmle.label | InitializeParameter: B |
| test.cpp:8:10:8:13 | Load: this | semmle.label | Load: this |
| test.cpp:11:10:11:10 | InitializeParameter: b | semmle.label | InitializeParameter: b |
| test.cpp:12:9:12:9 | ConvertToNonVirtualBase: (A)... | semmle.label | ConvertToNonVirtualBase: (A)... |
| test.cpp:12:9:12:9 | CopyValue: (reference dereference) | semmle.label | CopyValue: (reference dereference) |
| test.cpp:12:9:12:9 | Load: b | semmle.label | Load: b |
| test.cpp:15:2:15:3 | InitializeParameter: ~B | semmle.label | InitializeParameter: ~B |
| test.cpp:16:3:16:3 | Load: this | semmle.label | Load: this |
| test.cpp:21:2:21:2 | InitializeParameter: C | semmle.label | InitializeParameter: C |
| test.cpp:21:12:21:12 | ConvertToNonVirtualBase: call to B | semmle.label | ConvertToNonVirtualBase: call to B |
| test.cpp:22:10:22:13 | ConvertToNonVirtualBase: (B *)... | semmle.label | ConvertToNonVirtualBase: (B *)... |
| test.cpp:22:10:22:13 | Load: this | semmle.label | Load: this |
| test.cpp:31:5:31:5 | InitializeParameter: D | semmle.label | InitializeParameter: D |
| test.cpp:31:13:31:17 | ConvertToNonVirtualBase: (B)... | semmle.label | ConvertToNonVirtualBase: (B)... |
| test.cpp:31:13:31:17 | CopyValue: (reference to) | semmle.label | CopyValue: (reference to) |
| test.cpp:31:13:31:17 | CopyValue: * ... | semmle.label | CopyValue: * ... |
| test.cpp:31:14:31:17 | Load: this | semmle.label | Load: this |
| test.cpp:7:3:7:3 | InitializeParameter: B | semmle.label | InitializeParameter: B |
| test.cpp:8:12:8:15 | Load: this | semmle.label | Load: this |
| test.cpp:11:8:11:8 | InitializeParameter: b | semmle.label | InitializeParameter: b |
| test.cpp:12:5:12:5 | ConvertToNonVirtualBase: (A)... | semmle.label | ConvertToNonVirtualBase: (A)... |
| test.cpp:12:5:12:5 | CopyValue: (reference dereference) | semmle.label | CopyValue: (reference dereference) |
| test.cpp:12:5:12:5 | Load: b | semmle.label | Load: b |
| test.cpp:15:3:15:4 | InitializeParameter: ~B | semmle.label | InitializeParameter: ~B |
| test.cpp:16:5:16:5 | Load: this | semmle.label | Load: this |
| test.cpp:21:3:21:3 | InitializeParameter: C | semmle.label | InitializeParameter: C |
| test.cpp:21:13:21:13 | ConvertToNonVirtualBase: call to B | semmle.label | ConvertToNonVirtualBase: call to B |
| test.cpp:22:12:22:15 | ConvertToNonVirtualBase: (B *)... | semmle.label | ConvertToNonVirtualBase: (B *)... |
| test.cpp:22:12:22:15 | Load: this | semmle.label | Load: this |
| test.cpp:25:7:25:10 | ConvertToNonVirtualBase: (A *)... | semmle.label | ConvertToNonVirtualBase: (A *)... |
| test.cpp:25:7:25:10 | ConvertToNonVirtualBase: (B *)... | semmle.label | ConvertToNonVirtualBase: (B *)... |
| test.cpp:25:7:25:10 | Load: this | semmle.label | Load: this |
| test.cpp:31:3:31:3 | InitializeParameter: D | semmle.label | InitializeParameter: D |
| test.cpp:31:11:31:15 | ConvertToNonVirtualBase: (B)... | semmle.label | ConvertToNonVirtualBase: (B)... |
| test.cpp:31:11:31:15 | CopyValue: (reference to) | semmle.label | CopyValue: (reference to) |
| test.cpp:31:11:31:15 | CopyValue: * ... | semmle.label | CopyValue: * ... |
| test.cpp:31:12:31:15 | Load: this | semmle.label | Load: this |
| test.cpp:34:16:34:16 | InitializeParameter: x | semmle.label | InitializeParameter: x |
| test.cpp:35:2:35:2 | ConvertToNonVirtualBase: (A *)... | semmle.label | ConvertToNonVirtualBase: (A *)... |
| test.cpp:35:2:35:2 | Load: x | semmle.label | Load: x |
| test.cpp:47:2:47:2 | InitializeParameter: F | semmle.label | InitializeParameter: F |
| test.cpp:48:4:48:11 | ConvertToNonVirtualBase: (A *)... | semmle.label | ConvertToNonVirtualBase: (A *)... |
| test.cpp:48:8:48:11 | ConvertToNonVirtualBase: (E *)... | semmle.label | ConvertToNonVirtualBase: (E *)... |
| test.cpp:48:8:48:11 | Load: this | semmle.label | Load: this |
| test.cpp:35:3:35:3 | ConvertToNonVirtualBase: (A *)... | semmle.label | ConvertToNonVirtualBase: (A *)... |
| test.cpp:35:3:35:3 | Load: x | semmle.label | Load: x |
| test.cpp:47:3:47:3 | InitializeParameter: F | semmle.label | InitializeParameter: F |
| test.cpp:48:6:48:13 | ConvertToNonVirtualBase: (A *)... | semmle.label | ConvertToNonVirtualBase: (A *)... |
| test.cpp:48:10:48:13 | ConvertToNonVirtualBase: (E *)... | semmle.label | ConvertToNonVirtualBase: (E *)... |
| test.cpp:48:10:48:13 | Load: this | semmle.label | Load: this |
#select
| test.cpp:12:11:12:11 | call to f | test.cpp:31:5:31:5 | InitializeParameter: D | test.cpp:12:9:12:9 | ConvertToNonVirtualBase: (A)... | Call to pure virtual function during construction |
| test.cpp:16:3:16:3 | call to f | test.cpp:15:2:15:3 | InitializeParameter: ~B | file://:0:0:0:0 | ConvertToNonVirtualBase: (A *)... | Call to pure virtual function during destruction |
| test.cpp:35:5:35:5 | call to f | test.cpp:7:2:7:2 | InitializeParameter: B | test.cpp:35:2:35:2 | ConvertToNonVirtualBase: (A *)... | Call to pure virtual function during construction |
| test.cpp:35:5:35:5 | call to f | test.cpp:21:2:21:2 | InitializeParameter: C | test.cpp:35:2:35:2 | ConvertToNonVirtualBase: (A *)... | Call to pure virtual function during construction |
| test.cpp:48:15:48:15 | call to f | test.cpp:47:2:47:2 | InitializeParameter: F | test.cpp:48:4:48:11 | ConvertToNonVirtualBase: (A *)... | Call to pure virtual function during construction |
| test.cpp:12:7:12:7 | call to f | test.cpp:31:3:31:3 | InitializeParameter: D | test.cpp:12:5:12:5 | ConvertToNonVirtualBase: (A)... | Call to pure virtual function during construction |
| test.cpp:16:5:16:5 | call to f | test.cpp:15:3:15:4 | InitializeParameter: ~B | file://:0:0:0:0 | ConvertToNonVirtualBase: (A *)... | Call to pure virtual function during destruction |
| test.cpp:25:13:25:13 | call to f | test.cpp:21:3:21:3 | InitializeParameter: C | test.cpp:25:7:25:10 | ConvertToNonVirtualBase: (A *)... | Call to pure virtual function during construction |
| test.cpp:35:6:35:6 | call to f | test.cpp:7:3:7:3 | InitializeParameter: B | test.cpp:35:3:35:3 | ConvertToNonVirtualBase: (A *)... | Call to pure virtual function during construction |
| test.cpp:35:6:35:6 | call to f | test.cpp:21:3:21:3 | InitializeParameter: C | test.cpp:35:3:35:3 | ConvertToNonVirtualBase: (A *)... | Call to pure virtual function during construction |
| test.cpp:48:17:48:17 | call to f | test.cpp:47:3:47:3 | InitializeParameter: F | test.cpp:48:6:48:13 | ConvertToNonVirtualBase: (A *)... | Call to pure virtual function during construction |

4
cpp/ql/test/query-tests/Critical/UnsafeUseOfThis/test.cpp
View File

@@ -22,7 +22,7 @@ struct C : public B {
call_f(this);
if(b) {
this->f(); // GOOD: Not a 'must' flow
this->f(); // BAD: undefined behavior
}
}
};
@@ -45,6 +45,6 @@ struct E : public A {
struct F : public E {
F() {
((A*)this)->f(); // BAD: undefined behavior
((A*)this)->f(); // GOOD: Will call `E::f` [FALSE POSITIVE]
}
};