Rust: Test and model some string and iterator methods

[geoffw0]

I decided to take a moment to test and model some string and iterator methods that I'm coming across frequently, in particular I want to get flow through the chain:

std::env::args().nth(1).unwrap().parse::<usize>().unwrap()

(std::env::args() being a flow source)

Modelling the string methods I wanted went fine (.parse and its inverse .to_string), but I don't seem to be able to produce a working MaD model (that is, one producing any flow edges) for any of the iterator methods (.iter, .next, .nth etc). I'm suspicious of the paths I've been using, for example for .iter for example I get:

getResolvedCrateOrigin: lang:core
getResolvedPath: <[_]>::iter

suggesting a model along the lines of:

- ["lang:core", "<[_]>::iter", "Argument[Self].Element", "ReturnValue.Element", "value", "manual"]

I'm also suspicious of the reference return values causing problems.

@hvitved @paldepind do you have any thoughts?

[Copilot]

PR Overview

This PR adds dataflow coverage for string parsing and iterator methods. It introduces new tests covering .parse, .to_string, and iterator usage in the local code, while also adding corresponding dataflow models in lang-core.model.yml.

• Adds tests for .parse and .to_string in main.rs
• Augments the dataflow model for parse and to_string in lang-core.model.yml
• Extends environment argument tests in sources/test.rs

Changes

File	Description
rust/ql/test/library-tests/dataflow/local/main.rs	Adds new functions to test .parse, .to_string, and iterator behavior
rust/ql/lib/codeql/rust/frameworks/stdlib/lang-core.model.yml	Adds dataflow modeling lines for parse and to_string
rust/ql/test/library-tests/dataflow/sources/test.rs	Adds an additional arg parse test case

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more

[paldepind]

I'm also suspicious of the reference return values causing problems.

The model looks fine to me and my guess would be the same as yours, that it's the & and *s that are causing problems.

[geoffw0]

I'll have a closer look at those ReferenceContents then...

[geoffw0]

Ah, it looks like there's some overlap between this and #18621 . It probably makes sense to get that merged first, then merge latest main into this, then address the problems here.

[geoffw0]

For this call to into_iter() (from the IntoIter trait), we don't seem to have a result for resolvable_resolved_paths.

[geoffw0]

@hvitved would you mind taking a quick look?

[hvitved]

resolvable_resolved_paths is populated by the extractor, so I think @redsun82 or @aibaars can better explain this.

[geoffw0]

I've created an issue to track this (to separate this concern from merging the PR). I'm keen to hear what people think we need to do.

[geoffw0]

note to self: I need to replace Variant with Field in this PR next week.

[geoffw0]

Done:

• merged main.
• dealt with Variant -> Field.
• model more of the things.
• this possibly might help: Fix interaction between defaultAdditionalTaintStep and defaultImplicitTaintRead #18776 --- it didn't.

[geoffw0]

I didn't have much luck fixing the models with data flow as it is now - I can see nothing obviously wrong with them, writing them a few different ways (including in QL) didn't seem to help, and guessing that some things should be a .Reference didn't seem to help either. So I've pushed the models as I think they should be and created an issue to come back and investigate what isn't working at some point. We're working on a couple of things that could change this up anyhow, but I really want to merge the test cases so we can see where we are and when things get better or worse.

[geoffw0]

Fixed merge conflict. There's some follow-up work, but it's all written up in issues. The sooner we merge this the better.

[paldepind]

Looks good to me :)