Skip to content

C++: Fix Expr <-> DataFlow::Node mapping for glvalue qualifiers - #23046

Open
MathiasVP wants to merge 2 commits into
github:mainfrom
MathiasVP:fix-gl-value-as-expr
Open

MathiasVP wants to merge 2 commits into
github:mainfrom
MathiasVP:fix-gl-value-as-expr

Conversation

@MathiasVP

@MathiasVP MathiasVP commented Nov 8, 2026 •

Copy link
Copy Markdown
Contributor

This PR fixes a special case in the Expr <-> DataFlow::Node mapping: When the qualifier instruction is a glvalue the resulting DataFlow::Node for which asExpr equals the qualifier expression was the InstructionNode for the instruction, whereas it should be the IndirectInstructionNode.

I noticed this problem in #22939 where we lost a result that I added as a testcase in #22990. It is essentially this situation:

void test_source_glvalue(FILE* log, std::string password)
{
  const char* pw = password.c_str(); // password is clearly sensitive information
  fprintf(log, "%s", pw); // and we log it as cleartext
}

On main it currently “works” because taint-tracking has this ad-hoc rule which conflates pointers and their indirection:

// Taint flow from a pointer argument to an output, when the model specifies flow from the deref
// to that output, but the deref is not modeled in the IR for the caller.
exists(
CallInstruction call, SideEffectOperandNode indirectArgument, Function func,
FunctionInput modelIn, FunctionOutput modelOut
|
indirectArgument = callInput(call, modelIn) and
indirectArgument.hasAddressOperandAndIndirectionIndex(nodeIn.asOperand(), _) and
call.getStaticCallTarget() = func and
(
func.(DataFlowFunction).hasDataFlow(modelIn, modelOut) and
model = "DataFlowFunction"
or
func.(TaintFunction).hasTaintFlow(modelIn, modelOut) and
model = "TaintFunction"
) and
nodeOut = callOutput(call, modelOut)
)

I want to get rid of this disjunct at some point, but until we fix some more pointer/pointee conflation it has to stay (stay tuned for another upcoming PR!).

On main this is the path we get for that test with debug-printing enabled:

Pasted Graphic 1

the r153_3 instruction is a glvalue-producing instruction which computes a pointer to the password variable. So the c_str step steps from a glvalue<std::string> expression (the qualifier glvalue instruction) to a const char expression (the indirection of the resulting const char* expression).

When I disable the conflation rule and runs the query on this database again we get:

Pasted Graphic

That is, we now start at the indirection of the qualifier (i.e., a std::string expression) and the c_str model step gets us to a const char expression (the indirection of the resulting const char* expression), as expected.

Because of the above conflation there shouldn’t really be any changes to any results. However, since toString is defined in terms of which expressions a node maps to, there are lots of changes to .expected files.

DCA was uneventful. There's a small change in a Coding Standards query's .expected file that needs to be accepted. See the backlinked PR

@MathiasVP MathiasVP added C++ no-change-note-required This PR does not need a change note labels Nov 8, 2026
@MathiasVP
MathiasVP force-pushed the fix-gl-value-as-expr branch from 47a1463 to 4c0bb1d Compare November 8, 2026 17:04
@MathiasVP
MathiasVP marked this pull request as ready for review November 8, 2026 18:08
@MathiasVP
MathiasVP requested a review from a team as a code owner November 8, 2026 18:08
Copilot AI auto review requested due to automatic review settings November 8, 2026 18:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The reference-conversion mapping can associate the same qualifier with both the reference address and the object, causing queries to select both instead of a single intended node.

3 open issues
What changed in this PR

Corrects C++ expression-to-data-flow-node mapping for glvalue member-call qualifiers, distinguishing object values from their addresses. This supports more precise standard-library modeling while retaining the existing pointer/pointee taint rule.

Changes:

  • Maps eligible glvalue qualifiers to indirect nodes while preserving temporary-materialization handling.
  • Updates expected node and path labels affected by the mapping change.
File Description
cpp/​ql/​test/​query-tests/​Security/​CWE/​CWE-611/​XXE.expected Updates qualifier labels in XXE query paths.
cpp/​ql/​test/​library-tests/​dataflow/​fields/​ir-path-flow.expected Updates qualifier labels in field-flow paths.
cpp/​ql/​test/​library-tests/​dataflow/​external-models/​steps.expected Updates labels for modeled data-flow steps.
cpp/​ql/​lib/​semmle/​code/​cpp/​ir/​dataflow/​internal/​ExprNodes.qll Adds glvalue qualifier mapping logic and explanatory comments.

Review effort: Max (auto)

// `x`.
operand.isGLValue() and
operand instanceof ThisArgumentOperand and
e.isGLValueCategory()
// `x`.
operand.isGLValue() and
operand instanceof ThisArgumentOperand and
e.isGLValueCategory()
Comment on lines +269 to +271
operand.isGLValue() and
operand instanceof ThisArgumentOperand and
e.isGLValueCategory()

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C++ no-change-note-required This PR does not need a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants