Skip to content

Commit fd62820

Browse files
MathiasVPMathiasVP
authored
Merge pull request github#12971 from MathiasVP/fix-fp-in-invalid-deref-2
C++: Fix more FPs on `cpp/invalid-pointer-deref`
2 parents 71be426 + f05cce8 commit fd62820

File tree

5 files changed

+22
-55
lines changed

5 files changed

+22
-55
lines changed

‎cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -562,6 +562,14 @@ class SsaPhiNode extends Node, TSsaPhiNode {
562562

563563
/** Gets the source variable underlying this phi node. */
564564
Ssa::SourceVariable getSourceVariable() { result = phi.getSourceVariable() }
565+
566+
/**
567+
* Holds if this phi node is a phi-read node.
568+
*
569+
* Phi-read nodes are like normal phi nodes, but they are inserted based
570+
* on reads instead of writes.
571+
*/
572+
predicate isPhiRead() { phi.isPhiRead() }
565573
}
566574

567575
/**

‎cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1012,6 +1012,14 @@ class PhiNode extends SsaImpl::DefinitionExt {
10121012
this instanceof SsaImpl::PhiNode or
10131013
this instanceof SsaImpl::PhiReadNode
10141014
}
1015+
1016+
/**
1017+
* Holds if this phi node is a phi-read node.
1018+
*
1019+
* Phi-read nodes are like normal phi nodes, but they are inserted based
1020+
* on reads instead of writes.
1021+
*/
1022+
predicate isPhiRead() { this instanceof SsaImpl::PhiReadNode }
10151023
}
10161024

10171025
class DefinitionExt = SsaImpl::DefinitionExt;

‎cpp/ql/src/experimental/Security/CWE/CWE-193/InvalidPointerDeref.ql

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -229,6 +229,10 @@ module InvalidPointerToDerefConfig implements DataFlow::ConfigSig {
229229

230230
pragma[inline]
231231
predicate isSink(DataFlow::Node sink) { isInvalidPointerDerefSink(sink, _, _) }
232+
233+
predicate isBarrier(DataFlow::Node node) {
234+
node = any(DataFlow::SsaPhiNode phi | not phi.isPhiRead()).getAnInput(true)
235+
}
232236
}
233237

234238
module InvalidPointerToDerefFlow = DataFlow::Global<InvalidPointerToDerefConfig>;

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/InvalidPointerDeref.expected

Lines changed: 0 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -594,8 +594,6 @@ edges
594594
| test.cpp:261:14:261:15 | xs | test.cpp:262:26:262:28 | end |
595595
| test.cpp:261:14:261:15 | xs | test.cpp:262:26:262:28 | end |
596596
| test.cpp:261:14:261:15 | xs | test.cpp:262:31:262:31 | x |
597-
| test.cpp:261:14:261:15 | xs | test.cpp:262:31:262:33 | ... ++ |
598-
| test.cpp:261:14:261:15 | xs | test.cpp:262:31:262:33 | ... ++ |
599597
| test.cpp:261:14:261:15 | xs | test.cpp:264:14:264:14 | x |
600598
| test.cpp:261:14:261:15 | xs | test.cpp:264:14:264:14 | x |
601599
| test.cpp:261:14:261:21 | ... + ... | test.cpp:261:14:261:21 | ... + ... |
@@ -608,20 +606,11 @@ edges
608606
| test.cpp:261:14:261:21 | ... + ... | test.cpp:264:13:264:14 | Load: * ... |
609607
| test.cpp:261:14:261:21 | ... + ... | test.cpp:264:13:264:14 | Load: * ... |
610608
| test.cpp:261:14:261:21 | ... + ... | test.cpp:264:13:264:14 | Load: * ... |
611-
| test.cpp:262:21:262:21 | x | test.cpp:264:13:264:14 | Load: * ... |
612609
| test.cpp:262:26:262:28 | end | test.cpp:262:26:262:28 | end |
613610
| test.cpp:262:26:262:28 | end | test.cpp:262:26:262:28 | end |
614611
| test.cpp:262:26:262:28 | end | test.cpp:264:13:264:14 | Load: * ... |
615612
| test.cpp:262:26:262:28 | end | test.cpp:264:13:264:14 | Load: * ... |
616613
| test.cpp:262:31:262:31 | x | test.cpp:264:13:264:14 | Load: * ... |
617-
| test.cpp:262:31:262:33 | ... ++ | test.cpp:262:21:262:21 | x |
618-
| test.cpp:262:31:262:33 | ... ++ | test.cpp:262:21:262:21 | x |
619-
| test.cpp:262:31:262:33 | ... ++ | test.cpp:262:31:262:31 | x |
620-
| test.cpp:262:31:262:33 | ... ++ | test.cpp:262:31:262:31 | x |
621-
| test.cpp:262:31:262:33 | ... ++ | test.cpp:264:14:264:14 | x |
622-
| test.cpp:262:31:262:33 | ... ++ | test.cpp:264:14:264:14 | x |
623-
| test.cpp:262:31:262:33 | ... ++ | test.cpp:264:14:264:14 | x |
624-
| test.cpp:262:31:262:33 | ... ++ | test.cpp:264:14:264:14 | x |
625614
| test.cpp:264:14:264:14 | x | test.cpp:262:31:262:31 | x |
626615
| test.cpp:264:14:264:14 | x | test.cpp:264:13:264:14 | Load: * ... |
627616
| test.cpp:264:14:264:14 | x | test.cpp:264:13:264:14 | Load: * ... |
@@ -634,8 +623,6 @@ edges
634623
| test.cpp:271:14:271:15 | xs | test.cpp:272:26:272:28 | end |
635624
| test.cpp:271:14:271:15 | xs | test.cpp:272:26:272:28 | end |
636625
| test.cpp:271:14:271:15 | xs | test.cpp:272:31:272:31 | x |
637-
| test.cpp:271:14:271:15 | xs | test.cpp:272:31:272:33 | ... ++ |
638-
| test.cpp:271:14:271:15 | xs | test.cpp:272:31:272:33 | ... ++ |
639626
| test.cpp:271:14:271:15 | xs | test.cpp:274:5:274:6 | * ... |
640627
| test.cpp:271:14:271:15 | xs | test.cpp:274:6:274:6 | x |
641628
| test.cpp:271:14:271:15 | xs | test.cpp:274:6:274:6 | x |
@@ -649,55 +636,19 @@ edges
649636
| test.cpp:271:14:271:21 | ... + ... | test.cpp:274:5:274:10 | Store: ... = ... |
650637
| test.cpp:271:14:271:21 | ... + ... | test.cpp:274:5:274:10 | Store: ... = ... |
651638
| test.cpp:271:14:271:21 | ... + ... | test.cpp:274:5:274:10 | Store: ... = ... |
652-
| test.cpp:272:21:272:21 | x | test.cpp:274:5:274:10 | Store: ... = ... |
653639
| test.cpp:272:26:272:28 | end | test.cpp:272:26:272:28 | end |
654640
| test.cpp:272:26:272:28 | end | test.cpp:272:26:272:28 | end |
655641
| test.cpp:272:26:272:28 | end | test.cpp:274:5:274:10 | Store: ... = ... |
656642
| test.cpp:272:26:272:28 | end | test.cpp:274:5:274:10 | Store: ... = ... |
657643
| test.cpp:272:31:272:31 | x | test.cpp:274:5:274:10 | Store: ... = ... |
658-
| test.cpp:272:31:272:33 | ... ++ | test.cpp:272:21:272:21 | x |
659-
| test.cpp:272:31:272:33 | ... ++ | test.cpp:272:21:272:21 | x |
660-
| test.cpp:272:31:272:33 | ... ++ | test.cpp:272:31:272:31 | x |
661-
| test.cpp:272:31:272:33 | ... ++ | test.cpp:272:31:272:31 | x |
662-
| test.cpp:272:31:272:33 | ... ++ | test.cpp:274:5:274:6 | * ... |
663-
| test.cpp:272:31:272:33 | ... ++ | test.cpp:274:5:274:6 | * ... |
664-
| test.cpp:272:31:272:33 | ... ++ | test.cpp:274:6:274:6 | x |
665-
| test.cpp:272:31:272:33 | ... ++ | test.cpp:274:6:274:6 | x |
666-
| test.cpp:272:31:272:33 | ... ++ | test.cpp:274:6:274:6 | x |
667-
| test.cpp:272:31:272:33 | ... ++ | test.cpp:274:6:274:6 | x |
668644
| test.cpp:274:5:274:6 | * ... | test.cpp:274:5:274:10 | Store: ... = ... |
669645
| test.cpp:274:6:274:6 | x | test.cpp:272:31:272:31 | x |
670646
| test.cpp:274:6:274:6 | x | test.cpp:274:5:274:6 | * ... |
671647
| test.cpp:274:6:274:6 | x | test.cpp:274:5:274:10 | Store: ... = ... |
672648
| test.cpp:274:6:274:6 | x | test.cpp:274:5:274:10 | Store: ... = ... |
673649
| test.cpp:280:13:280:24 | new[] | test.cpp:281:14:281:15 | xs |
674-
| test.cpp:281:14:281:15 | xs | test.cpp:282:30:282:32 | ... ++ |
675-
| test.cpp:281:14:281:15 | xs | test.cpp:282:30:282:32 | ... ++ |
676-
| test.cpp:282:21:282:21 | x | test.cpp:284:13:284:14 | Load: * ... |
677-
| test.cpp:282:30:282:30 | x | test.cpp:284:13:284:14 | Load: * ... |
678-
| test.cpp:282:30:282:32 | ... ++ | test.cpp:282:21:282:21 | x |
679-
| test.cpp:282:30:282:32 | ... ++ | test.cpp:282:21:282:21 | x |
680-
| test.cpp:282:30:282:32 | ... ++ | test.cpp:282:30:282:30 | x |
681-
| test.cpp:282:30:282:32 | ... ++ | test.cpp:282:30:282:30 | x |
682-
| test.cpp:282:30:282:32 | ... ++ | test.cpp:284:14:284:14 | x |
683-
| test.cpp:282:30:282:32 | ... ++ | test.cpp:284:14:284:14 | x |
684-
| test.cpp:284:14:284:14 | x | test.cpp:284:13:284:14 | Load: * ... |
685650
| test.cpp:290:13:290:24 | new[] | test.cpp:291:14:291:15 | xs |
686651
| test.cpp:290:13:290:24 | new[] | test.cpp:292:30:292:30 | x |
687-
| test.cpp:291:14:291:15 | xs | test.cpp:292:30:292:32 | ... ++ |
688-
| test.cpp:291:14:291:15 | xs | test.cpp:292:30:292:32 | ... ++ |
689-
| test.cpp:292:21:292:21 | x | test.cpp:294:5:294:10 | Store: ... = ... |
690-
| test.cpp:292:30:292:30 | x | test.cpp:294:5:294:10 | Store: ... = ... |
691-
| test.cpp:292:30:292:32 | ... ++ | test.cpp:292:21:292:21 | x |
692-
| test.cpp:292:30:292:32 | ... ++ | test.cpp:292:21:292:21 | x |
693-
| test.cpp:292:30:292:32 | ... ++ | test.cpp:292:30:292:30 | x |
694-
| test.cpp:292:30:292:32 | ... ++ | test.cpp:292:30:292:30 | x |
695-
| test.cpp:292:30:292:32 | ... ++ | test.cpp:294:5:294:6 | * ... |
696-
| test.cpp:292:30:292:32 | ... ++ | test.cpp:294:5:294:6 | * ... |
697-
| test.cpp:292:30:292:32 | ... ++ | test.cpp:294:6:294:6 | x |
698-
| test.cpp:292:30:292:32 | ... ++ | test.cpp:294:6:294:6 | x |
699-
| test.cpp:294:5:294:6 | * ... | test.cpp:294:5:294:10 | Store: ... = ... |
700-
| test.cpp:294:6:294:6 | x | test.cpp:294:5:294:10 | Store: ... = ... |
701652
#select
702653
| test.cpp:6:14:6:15 | Load: * ... | test.cpp:4:15:4:20 | call to malloc | test.cpp:6:14:6:15 | Load: * ... | This read might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:4:15:4:20 | call to malloc | call to malloc | test.cpp:5:19:5:22 | size | size |
703654
| test.cpp:8:14:8:21 | Load: * ... | test.cpp:4:15:4:20 | call to malloc | test.cpp:8:14:8:21 | Load: * ... | This read might be out of bounds, as the pointer might be equal to $@ + $@ + 1. | test.cpp:4:15:4:20 | call to malloc | call to malloc | test.cpp:5:19:5:22 | size | size |
@@ -719,9 +670,5 @@ edges
719670
| test.cpp:232:3:232:20 | Store: ... = ... | test.cpp:231:18:231:30 | new[] | test.cpp:232:3:232:20 | Store: ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:231:18:231:30 | new[] | new[] | test.cpp:232:11:232:15 | index | index |
720671
| test.cpp:239:5:239:22 | Store: ... = ... | test.cpp:238:20:238:32 | new[] | test.cpp:239:5:239:22 | Store: ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:238:20:238:32 | new[] | new[] | test.cpp:239:13:239:17 | index | index |
721672
| test.cpp:254:9:254:16 | Store: ... = ... | test.cpp:248:24:248:30 | call to realloc | test.cpp:254:9:254:16 | Store: ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:248:24:248:30 | call to realloc | call to realloc | test.cpp:254:11:254:11 | i | i |
722-
| test.cpp:264:13:264:14 | Load: * ... | test.cpp:260:13:260:24 | new[] | test.cpp:264:13:264:14 | Load: * ... | This read might be out of bounds, as the pointer might be equal to $@ + $@ + 1. | test.cpp:260:13:260:24 | new[] | new[] | test.cpp:261:19:261:21 | len | len |
723673
| test.cpp:264:13:264:14 | Load: * ... | test.cpp:260:13:260:24 | new[] | test.cpp:264:13:264:14 | Load: * ... | This read might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:260:13:260:24 | new[] | new[] | test.cpp:261:19:261:21 | len | len |
724-
| test.cpp:274:5:274:10 | Store: ... = ... | test.cpp:270:13:270:24 | new[] | test.cpp:274:5:274:10 | Store: ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@ + 1. | test.cpp:270:13:270:24 | new[] | new[] | test.cpp:271:19:271:21 | len | len |
725674
| test.cpp:274:5:274:10 | Store: ... = ... | test.cpp:270:13:270:24 | new[] | test.cpp:274:5:274:10 | Store: ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:270:13:270:24 | new[] | new[] | test.cpp:271:19:271:21 | len | len |
726-
| test.cpp:284:13:284:14 | Load: * ... | test.cpp:280:13:280:24 | new[] | test.cpp:284:13:284:14 | Load: * ... | This read might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:280:13:280:24 | new[] | new[] | test.cpp:281:19:281:21 | len | len |
727-
| test.cpp:294:5:294:10 | Store: ... = ... | test.cpp:290:13:290:24 | new[] | test.cpp:294:5:294:10 | Store: ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:290:13:290:24 | new[] | new[] | test.cpp:291:19:291:21 | len | len |

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/test.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -281,7 +281,7 @@ void test19(unsigned len)
281281
int *end = xs + len;
282282
for (int *x = xs; x < end; x++)
283283
{
284-
int i = *x; // GOOD [FALSE POSITIVE]
284+
int i = *x; // GOOD
285285
}
286286
}
287287

@@ -291,6 +291,6 @@ void test20(unsigned len)
291291
int *end = xs + len;
292292
for (int *x = xs; x < end; x++)
293293
{
294-
*x = 0; // GOOD [FALSE POSITIVE]
294+
*x = 0; // GOOD
295295
}
296296
}

0 commit comments

Comments
 (0)