[MathiasVP]

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:

codeql/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/TaintTrackingUtil.qll

Lines 212 to 229 in 682545f

	// 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:

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:

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.

Additionally, Copilot flagged up a problem with the new asExpr rule causing multiple DataFlow::Nodes' asExpr to return the same expression for reference-typed qualifies. So that required a fix in 13a9724, and fixing that one revealed a bug in Parameter <-> DataFlow::Node mapping which affected Coding Standards: Node.asParameter returned a non-indirect parameter most of the time, but for references it returns the indirect parameter (since that's most often what you want), but the DataFlow::parameterNode predicate's return type required the node to be a direct parameter node. So this was fixed in a7eb87a.

github/codeql-coding-standards#1218 should be merged once this PR goes in.

[Copilot]

🟡 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

• Map reference-conversion chains to only one qualifier node · New
• Add focused member-call qualifier mapping regressions · New
• Provide DCA results for the final mapping change · New

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)}

[jketema]

I've a hard time following this, especially since the example you give seems many-to-one (there's only one expression in the example *x).

[MathiasVP]

Oh, right. I can see how this is a bit confusing. Let me clear up things in the conversation here, and then we can figure out how to improve the comments.

The instruction-to-node mapping is many-to-one, but the node-to-expression mapping is one-to-many. This does play a role here:

• Since the instruction-to-node mapping is many-to-one a single node can correspond to multiple instructions, and one of those can be a glvalue.
• Since the node-to-expression mapping is one-to-many a single node may correspond to multiple expression, and one of those can be a glvalue.

In the (*x).f() example we have:

• One node (representing the qualifier) consisting of three instructions:

  r1(glval<Foo *>) = VariableAddress[x] :
  r2(Foo *)        = Load[x]            : &:r1
  r3(glval<Foo>)   = CopyValue          : r2
  

• Two relevant expressions: x and *x. The r2 instruction maps to x, and r3 maps to *x. So the combined mapping (i.e., node-to-instruction) maps the node to both x and *x, and only one of those is a glvalue.

Does that make it more clear? I'm still trying to figure out how to articulate this in a code comment 😅

[jketema]

That makes it more clear indeed.

[MathiasVP]

I've slightly rewritten it in 4b9c587. Let me know if that transfers the necessary information to the comment! 🤞