Rust: Adopt shared flow summaries library #18130

hvitved · 2024-11-27T13:23:39Z

Introduces the class SummarizedCallable for defining flow summaries in QL. Models-as-data will be added follow-up.

rust/ql/test/library-tests/dataflow/models/main.rs

+    1000 + i
+}
+
+fn sink(s: i64) {


rust/ql/test/library-tests/dataflow/models/main.rs

+}
+
+// has a flow model
+fn identity(i: i64) -> i64 {


rust/ql/test/library-tests/dataflow/models/main.rs

+}
+
+// has a flow model
+fn get_var_pos(e: MyPosEnum) -> i64 {


rust/ql/test/library-tests/dataflow/models/main.rs

+}
+
+// has a flow model
+fn set_var_pos(i: i64) -> MyPosEnum {


rust/ql/test/library-tests/dataflow/models/main.rs

+}
+
+// has a flow model
+fn get_var_field(e: MyFieldEnum) -> i64 {


rust/ql/test/library-tests/dataflow/models/main.rs

+}
+
+// has a flow model
+fn set_var_field(i: i64) -> MyFieldEnum {


rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll

+      result.asLibraryCallable() = this.getSummarizedCallable()
+    }
+
+    override EmptyLocation getLocation() { any() }


rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll

+    (
+      LocalFlow::localFlowStepCommon(nodeFrom, nodeTo)
+      or
+      exists(SsaImpl::DefinitionExt def, boolean isUseStep |


paldepind

Looks really great. Super nice to get flow through unwrap.

I've left a few nitpick comments.

paldepind · 2024-12-02T14:26:55Z

rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll

@@ -490,6 +638,13 @@ private module Aliases {
  class ContentSetAlias = ContentSet;
 }

+pragma[nomagic]
+Resolvable getCallResolvable(CallExprBase call) {


This duplicates the private getCallResolvable on CallExprBase. I think we should make that public or remove the duplication some other way.

paldepind · 2024-12-02T14:32:52Z

rust/ql/lib/codeql/rust/dataflow/FlowSummary.qll

+private module Summaries {
+  private import codeql.rust.Frameworks
+
+  // TODO: Used models-as-data when it's available


Suggested change

// TODO: Used models-as-data when it's available

// TODO: Use models-as-data when it's available

paldepind · 2024-12-02T14:47:35Z

rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll

+      exists(SsaImpl::DefinitionExt def, boolean isUseStep |
+        SsaFlow::localFlowStep(def, nodeFrom, nodeTo, isUseStep)
+      |
+        isUseStep = false
+        or
+        isUseStep = true and
+        not FlowSummaryImpl::Private::Steps::prohibitsUseUseFlow(nodeFrom, _)
+      )


A bit shorter:

Suggested change

exists(SsaImpl::DefinitionExt def, boolean isUseStep |

SsaFlow::localFlowStep(def, nodeFrom, nodeTo, isUseStep)

|

isUseStep = false

or

isUseStep = true and

not FlowSummaryImpl::Private::Steps::prohibitsUseUseFlow(nodeFrom, _)

)

exists(boolean isUseStep | SsaFlow::localFlowStep(_, nodeFrom, nodeTo, isUseStep) |

isUseStep = true

implies

not FlowSummaryImpl::Private::Steps::prohibitsUseUseFlow(nodeFrom, _)

)

While it preserves semantics, it will introduce unnecessary tuple duplication. a implies b is syntactic sugar for not a or b, so the rewrite becomes

SsaFlow::localFlowStep(def, nodeFrom, nodeTo, isUseStep) and not (isUseStep = true) or SsaFlow::localFlowStep(def, nodeFrom, nodeTo, isUseStep) and not FlowSummaryImpl::Private::Steps::prohibitsUseUseFlow(nodeFrom, _)

Note how isUseStep = true is not part of the last disjunct.

This is a bit hard for me to wrap my head around. If isUseStep = true is not the case then the first disjunct will always hold, so I would've thought that adding it to the last disjunct wouldn't make a difference. But I guess that's only the "preserves semantics" part.

Is it something along the lines of: For a disjunction P or Q all the tuples satisfying P will be created, then all tuples satisfying Q will be created, and only then will they be joined together (and de-duplicated). With my rewrite Q could be a larger stream of tuples overlapping with P, and hence the result will be the same but more de-duplication is going on?

I guess turning def into _ is still applicable.

Is it something along the lines of: For a disjunction P or Q all the tuples satisfying P will be created, then all tuples satisfying Q will be created, and only then will they be joined together (and de-duplicated). With my rewrite Q could be a larger stream of tuples overlapping with P, and hence the result will be the same but more de-duplication is going on?

Exactly.

I oversaw the _ part; that is certainly valid, let's do it follow-up.

paldepind · 2024-12-02T14:49:36Z

rust/ql/test/library-tests/dataflow/local/main.rs

@@ -126,6 +126,11 @@ fn option_pattern_match_unqualified() {
    }
 }

+fn option_unwrap() {
+    let s1 = Some(source(19));
+    sink(s1.unwrap()); // $ hasValueFlow=19


paldepind · 2024-12-02T15:17:57Z

rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll

-  final class ReturnNode extends ExprNode {
-    ReturnNode() { this.getCfgNode().getASuccessor() instanceof AnnotatedExitCfgNode }
+  abstract class ReturnNode extends Node {
+    abstract ReturnKind getKind();


Do we think we'll get additional return kinds for Rust? If not, could we leave this as it was before and remove kind in getCall, etc?

Who knows what Future Rust brings :-) If you prefer, I can change it back.

:D My preference would be to await the Rust future and just keep it simple for now without carrying around the return kind, but it obviously doesn't matter much.

Actually, given that all the other comments are addressed I think that we should just move forward and merge instead of changing this back.

paldepind · 2024-12-02T15:24:40Z

rust/ql/lib/codeql/rust/dataflow/internal/FlowSummaryImpl.qll

+    result = pos.getPosition().toString()
+    or
+    pos.isSelf() and
+    result = "self"


This is the same as the toString implementation on ParameterPosition.

Suggested change

result = pos.getPosition().toString()

or

pos.isSelf() and

result = "self"

result = pos.toString()

paldepind · 2024-12-02T15:40:08Z

rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll

@@ -148,6 +173,26 @@ module Node {
    override Location getLocation() { none() }
  }

+  /** A data-flow node used to model flow summaries. */


Suggested change

/** A data-flow node used to model flow summaries. */

/** A data flow node used to model flow summaries. */

paldepind · 2024-12-02T15:53:12Z

rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll

+    override CfgScope getCfgScope() { result = PostUpdateNode.super.getCfgScope() }

-    final override Location getLocation() { result = n.getLocation() }
+    override EmptyLocation getLocation() { result = PostUpdateNode.super.getLocation() }


I think the overloading is a bit convoluted here. PostUpdateNode.super.getLocation ends up calling getLocation on the pre node which is a FlowSummaryNode whose getLocation returns and empty location. Why not just call FlowSummaryNode.super.getLocation directly? And in that case, we don't need the getCfgScope and getLocation inside PostUpdateNode. These two can be moved to ArgumentPostUpdateNode and then these two overrides can just be deleted.

github-actions bot added the Rust Pull requests that update Rust code label Nov 27, 2024

github-advanced-security bot found potential problems Nov 27, 2024

View reviewed changes

hvitved force-pushed the rust/flow-summary-impl branch from 5812246 to 93367bb Compare November 27, 2024 13:44

github-actions bot added C# Java Ruby DataFlow Library labels Nov 27, 2024

hvitved force-pushed the rust/flow-summary-impl branch from 06355a0 to 3bb1ae0 Compare November 27, 2024 14:41

hvitved added the no-change-note-required This PR does not need a change note label Nov 27, 2024

hvitved marked this pull request as ready for review November 27, 2024 14:42

hvitved requested review from a team as code owners November 27, 2024 14:42

hvitved force-pushed the rust/flow-summary-impl branch 2 times, most recently from 7c47ade to e6e4f92 Compare December 2, 2024 09:59

paldepind requested changes Dec 2, 2024

View reviewed changes

hvitved added 4 commits December 3, 2024 09:11

Rust: Adopt shared flow summaries library

395901b

Shared: Move shared logic into FlowSummaryImpl.qll

fbeb6f3

Rust: Add a model for unwrap

3e5f4b7

Address review comments

52dc79e

hvitved force-pushed the rust/flow-summary-impl branch from e6e4f92 to 52dc79e Compare December 3, 2024 08:28

hvitved requested a review from paldepind December 3, 2024 08:28

paldepind approved these changes Dec 3, 2024

View reviewed changes

hvitved merged commit 0bebfa6 into github:main Dec 3, 2024
58 checks passed

hvitved deleted the rust/flow-summary-impl branch December 3, 2024 09:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rust: Adopt shared flow summaries library #18130

Rust: Adopt shared flow summaries library #18130

hvitved commented Nov 27, 2024 •

edited

Loading

paldepind left a comment

paldepind Dec 2, 2024

paldepind Dec 2, 2024

paldepind Dec 2, 2024

hvitved Dec 3, 2024 •

edited

Loading

paldepind Dec 3, 2024

hvitved Dec 3, 2024

hvitved Dec 3, 2024

paldepind Dec 2, 2024

paldepind Dec 2, 2024

hvitved Dec 3, 2024

paldepind Dec 3, 2024

paldepind Dec 3, 2024

paldepind Dec 2, 2024

paldepind Dec 2, 2024

paldepind Dec 2, 2024

	// TODO: Used models-as-data when it's available
	// TODO: Use models-as-data when it's available

	/** A data-flow node used to model flow summaries. */
	/** A data flow node used to model flow summaries. */

Rust: Adopt shared flow summaries library #18130

Rust: Adopt shared flow summaries library #18130

Conversation

hvitved commented Nov 27, 2024 • edited Loading

paldepind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hvitved Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hvitved commented Nov 27, 2024 •

edited

Loading

hvitved Dec 3, 2024 •

edited

Loading