Rust: Refactor MaD provenance-based filtering

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

@@ -47,7 +47,7 @@ final class DataFlowCallable extends TDataFlowCallable {
 
   /** Gets a textual representation of this callable. */
   string toString() {
-    result = [this.asCfgScope().toString(), this.asSummarizedCallable().toString()]
+    result = [this.asCfgScope().toString(), "[summarized] " + this.asSummarizedCallable()]
   }
 
   /** Gets the location of this callable. */
@@ -443,25 +443,7 @@ module RustDataFlow implements InputSig<Location> {
     exists(Call c | c = call.asCall() |
       result.asCfgScope() = c.getARuntimeTarget()
       or
-      exists(SummarizedCallable sc, Function staticTarget |
-        staticTarget = getStaticTargetExt(c) and
-        sc = result.asSummarizedCallable() and
-        // Only use summarized callables with generated summaries in case
-        // the static call target is not in the source code.
-        // Note that if `applyGeneratedModel` holds it implies that there doesn't
-        // exist a manual model.
-        not (
-          staticTarget.fromSource() and
-          sc.applyGeneratedModel()
-        )
-      |
-        sc = staticTarget
-        or
-        // only apply trait models to concrete implementations when they are not
-        // defined in source code
-        staticTarget.implements(sc) and
-        not staticTarget.fromSource()
-      )
+      result.asSummarizedCallable() = getStaticTargetExt(c)
     )
   }
 

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

@@ -111,27 +111,56 @@ predicate interpretModelForTest(QlBuiltins::ExtensionId madId, string model) {
   )
 }
 
