Skip to content

C++: Don't count every conversion as a use #11976

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 4 commits into from
Jan 30, 2023
Merged

C++: Don't count every conversion as a use #11976

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7ecc346
C++: Add another column to 'conversionFlow'.
MathiasVP Jan 27, 2023
9de8d5c
C++: Reduce the number of uses by excluding conversions.
MathiasVP Jan 27, 2023
1b45c5f
C++: Accept test changes.
MathiasVP Jan 27, 2023
7cc7675
C++: Accept query changes. These are just deduplications.
MathiasVP Jan 30, 2023
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
8 changes: 4 additions & 4 deletions cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ class ReturnIndirectionNode extends IndirectReturnNode, ReturnNode {
private Operand fullyConvertedCallStep(Operand op) {
not exists(getANonConversionUse(op)) and
exists(Instruction instr |
conversionFlow(op, instr, _) and
conversionFlow(op, instr, _, _) and
result = getAUse(instr)
)
}
Expand All @@ -397,7 +397,7 @@ Operand getAUse(Instruction instr) {
*/
private Instruction getANonConversionUse(Operand operand) {
result = getUse(operand) and
not conversionFlow(_, result, _)
not conversionFlow(_, result, _, _)
}

/**
Expand Down Expand Up @@ -555,7 +555,7 @@ private predicate numberOfLoadsFromOperandRec(Operand operandFrom, Operand opera
or
exists(Operand op, Instruction instr |
instr = op.getDef() and
conversionFlow(operandFrom, instr, _) and
conversionFlow(operandFrom, instr, _, _) and
numberOfLoadsFromOperand(op, operandTo, ind)
)
}
Expand All @@ -568,7 +568,7 @@ private predicate numberOfLoadsFromOperand(Operand operandFrom, Operand operandT
numberOfLoadsFromOperandRec(operandFrom, operandTo, n)
or
not Ssa::isDereference(_, operandFrom) and
not conversionFlow(operandFrom, _, _) and
not conversionFlow(operandFrom, _, _, _) and
operandFrom = operandTo and
n = 0
}
Expand Down
28 changes: 19 additions & 9 deletions cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,22 +77,32 @@ class FieldAddress extends Operand {
*
* `isPointerArith` is `true` if `instrTo` is a `PointerArithmeticInstruction` and `opFrom`
* is the left operand.
*
* `additional` is `true` if the conversion is supplied by an implementation of the
* `Indirection` class. It is sometimes useful to exclude such conversions.
*/
predicate conversionFlow(Operand opFrom, Instruction instrTo, boolean isPointerArith) {
predicate conversionFlow(
Operand opFrom, Instruction instrTo, boolean isPointerArith, boolean additional
) {
isPointerArith = false and
(
instrTo.(CopyValueInstruction).getSourceValueOperand() = opFrom
or
instrTo.(ConvertInstruction).getUnaryOperand() = opFrom
or
instrTo.(CheckedConvertOrNullInstruction).getUnaryOperand() = opFrom
or
instrTo.(InheritanceConversionInstruction).getUnaryOperand() = opFrom
additional = false and
(
instrTo.(CopyValueInstruction).getSourceValueOperand() = opFrom
or
instrTo.(ConvertInstruction).getUnaryOperand() = opFrom
or
instrTo.(CheckedConvertOrNullInstruction).getUnaryOperand() = opFrom
or
instrTo.(InheritanceConversionInstruction).getUnaryOperand() = opFrom
)
or
additional = true and
Ssa::isAdditionalConversionFlow(opFrom, instrTo)
)
or
isPointerArith = true and
additional = false and
instrTo.(PointerArithmeticInstruction).getLeftOperand() = opFrom
}

Expand Down Expand Up @@ -1365,7 +1375,7 @@ private module Cached {

private predicate simpleInstructionLocalFlowStep(Operand opFrom, Instruction iTo) {
// Treat all conversions as flow, even conversions between different numeric types.
conversionFlow(opFrom, iTo, false)
conversionFlow(opFrom, iTo, false, _)
or
iTo.(CopyInstruction).getSourceValueOperand() = opFrom
}
Expand Down
2 changes: 1 addition & 1 deletion cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,7 +469,7 @@ private predicate indirectConversionFlowStep(Node nFrom, Node nTo) {
hasOperandAndIndex(nFrom, op1, pragma[only_bind_into](indirectionIndex)) and
hasOperandAndIndex(nTo, op2, pragma[only_bind_into](indirectionIndex)) and
instr = op2.getDef() and
conversionFlow(op1, instr, _)
conversionFlow(op1, instr, _, _)
)
}

Expand Down
18 changes: 15 additions & 3 deletions cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternalsCommon.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ private module IteratorIndirections {

override predicate isAdditionalDereference(Instruction deref, Operand address) {
exists(CallInstruction call |
operandForfullyConvertedCall(deref.getAUse(), call) and
operandForfullyConvertedCall(getAUse(deref), call) and
this = call.getStaticCallTarget().getClassAndName("operator*") and
address = call.getThisArgumentOperand()
)
Expand Down Expand Up @@ -585,6 +585,15 @@ private module Cached {
)
}

/** Holds if `op` is the only use of its defining instruction, and that op is used in a conversation */
private predicate isConversion(Operand op) {
exists(Instruction def, Operand use |
def = op.getDef() and
use = unique( | | getAUse(def)) and
conversionFlow(use, _, false, false)
)
}

/**
* Holds if `op` is a use of an SSA variable rooted at `base` with `ind` number
* of indirections.
Expand All @@ -602,6 +611,9 @@ private module Cached {
type = getLanguageType(op) and
upper = countIndirectionsForCppType(type) and
isUseImpl(op, base, ind0) and
// Don't count every conversion as their own use. Instead, only the first
// use (i.e., before any conversions are applied) will count as a use.
not isConversion(op) and
ind = ind0 + [0 .. upper] and
indirectionIndex = ind - ind0
)
Expand Down Expand Up @@ -652,7 +664,7 @@ private module Cached {
exists(Operand mid, Instruction instr |
isUseImpl(mid, base, ind) and
instr = operand.getDef() and
conversionFlow(mid, instr, false)
conversionFlow(mid, instr, false, _)
)
or
exists(int ind0 |
Expand Down Expand Up @@ -722,7 +734,7 @@ private module Cached {
exists(Operand mid, Instruction instr, boolean certain0, boolean isPointerArith |
isDefImpl(mid, base, ind, certain0) and
instr = operand.getDef() and
conversionFlow(mid, instr, isPointerArith) and
conversionFlow(mid, instr, isPointerArith, _) and
if isPointerArith = true then certain = false else certain = certain0
)
or
Expand Down
2 changes: 1 addition & 1 deletion cpp/ql/test/library-tests/dataflow/dataflow-tests/clang.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,5 +48,5 @@ void following_pointers(

int stackArray[2] = { source(), source() };
stackArray[0] = source();
sink(stackArray); // $ ast ir ir=49:35 ir=50:19
sink(stackArray); // $ ast ir
}
4 changes: 2 additions & 2 deletions cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -533,12 +533,12 @@ void test_set_through_const_pointer(int *e)
}

void sink_then_source_1(int* p) {
sink(*p); // $ SPURIOUS: ir=537:10 ir=547:9
sink(*p); // $ ir // Flow from the unitialized x to the dereference.
*p = source();
}

void sink_then_source_2(int* p, int y) {
sink(y); // $ SPURIOUS: ast ir=542:10 ir=551:9
sink(y); // $ SPURIOUS: ast ir
*p = source();
}

Expand Down
5 changes: 0 additions & 5 deletions cpp/ql/test/library-tests/dataflow/fields/ir-path-flow.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ edges
| A.cpp:31:20:31:20 | c | A.cpp:31:14:31:21 | call to B [c] |
| A.cpp:41:15:41:21 | new | A.cpp:43:10:43:12 | & ... indirection |
| A.cpp:41:15:41:21 | new | A.cpp:43:10:43:12 | & ... indirection |
| A.cpp:41:15:41:21 | new | A.cpp:43:10:43:12 | & ... indirection |
| A.cpp:41:15:41:21 | new | A.cpp:43:10:43:12 | & ... indirection |
| A.cpp:47:12:47:18 | new | A.cpp:48:20:48:20 | c |
| A.cpp:48:12:48:18 | call to make indirection [c] | A.cpp:49:10:49:10 | b indirection [c] |
| A.cpp:48:20:48:20 | c | A.cpp:29:23:29:23 | c |
Expand Down Expand Up @@ -907,7 +905,6 @@ nodes
| A.cpp:41:15:41:21 | new | semmle.label | new |
| A.cpp:41:15:41:21 | new | semmle.label | new |
| A.cpp:43:10:43:12 | & ... indirection | semmle.label | & ... indirection |
| A.cpp:43:10:43:12 | & ... indirection | semmle.label | & ... indirection |
| A.cpp:47:12:47:18 | new | semmle.label | new |
| A.cpp:48:12:48:18 | call to make indirection [c] | semmle.label | call to make indirection [c] |
| A.cpp:48:20:48:20 | c | semmle.label | c |
Expand Down Expand Up @@ -1765,8 +1762,6 @@ subpaths
#select
| A.cpp:43:10:43:12 | & ... indirection | A.cpp:41:15:41:21 | new | A.cpp:43:10:43:12 | & ... indirection | & ... indirection flows from $@ | A.cpp:41:15:41:21 | new | new |
| A.cpp:43:10:43:12 | & ... indirection | A.cpp:41:15:41:21 | new | A.cpp:43:10:43:12 | & ... indirection | & ... indirection flows from $@ | A.cpp:41:15:41:21 | new | new |
| A.cpp:43:10:43:12 | & ... indirection | A.cpp:41:15:41:21 | new | A.cpp:43:10:43:12 | & ... indirection | & ... indirection flows from $@ | A.cpp:41:15:41:21 | new | new |
| A.cpp:43:10:43:12 | & ... indirection | A.cpp:41:15:41:21 | new | A.cpp:43:10:43:12 | & ... indirection | & ... indirection flows from $@ | A.cpp:41:15:41:21 | new | new |
| A.cpp:49:10:49:13 | c | A.cpp:47:12:47:18 | new | A.cpp:49:10:49:13 | c | c flows from $@ | A.cpp:47:12:47:18 | new | new |
| A.cpp:49:13:49:13 | c | A.cpp:47:12:47:18 | new | A.cpp:49:13:49:13 | c | c flows from $@ | A.cpp:47:12:47:18 | new | new |
| A.cpp:56:10:56:17 | call to get | A.cpp:55:12:55:19 | new | A.cpp:56:10:56:17 | call to get | call to get flows from $@ | A.cpp:55:12:55:19 | new | new |
Expand Down
Loading