Skip to content

FlowSummaryImpl: Embed AST nodes in source/sink summary nodes for better flow paths - #22145

Open
hvitved wants to merge 15 commits into
github:mainfrom
hvitved:flow-summary-source-sink-locations
Open

FlowSummaryImpl: Embed AST nodes in source/sink summary nodes for better flow paths#22145
hvitved wants to merge 15 commits into
github:mainfrom
hvitved:flow-summary-source-sink-locations

Conversation

@hvitved

@hvitved hvitved commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

This PR addresses the issues identified by @MathiasVP on #22113.

Note that the issue (and fix) only applies to Rust, which is the only language that currently supports source/sink definitions with non-empty access paths.

The underlying issue was that we were unable to map source/sink summary nodes to the locations represented by the output/input models-as-data specs, so for example a source with an Argument[0] spec would be unable to be mapped to the corresponding argument, and we instead resorted to using the location of the call as the source location. The second issue identified by @MathiasVP, defining a source that is supposed to be a parameter, was not even supported in Rust.

The fix to both issues is to embed language-specific AST nodes into the source/sink summary nodes, and then use the locations of those as the locations of the source/sink nodes. This PR also shows how to add support for parameter sources, as well as more complex sinks like Argument[0].ReturnValue.Field[A].Field[B] (a value stored inside B stored inside A, which is returned from a callback at position 0).

For source/sink specs with complex access paths like the one above, we include a data flow node for each of the access path tokens, which means we can get much more helpful flow paths:

let a = |a| {
    let s = source(1);
//          ^^^^^^^^^ source (both before and after)
    return s;
//         ^ sink before, intermediate node after
};

// a sink with access path `Argument[0].ReturnValue`
complex_sink(a);
//           ^ sink after

Commit-by-commit review is strongly encouraged, and the second commit should be reviewed ignoring whitespaces.

The impact for Rust is that some alert locations have changed (to more precise locations, and in alignment with other languages), and I have added a change note for this.

Comment thread rust/ql/lib/codeql/rust/dataflow/internal/FlowSummaryImpl.qll Fixed
Comment thread shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll Fixed
@hvitved
hvitved force-pushed the flow-summary-source-sink-locations branch from b24377a to d2107ce Compare July 9, 2026 11:51
@github-actions github-actions Bot added Rust Pull requests that update Rust code DataFlow Library labels Jul 9, 2026
@hvitved
hvitved force-pushed the flow-summary-source-sink-locations branch from d2107ce to d397063 Compare July 9, 2026 12:42
@hvitved
hvitved force-pushed the flow-summary-source-sink-locations branch 3 times, most recently from 61ebe22 to fddb4f0 Compare July 10, 2026 13:40
@hvitved
hvitved force-pushed the flow-summary-source-sink-locations branch 5 times, most recently from bb0f3dc to 332303a Compare August 4, 2026 10:20
Comment thread shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll Fixed
Comment thread shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll Fixed
@hvitved
hvitved force-pushed the flow-summary-source-sink-locations branch 3 times, most recently from 8196e39 to bca818e Compare August 5, 2026 12:21
@hvitved
hvitved force-pushed the flow-summary-source-sink-locations branch from bca818e to 74ec8fd Compare August 5, 2026 12:47
@hvitved
hvitved force-pushed the flow-summary-source-sink-locations branch from db6ff1d to 9c03c21 Compare August 6, 2026 11:46
@hvitved
hvitved marked this pull request as ready for review August 6, 2026 12:28
@hvitved
hvitved requested review from a team as code owners August 6, 2026 12:28
Comment thread cpp/ql/lib/semmle/code/cpp/dataflow/internal/FlowSummaryImpl.qll
Comment thread shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll Outdated
@hvitved

hvitved commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Ping @geoffw0 for the Rust alert location changes (improvements).

@geoffw0 geoffw0 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.

I'm happy with the changes to Rust tests, including locations. They're pretty wide-spread, I can see why you went with a majorAnalysis change note. I'm still checking some details of the DCA run...

@geoffw0 geoffw0 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.

Sorry for the delay, I am happy with all this now (at least the bits I reviewed - Rust test changes and DCA run).

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants