diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll index 19140653877..4f60db4cc00 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll @@ -7,6 +7,9 @@ private import SsaInternals as Ssa private import DataFlowImplCommon as DataFlowImplCommon private import codeql.util.Unit private import Node0ToString +private import ModelUtil +private import semmle.code.cpp.models.interfaces.FunctionInputsAndOutputs as IO +private import semmle.code.cpp.models.interfaces.DataFlow as DF cached private module Cached { @@ -1178,6 +1181,19 @@ private int countNumberOfBranchesUsingParameter(SwitchInstruction switch, Parame ) } +pragma[nomagic] +private predicate isInputOutput( + DF::DataFlowFunction target, Node node1, Node node2, IO::FunctionInput input, + IO::FunctionOutput output +) { + exists(CallInstruction call | + node1 = callInput(call, input) and + node2 = callOutput(call, output) and + call.getStaticCallTarget() = target and + target.hasDataFlow(input, output) + ) +} + /** * Holds if the data-flow step from `node1` to `node2` can be used to * determine where side-effects may return from a callable. @@ -1189,6 +1205,11 @@ private int countNumberOfBranchesUsingParameter(SwitchInstruction switch, Parame * int x = *p; * ``` * does not preserve the identity of `*p`. + * + * Similarly, functions that copy the contents of a string into a new location + * does not also preserve the identity. For example, `strdup(p)` does not + * preserve the identity of `*p` (since it allocates new storage and copies + * the string into the new storage). */ bindingset[node1, node2] pragma[inline_late] @@ -1225,7 +1246,16 @@ predicate validParameterAliasStep(Node node1, Node node2) { not exists(Operand operand | node1.asOperand() = operand and node2.asInstruction().(StoreInstruction).getSourceValueOperand() = operand + ) and + ( + // Either this is not a modeled flow. + not isInputOutput(_, node1, node2, _, _) + or + exists(DF::DataFlowFunction target, IO::FunctionInput input, IO::FunctionOutput output | + // Or it is a modeled flow and there's `*input` to `*output` flow + isInputOutput(target, node1, node2, input.getIndirectionInput(), output.getIndirectionOutput()) and + // and in that case there should also be `input` to `output` flow + target.hasDataFlow(input, output) + ) ) - // TODO: Also block flow through models that don't preserve identity such - // as `strdup`. } diff --git a/cpp/ql/lib/semmle/code/cpp/models/interfaces/FunctionInputsAndOutputs.qll b/cpp/ql/lib/semmle/code/cpp/models/interfaces/FunctionInputsAndOutputs.qll index e7bcd1ec4bd..9c8ee43b52a 100644 --- a/cpp/ql/lib/semmle/code/cpp/models/interfaces/FunctionInputsAndOutputs.qll +++ b/cpp/ql/lib/semmle/code/cpp/models/interfaces/FunctionInputsAndOutputs.qll @@ -23,6 +23,14 @@ private newtype TFunctionInput = class FunctionInput extends TFunctionInput { abstract string toString(); + /** + * INTERNAL: Do not use. + * + * Gets the `FunctionInput` that represents the indirection of this input, + * if any. + */ + FunctionInput getIndirectionInput() { none() } + /** * Holds if this is the input value of the parameter with index `index`. * @@ -226,6 +234,8 @@ class InParameter extends FunctionInput, TInParameter { ParameterIndex getIndex() { result = index } override predicate isParameter(ParameterIndex i) { i = index } + + override FunctionInput getIndirectionInput() { result = TInParameterDeref(index, 1) } } /** @@ -257,6 +267,10 @@ class InParameterDeref extends FunctionInput, TInParameterDeref { override predicate isParameterDeref(ParameterIndex i, int indirection) { i = index and indirectionIndex = indirection } + + override FunctionInput getIndirectionInput() { + result = TInParameterDeref(index, indirectionIndex + 1) + } } /** @@ -275,6 +289,8 @@ class InQualifierObject extends FunctionInput, TInQualifierObject { override string toString() { result = "InQualifierObject" } override predicate isQualifierObject() { any() } + + override FunctionInput getIndirectionInput() { none() } } /** @@ -293,6 +309,8 @@ class InQualifierAddress extends FunctionInput, TInQualifierAddress { override string toString() { result = "InQualifierAddress" } override predicate isQualifierAddress() { any() } + + override FunctionInput getIndirectionInput() { result = TInQualifierObject() } } /** @@ -321,6 +339,8 @@ class InReturnValueDeref extends FunctionInput, TInReturnValueDeref { override string toString() { result = "InReturnValueDeref" } override predicate isReturnValueDeref() { any() } + + override FunctionInput getIndirectionInput() { none() } } private newtype TFunctionOutput = @@ -340,6 +360,14 @@ private newtype TFunctionOutput = class FunctionOutput extends TFunctionOutput { abstract string toString(); + /** + * INTERNAL: Do not use. + * + * Gets the `FunctionOutput` that represents the indirection of this output, + * if any. + */ + FunctionOutput getIndirectionOutput() { none() } + /** * Holds if this is the output value pointed to by a pointer parameter to a function, or the * output value referred to by a reference parameter to a function, where the parameter has @@ -512,6 +540,10 @@ class OutParameterDeref extends FunctionOutput, TOutParameterDeref { override predicate isParameterDeref(ParameterIndex i, int ind) { i = index and ind = indirectionIndex } + + override FunctionOutput getIndirectionOutput() { + result = TOutParameterDeref(index, indirectionIndex + 1) + } } /** @@ -530,6 +562,8 @@ class OutQualifierObject extends FunctionOutput, TOutQualifierObject { override string toString() { result = "OutQualifierObject" } override predicate isQualifierObject() { any() } + + override FunctionOutput getIndirectionOutput() { none() } } /** @@ -552,6 +586,8 @@ class OutReturnValue extends FunctionOutput, TOutReturnValue { override string toString() { result = "OutReturnValue" } override predicate isReturnValue() { any() } + + override FunctionOutput getIndirectionOutput() { result = TOutReturnValueDeref(1) } } /** @@ -571,11 +607,19 @@ class OutReturnValue extends FunctionOutput, TOutReturnValue { * of `getInt()` is neither a pointer nor a reference. */ class OutReturnValueDeref extends FunctionOutput, TOutReturnValueDeref { + int indirectionIndex; + + OutReturnValueDeref() { this = TOutReturnValueDeref(indirectionIndex) } + override string toString() { result = "OutReturnValueDeref" } override predicate isReturnValueDeref() { any() } - override predicate isReturnValueDeref(int indirectionIndex) { - this = TOutReturnValueDeref(indirectionIndex) + override predicate isReturnValueDeref(int indirectionIndex_) { + indirectionIndex = indirectionIndex_ + } + + override FunctionOutput getIndirectionOutput() { + result = TOutReturnValueDeref(indirectionIndex + 1) } }