-private class SummarizedCallableFromModel extends SummarizedCallable::Range {
-  private string path;
+private predicate summaryModel(
+  Function f, string input, string output, string kind, Provenance provenance, boolean isInherited,
+  QlBuiltins::ExtensionId madId
+) {
+  exists(string path, Function f0 |
+    summaryModel(path, input, output, kind, provenance, madId) and
+    f0.getCanonicalPath() = path
+  |
+    f = f0 and
+    isInherited = false
+    or
+    f.implements(f0) and
+    isInherited = true
+  )
+}
 
-  SummarizedCallableFromModel() {
-    summaryModel(path, _, _, _, _, _) and
-    this.getCanonicalPath() = path
-  }
+private predicate summaryModelRelevant(
+  Function f, string input, string output, string kind, Provenance provenance,
+  QlBuiltins::ExtensionId madId
+) {
+  exists(boolean isInherited |
+    summaryModel(f, input, output, kind, provenance, isInherited, madId)
+  |
+    // Only apply generated or inherited models to functions in library code and
+    // when no strictly better model exists
+    if provenance.isGenerated() or isInherited = true
+    then
+      not f.fromSource() and
+      not exists(Provenance other | summaryModel(f, _, _, _, other, false, _) |
+        provenance.isGenerated() and other.isManual()
+        or
+        provenance = other and isInherited = true
+      )
+    else any()
+  )
+}
+
+private class SummarizedCallableFromModel extends SummarizedCallable::Range {
+  SummarizedCallableFromModel() { summaryModelRelevant(this, _, _, _, _, _) }
 
   override predicate hasProvenance(Provenance provenance) {
-    summaryModel(path, _, _, _, provenance, _)
+    summaryModelRelevant(this, _, _, _, provenance, _)
   }
 
-  private predicate hasManualModel() { summaryModel(path, _, _, _, "manual", _) }
-
   override predicate propagatesFlow(
     string input, string output, boolean preservesValue, string model
   ) {
-    exists(string kind, string provenance, QlBuiltins::ExtensionId madId |
-      summaryModel(path, input, output, kind, provenance, madId) and
-      model = "MaD:" + madId.toString() and
-      (provenance = "manual" or not this.hasManualModel())
+    exists(string kind, QlBuiltins::ExtensionId madId |
+      summaryModelRelevant(this, input, output, kind, _, madId) and
+      model = "MaD:" + madId.toString()
     |
       kind = "value" and
       preservesValue = true

rust/ql/lib/codeql/rust/frameworks/stdlib/alloc.model.yml

@@ -50,5 +50,8 @@ extensions:
       - ["<alloc::string::String>::as_str", "Argument[self]", "ReturnValue", "value", "manual"]
       - ["<alloc::string::String>::as_bytes", "Argument[self]", "ReturnValue", "value", "manual"]
       - ["<_ as alloc::string::ToString>::to_string", "Argument[self].Reference", "ReturnValue", "taint", "manual"]
+        # Overwrite generated model
+      - ["<alloc::string::String as core::ops::arith::Add>::add", "Argument[self,0]", "ReturnValue", "taint", "manual"]
+      - ["<alloc::string::String as core::ops::arith::Add>::add", "Argument[0].Reference", "ReturnValue", "taint", "manual"]
       # Vec
       - ["alloc::vec::from_elem", "Argument[0]", "ReturnValue.Element", "value", "manual"]

rust/ql/test/library-tests/dataflow/global/viableCallable.expected

@@ -1,4 +1,4 @@
-| main.rs:2:5:2:12 | ... + ... | {EXTERNAL LOCATION} | fn add |
+| main.rs:2:5:2:12 | ... + ... | {EXTERNAL LOCATION} | [summarized] fn add |
 | main.rs:13:5:13:13 | source(...) | main.rs:1:1:3:1 | fn source |
 | main.rs:17:13:17:23 | get_data(...) | main.rs:12:1:14:1 | fn get_data |
 | main.rs:18:5:18:11 | sink(...) | main.rs:5:1:7:1 | fn sink |
@@ -60,7 +60,7 @@
 | main.rs:228:13:228:34 | ...::new(...) | main.rs:221:5:224:5 | fn new |
 | main.rs:228:24:228:33 | source(...) | main.rs:1:1:3:1 | fn source |
 | main.rs:230:5:230:11 | sink(...) | main.rs:5:1:7:1 | fn sink |
-| main.rs:252:11:252:15 | * ... | {EXTERNAL LOCATION} | fn deref |
+| main.rs:252:11:252:15 | * ... | {EXTERNAL LOCATION} | [summarized] fn deref |
 | main.rs:258:28:258:36 | source(...) | main.rs:1:1:3:1 | fn source |
 | main.rs:260:13:260:17 | ... + ... | main.rs:236:5:239:5 | fn add |
 | main.rs:261:5:261:17 | sink(...) | main.rs:5:1:7:1 | fn sink |
@@ -77,7 +77,7 @@
 | main.rs:282:5:282:10 | ... *= ... | main.rs:243:5:245:5 | fn mul_assign |
 | main.rs:283:5:283:17 | sink(...) | main.rs:5:1:7:1 | fn sink |
 | main.rs:286:28:286:37 | source(...) | main.rs:1:1:3:1 | fn source |
-| main.rs:288:13:288:29 | * ... | {EXTERNAL LOCATION} | fn deref |
+| main.rs:288:13:288:29 | * ... | {EXTERNAL LOCATION} | [summarized] fn deref |
 | main.rs:288:14:288:29 | ...::deref(...) | main.rs:251:5:253:5 | fn deref |
 | main.rs:289:5:289:11 | sink(...) | main.rs:5:1:7:1 | fn sink |
 | main.rs:291:28:291:37 | source(...) | main.rs:1:1:3:1 | fn source |
@@ -101,14 +101,14 @@
 | main.rs:346:17:346:25 | source(...) | main.rs:1:1:3:1 | fn source |
 | main.rs:347:9:347:15 | sink(...) | main.rs:5:1:7:1 | fn sink |
 | main.rs:350:5:350:17 | sink(...) | main.rs:5:1:7:1 | fn sink |
-| main.rs:354:13:354:55 | ...::block_on(...) | {EXTERNAL LOCATION} | fn block_on |
+| main.rs:354:13:354:55 | ...::block_on(...) | {EXTERNAL LOCATION} | [summarized] fn block_on |
 | main.rs:354:41:354:54 | async_source(...) | main.rs:335:1:339:1 | fn async_source |
 | main.rs:355:5:355:11 | sink(...) | main.rs:5:1:7:1 | fn sink |
-| main.rs:357:5:357:62 | ...::block_on(...) | {EXTERNAL LOCATION} | fn block_on |
+| main.rs:357:5:357:62 | ...::block_on(...) | {EXTERNAL LOCATION} | [summarized] fn block_on |
 | main.rs:357:33:357:61 | test_async_await_async_part(...) | main.rs:341:1:351:1 | fn test_async_await_async_part |
 | main.rs:367:13:367:29 | self.get_number() | main.rs:378:9:380:9 | fn get_number |
 | main.rs:367:13:367:29 | self.get_number() | main.rs:386:9:388:9 | fn get_number |
-| main.rs:367:13:367:33 | ... * ... | {EXTERNAL LOCATION} | fn mul |
+| main.rs:367:13:367:33 | ... * ... | {EXTERNAL LOCATION} | [summarized] fn mul |
 | main.rs:371:13:371:21 | source(...) | main.rs:1:1:3:1 | fn source |
 | main.rs:379:13:379:21 | source(...) | main.rs:1:1:3:1 | fn source |
 | main.rs:391:13:391:22 | source(...) | main.rs:1:1:3:1 | fn source |

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

@@ -405,6 +405,9 @@ fn test_trait_model<T: Ord>(x: T) {
 
     let x6 = source(27) < 1;
     sink(x6); // $ hasTaintFlow=27
+
+    let x7 = (source(28) as i32) < 1;
+    sink(x7);
 }
 
 #[tokio::main]

rust/ql/test/library-tests/dataflow/models/models.ext.yml

@@ -37,3 +37,5 @@ extensions:
       - ["main::apply", "Argument[1].ReturnValue", "ReturnValue", "value", "manual"]
       - ["main::get_async_number", "Argument[0]", "ReturnValue.Future", "value", "manual"]
       - ["<_ as core::cmp::PartialOrd>::lt", "Argument[self].Reference", "ReturnValue", "taint", "manual"]
+      # Overwrites the generic trait model for i32
+      - ["<core::i32 as core::cmp::PartialOrd>::lt", "Argument[0]", "ReturnValue", "taint", "manual"]

rust/ql/test/library-tests/dataflow/strings/inline-taint-flow.expected

@@ -1,71 +1,69 @@
 models
 | 1 | Summary: <_ as alloc::string::ToString>::to_string; Argument[self].Reference; ReturnValue; taint |
 | 2 | Summary: <_ as core::convert::From>::from; Argument[0]; ReturnValue; taint |
-| 3 | Summary: <_ as core::ops::arith::Add>::add; Argument[0].Reference; ReturnValue; taint |
-| 4 | Summary: <_ as core::ops::arith::Add>::add; Argument[self]; ReturnValue; taint |
-| 5 | Summary: <_ as core::ops::index::Index>::index; Argument[self].Reference.Element; ReturnValue.Reference; value |
-| 6 | Summary: <alloc::string::String as core::convert::From>::from; Argument[0].Reference; ReturnValue; value |
-| 7 | Summary: <alloc::string::String as core::ops::arith::Add>::add; Argument[self]; ReturnValue; value |
-| 8 | Summary: <alloc::string::String>::as_str; Argument[self]; ReturnValue; value |
-| 9 | Summary: alloc::fmt::format; Argument[0]; ReturnValue; taint |
-| 10 | Summary: core::hint::must_use; Argument[0]; ReturnValue; value |
+| 3 | Summary: <_ as core::ops::index::Index>::index; Argument[self].Reference.Element; ReturnValue.Reference; value |
+| 4 | Summary: <alloc::string::String as core::convert::From>::from; Argument[0].Reference; ReturnValue; value |
+| 5 | Summary: <alloc::string::String as core::ops::arith::Add>::add; Argument[0].Reference; ReturnValue; taint |
+| 6 | Summary: <alloc::string::String as core::ops::arith::Add>::add; Argument[self,0]; ReturnValue; taint |
+| 7 | Summary: <alloc::string::String>::as_str; Argument[self]; ReturnValue; value |
+| 8 | Summary: alloc::fmt::format; Argument[0]; ReturnValue; taint |
+| 9 | Summary: core::hint::must_use; Argument[0]; ReturnValue; value |
 edges
 | main.rs:26:9:26:9 | s | main.rs:27:19:27:19 | s | provenance |  |
 | main.rs:26:9:26:9 | s | main.rs:27:19:27:25 | s[...] | provenance |  |
 | main.rs:26:13:26:22 | source(...) | main.rs:26:9:26:9 | s | provenance |  |
 | main.rs:27:9:27:14 | sliced [&ref] | main.rs:28:16:28:21 | sliced | provenance |  |
 | main.rs:27:18:27:25 | &... [&ref] | main.rs:27:9:27:14 | sliced [&ref] | provenance |  |
-| main.rs:27:19:27:19 | s | main.rs:27:19:27:25 | s[...] | provenance | MaD:5 |
+| main.rs:27:19:27:19 | s | main.rs:27:19:27:25 | s[...] | provenance | MaD:3 |
 | main.rs:27:19:27:25 | s[...] | main.rs:27:18:27:25 | &... [&ref] | provenance |  |
 | main.rs:32:9:32:10 | s1 | main.rs:35:14:35:15 | s1 | provenance |  |
 | main.rs:32:14:32:23 | source(...) | main.rs:32:9:32:10 | s1 | provenance |  |
 | main.rs:35:9:35:10 | s4 | main.rs:38:10:38:11 | s4 | provenance |  |
-| main.rs:35:14:35:15 | s1 | main.rs:35:14:35:20 | ... + ... | provenance | MaD:4 |
-| main.rs:35:14:35:15 | s1 | main.rs:35:14:35:20 | ... + ... | provenance | MaD:7 |
+| main.rs:35:14:35:15 | s1 | main.rs:35:14:35:20 | ... + ... | provenance | MaD:6 |
 | main.rs:35:14:35:20 | ... + ... | main.rs:35:9:35:10 | s4 | provenance |  |
 | main.rs:43:9:43:10 | s1 | main.rs:46:34:46:35 | s1 | provenance |  |
 | main.rs:43:14:43:23 | source(...) | main.rs:43:9:43:10 | s1 | provenance |  |
-| main.rs:46:33:46:35 | &s1 [&ref] | main.rs:46:10:46:35 | ... + ... | provenance | MaD:3 |
+| main.rs:46:33:46:35 | &s1 [&ref] | main.rs:46:10:46:35 | ... + ... | provenance | MaD:5 |
 | main.rs:46:34:46:35 | s1 | main.rs:46:33:46:35 | &s1 [&ref] | provenance |  |
 | main.rs:51:9:51:10 | s1 | main.rs:52:27:52:28 | s1 | provenance |  |
 | main.rs:51:14:51:29 | source_slice(...) | main.rs:51:9:51:10 | s1 | provenance |  |
 | main.rs:52:9:52:10 | s2 | main.rs:53:10:53:11 | s2 | provenance |  |
 | main.rs:52:14:52:29 | ...::from(...) | main.rs:52:9:52:10 | s2 | provenance |  |
 | main.rs:52:27:52:28 | s1 | main.rs:52:14:52:29 | ...::from(...) | provenance | MaD:2 |
-| main.rs:52:27:52:28 | s1 | main.rs:52:14:52:29 | ...::from(...) | provenance | MaD:6 |
+| main.rs:52:27:52:28 | s1 | main.rs:52:14:52:29 | ...::from(...) | provenance | MaD:4 |
 | main.rs:57:9:57:10 | s1 | main.rs:58:14:58:15 | s1 | provenance |  |
 | main.rs:57:14:57:29 | source_slice(...) | main.rs:57:9:57:10 | s1 | provenance |  |
 | main.rs:58:9:58:10 | s2 | main.rs:59:10:59:11 | s2 | provenance |  |
 | main.rs:58:14:58:15 | s1 | main.rs:58:14:58:27 | s1.to_string() | provenance | MaD:1 |
 | main.rs:58:14:58:27 | s1.to_string() | main.rs:58:9:58:10 | s2 | provenance |  |
 | main.rs:63:9:63:9 | s | main.rs:64:16:64:16 | s | provenance |  |
 | main.rs:63:13:63:22 | source(...) | main.rs:63:9:63:9 | s | provenance |  |
-| main.rs:64:16:64:16 | s | main.rs:64:16:64:25 | s.as_str() | provenance | MaD:8 |
+| main.rs:64:16:64:16 | s | main.rs:64:16:64:25 | s.as_str() | provenance | MaD:7 |
 | main.rs:68:9:68:9 | s | main.rs:70:34:70:61 | MacroExpr | provenance |  |
 | main.rs:68:9:68:9 | s | main.rs:73:34:73:59 | MacroExpr | provenance |  |
 | main.rs:68:13:68:22 | source(...) | main.rs:68:9:68:9 | s | provenance |  |
 | main.rs:70:9:70:18 | formatted1 | main.rs:71:10:71:19 | formatted1 | provenance |  |
 | main.rs:70:22:70:62 | ...::format(...) | main.rs:70:9:70:18 | formatted1 | provenance |  |
-| main.rs:70:34:70:61 | MacroExpr | main.rs:70:22:70:62 | ...::format(...) | provenance | MaD:9 |
+| main.rs:70:34:70:61 | MacroExpr | main.rs:70:22:70:62 | ...::format(...) | provenance | MaD:8 |
 | main.rs:73:9:73:18 | formatted2 | main.rs:74:10:74:19 | formatted2 | provenance |  |
 | main.rs:73:22:73:60 | ...::format(...) | main.rs:73:9:73:18 | formatted2 | provenance |  |
-| main.rs:73:34:73:59 | MacroExpr | main.rs:73:22:73:60 | ...::format(...) | provenance | MaD:9 |
+| main.rs:73:34:73:59 | MacroExpr | main.rs:73:22:73:60 | ...::format(...) | provenance | MaD:8 |
 | main.rs:76:9:76:13 | width | main.rs:77:34:77:74 | MacroExpr | provenance |  |
 | main.rs:76:17:76:32 | source_usize(...) | main.rs:76:9:76:13 | width | provenance |  |
 | main.rs:77:9:77:18 | formatted3 | main.rs:78:10:78:19 | formatted3 | provenance |  |
 | main.rs:77:22:77:75 | ...::format(...) | main.rs:77:9:77:18 | formatted3 | provenance |  |
-| main.rs:77:34:77:74 | MacroExpr | main.rs:77:22:77:75 | ...::format(...) | provenance | MaD:9 |
+| main.rs:77:34:77:74 | MacroExpr | main.rs:77:22:77:75 | ...::format(...) | provenance | MaD:8 |
 | main.rs:82:9:82:10 | s1 | main.rs:86:18:86:25 | MacroExpr | provenance |  |
 | main.rs:82:9:82:10 | s1 | main.rs:87:18:87:32 | MacroExpr | provenance |  |
 | main.rs:82:14:82:23 | source(...) | main.rs:82:9:82:10 | s1 | provenance |  |
 | main.rs:86:18:86:25 | ...::format(...) | main.rs:86:18:86:25 | { ... } | provenance |  |
 | main.rs:86:18:86:25 | ...::must_use(...) | main.rs:86:10:86:26 | MacroExpr | provenance |  |
-| main.rs:86:18:86:25 | MacroExpr | main.rs:86:18:86:25 | ...::format(...) | provenance | MaD:9 |
-| main.rs:86:18:86:25 | { ... } | main.rs:86:18:86:25 | ...::must_use(...) | provenance | MaD:10 |
+| main.rs:86:18:86:25 | MacroExpr | main.rs:86:18:86:25 | ...::format(...) | provenance | MaD:8 |
+| main.rs:86:18:86:25 | { ... } | main.rs:86:18:86:25 | ...::must_use(...) | provenance | MaD:9 |
 | main.rs:87:18:87:32 | ...::format(...) | main.rs:87:18:87:32 | { ... } | provenance |  |
 | main.rs:87:18:87:32 | ...::must_use(...) | main.rs:87:10:87:33 | MacroExpr | provenance |  |
-| main.rs:87:18:87:32 | MacroExpr | main.rs:87:18:87:32 | ...::format(...) | provenance | MaD:9 |
-| main.rs:87:18:87:32 | { ... } | main.rs:87:18:87:32 | ...::must_use(...) | provenance | MaD:10 |
+| main.rs:87:18:87:32 | MacroExpr | main.rs:87:18:87:32 | ...::format(...) | provenance | MaD:8 |
+| main.rs:87:18:87:32 | { ... } | main.rs:87:18:87:32 | ...::must_use(...) | provenance | MaD:9 |
 nodes
 | main.rs:26:9:26:9 | s | semmle.label | s |
 | main.rs:26:13:26:22 | source(...) | semmle.label | source(...) |

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

@@ -35,7 +35,7 @@ fn string_add() {
     let s4 = s1 + s3;
     let s5 = s2 + s3;
 
-    sink(s4); // $ SPURIOUS: hasValueFlow=83 MISSING: hasTaintFlow=83
+    sink(s4); // $ hasTaintFlow=83
     sink(s5);
 }