Rust: Do not treat the early return of ? as returning the unwrapped value

rust/ql/lib/change-notes/2026-10-27-try-operator-early-return.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The early return taken by the `?` operator no longer makes the unwrapped value of `x?` flow out of the enclosing function. This fixes false positive results from queries such as `rust/cleartext-logging`, where a sensitive value read with `?` was reported as part of an unrelated return value.

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

@@ -144,8 +144,6 @@ private Callable getCallable(Expr expr) {
 }
 
 module Input2 implements Impl::Private::InputSig2 {
-  private import codeql.rust.controlflow.CfgNodes
-
   class SourceSinkReportingElement extends AstNode {
     DataFlowCallable getEnclosingCallable() { result.asCfgScope() = this.getEnclosingCfgScope() }
 
@@ -158,7 +156,7 @@ module Input2 implements Impl::Private::InputSig2 {
       exists(Callable c |
         sc = Impl::Private::SummaryComponent::return(_) and
         c = getCallable(this) and
-        result.getACfgNode().getASuccessor() instanceof AnnotatedExitCfgNode and
+        isReturnedValue(result) and
         result.getEnclosingCfgScope() = c
       )
     }

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

@@ -472,8 +472,17 @@ abstract class ReturnNode extends Node {
   abstract ReturnKind getKind();
 }
 
+/** Holds if the value of `n` is returned from its enclosing callable. */
+predicate isReturnedValue(AstNode n) {
+  exists(SuccessorType t | n.getACfgNode().getASuccessor(t) instanceof AnnotatedExitCfgNode |
+    // The early return of `x?` returns the `Err` or `None` held by `x`, which is
+    // a different value from the one `x?` evaluates to
+    not (n instanceof TryExpr and t instanceof ReturnSuccessor)
+  )
+}
+
 final class ExprReturnNode extends ExprNode, ReturnNode {
-  ExprReturnNode() { n.getACfgNode().getASuccessor() instanceof AnnotatedExitCfgNode }
+  ExprReturnNode() { isReturnedValue(n) }
 
   override ReturnKind getKind() { result = TNormalReturnKind() }
 }

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

@@ -1,7 +1,8 @@
 models
 | 1 | Summary: <& as core::ops::deref::Deref>::deref; Argument[self].Reference; ReturnValue; value |
 | 2 | Summary: <core::ops::arith::Mul>::mul; Argument[self,0]; ReturnValue; taint |
-| 3 | Summary: futures_executor::local_pool::block_on; Argument[0]; ReturnValue; value |
+| 3 | Summary: <core::result::Result>::unwrap; Argument[self].Field[core::result::Result::Ok(0)]; ReturnValue; value |
+| 4 | Summary: futures_executor::local_pool::block_on; Argument[0]; ReturnValue; value |
 edges
 | main.rs:12:28:14:1 | { ... } | main.rs:17:13:17:23 | get_data(...) | provenance |  |
 | main.rs:13:5:13:13 | source(...) | main.rs:12:28:14:1 | { ... } | provenance |  |
@@ -201,7 +202,7 @@ edges
 | main.rs:348:9:348:9 | c | main.rs:350:10:350:16 | await b | provenance |  |
 | main.rs:354:9:354:9 | a | main.rs:355:10:355:10 | a | provenance |  |
 | main.rs:354:13:354:55 | ...::block_on(...) | main.rs:354:9:354:9 | a | provenance |  |
-| main.rs:354:41:354:54 | async_source(...) | main.rs:354:13:354:55 | ...::block_on(...) | provenance | MaD:3 |
+| main.rs:354:41:354:54 | async_source(...) | main.rs:354:13:354:55 | ...::block_on(...) | provenance | MaD:4 |
 | main.rs:366:44:368:9 | { ... } | main.rs:403:18:403:38 | t.get_double_number() | provenance |  |
 | main.rs:366:44:368:9 | { ... } | main.rs:407:18:407:50 | ...::get_double_number(...) | provenance |  |
 | main.rs:367:13:367:29 | self.get_number() | main.rs:367:13:367:33 | ... * ... | provenance | MaD:2 |
@@ -233,6 +234,14 @@ edges
 | main.rs:433:32:433:43 | STATIC_VALUE | main.rs:433:17:433:28 | static_value | provenance |  |
 | main.rs:436:13:436:24 | STATIC_VALUE | main.rs:427:5:427:46 | static STATIC_VALUE | provenance |  |
 | main.rs:436:28:436:37 | source(...) | main.rs:436:13:436:24 | STATIC_VALUE | provenance |  |
+| main.rs:466:55:469:5 | { ... } [Ok] | main.rs:473:14:473:39 | unwrapped_value_returned(...) [Ok] | provenance |  |
+| main.rs:467:13:467:13 | a [Ok, Ok] | main.rs:468:9:468:9 | a [Ok, Ok] | provenance |  |
+| main.rs:467:48:467:65 | Ok(...) [Ok, Ok] | main.rs:467:13:467:13 | a [Ok, Ok] | provenance |  |
+| main.rs:467:51:467:64 | Ok(...) [Ok] | main.rs:467:48:467:65 | Ok(...) [Ok, Ok] | provenance |  |
+| main.rs:467:54:467:63 | source(...) | main.rs:467:51:467:64 | Ok(...) [Ok] | provenance |  |
+| main.rs:468:9:468:9 | a [Ok, Ok] | main.rs:468:9:468:10 | TryExpr [Ok] | provenance |  |
+| main.rs:468:9:468:10 | TryExpr [Ok] | main.rs:466:55:469:5 | { ... } [Ok] | provenance |  |
+| main.rs:473:14:473:39 | unwrapped_value_returned(...) [Ok] | main.rs:473:14:473:48 | ... .unwrap() | provenance | MaD:3 |
 nodes
 | main.rs:12:28:14:1 | { ... } | semmle.label | { ... } |
 | main.rs:13:5:13:13 | source(...) | semmle.label | source(...) |
@@ -488,6 +497,15 @@ nodes
 | main.rs:436:13:436:24 | STATIC_VALUE | semmle.label | STATIC_VALUE |
 | main.rs:436:28:436:37 | source(...) | semmle.label | source(...) |
 | main.rs:437:18:437:29 | STATIC_VALUE | semmle.label | STATIC_VALUE |
+| main.rs:466:55:469:5 | { ... } [Ok] | semmle.label | { ... } [Ok] |
+| main.rs:467:13:467:13 | a [Ok, Ok] | semmle.label | a [Ok, Ok] |
+| main.rs:467:48:467:65 | Ok(...) [Ok, Ok] | semmle.label | Ok(...) [Ok, Ok] |
+| main.rs:467:51:467:64 | Ok(...) [Ok] | semmle.label | Ok(...) [Ok] |
+| main.rs:467:54:467:63 | source(...) | semmle.label | source(...) |
+| main.rs:468:9:468:9 | a [Ok, Ok] | semmle.label | a [Ok, Ok] |
+| main.rs:468:9:468:10 | TryExpr [Ok] | semmle.label | TryExpr [Ok] |
+| main.rs:473:14:473:39 | unwrapped_value_returned(...) [Ok] | semmle.label | unwrapped_value_returned(...) [Ok] |
+| main.rs:473:14:473:48 | ... .unwrap() | semmle.label | ... .unwrap() |
 subpaths
 | main.rs:38:16:38:24 | source(...) | main.rs:26:28:26:33 | ...: i64 | main.rs:26:17:26:25 | SelfParam [Return] [&ref, MyStruct] | main.rs:38:5:38:5 | [post] a [MyStruct] |
 | main.rs:39:10:39:10 | a [MyStruct] | main.rs:30:17:30:21 | SelfParam [&ref, MyStruct] | main.rs:30:31:32:5 | { ... } | main.rs:39:10:39:21 | a.get_data() |
@@ -555,3 +573,4 @@ testFailures
 | main.rs:434:18:434:29 | static_value | main.rs:436:28:436:37 | source(...) | main.rs:434:18:434:29 | static_value | $@ | main.rs:436:28:436:37 | source(...) | source(...) |
 | main.rs:437:18:437:29 | STATIC_VALUE | main.rs:427:36:427:45 | source(...) | main.rs:437:18:437:29 | STATIC_VALUE | $@ | main.rs:427:36:427:45 | source(...) | source(...) |
 | main.rs:437:18:437:29 | STATIC_VALUE | main.rs:436:28:436:37 | source(...) | main.rs:437:18:437:29 | STATIC_VALUE | $@ | main.rs:436:28:436:37 | source(...) | source(...) |
+| main.rs:473:14:473:48 | ... .unwrap() | main.rs:467:54:467:63 | source(...) | main.rs:473:14:473:48 | ... .unwrap() | $@ | main.rs:467:54:467:63 | source(...) | source(...) |

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

@@ -454,3 +454,22 @@ fn main() {
     test_operator_overloading();
     test_async_await();
 }
+
+mod try_operator {
+    use super::{sink, source};
+
+    fn unwrapped_value_not_returned() -> Option<i64> {
+        let _a = Some(source(50))?;
+        Some(0)
+    }
+
+    fn unwrapped_value_returned() -> Result<i64, i64> {
+        let a: Result<Result<i64, i64>, i64> = Ok(Ok(source(51)));
+        a?
+    }
+
+    fn test_try_operator() {
+        sink(unwrapped_value_not_returned().unwrap());
+        sink(unwrapped_value_returned().unwrap()); // $ hasValueFlow=51
+    }
+}

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

@@ -145,3 +145,11 @@
 | main.rs:452:5:452:25 | data_through_method(...) | main.rs:187:1:199:1 | fn data_through_method |
 | main.rs:454:5:454:31 | test_operator_overloading(...) | main.rs:256:1:298:1 | fn test_operator_overloading |
 | main.rs:455:5:455:22 | test_async_await(...) | main.rs:353:1:358:1 | fn test_async_await |
+| main.rs:462:23:462:32 | source(...) | main.rs:1:1:3:1 | fn source |
+| main.rs:467:54:467:63 | source(...) | main.rs:1:1:3:1 | fn source |
+| main.rs:472:9:472:53 | sink(...) | main.rs:5:1:7:1 | fn sink |
+| main.rs:472:14:472:43 | unwrapped_value_not_returned(...) | main.rs:461:5:464:5 | fn unwrapped_value_not_returned |
+| main.rs:472:14:472:52 | ... .unwrap() | {EXTERNAL LOCATION} | [summarized] fn unwrap |
+| main.rs:473:9:473:49 | sink(...) | main.rs:5:1:7:1 | fn sink |
+| main.rs:473:14:473:39 | unwrapped_value_returned(...) | main.rs:466:5:469:5 | fn unwrapped_value_returned |
+| main.rs:473:14:473:48 | ... .unwrap() | {EXTERNAL LOCATION} | [summarized] fn unwrap |

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

@@ -581,3 +581,18 @@ async fn main() {
     test_neutrals();
     let dummy = Some(0); // ensure that the the `lang:core` crate is extracted
 }
+
+mod try_in_sink_callback {
+    use super::source;
+
+    // has a sink model
+    fn pass_sink_option(f: impl FnOnce(()) -> Option<i64>) {}
+
+    fn test_try_in_sink_callback() {
+        let a = |_a| {
+            let _s = Some(source(100))?;
+            Some(0)
+        };
+        pass_sink_option(a);
+    }
+}

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

@@ -26,6 +26,7 @@ extensions:
       - ["<main::MyFieldEnum>::sink", "Argument[self].Field[main::MyFieldEnum::D::field_d]", "test-sink", "manual"]
       - ["main::sink_out_of_function::pass_sink", "Argument[0].ReturnValue", "test-sink", "manual"]
       - ["main::sink_out_of_function::pass_sink_nested", "Argument[0].ReturnValue.Field[main::MyFieldEnum::E::field_e].Field[core::option::Option::Some(0)]", "test-sink", "manual"]
+      - ["main::try_in_sink_callback::pass_sink_option", "Argument[0].ReturnValue", "test-sink", "manual"]
       - ["main::external_file::generated_sink", "Argument[0]", "test-sink", "dfc-generated"]
       - ["main::external_file::neutral_generated_sink", "Argument[0]", "test-sink", "dfc-generated"]
       - ["main::external_file::neutral_manual_sink", "Argument[0]", "test-sink", "manual"]

rust/ql/test/query-tests/security/CWE-312/CleartextLogging.expected

@@ -55,6 +55,7 @@
 | test_logging.rs:245:20:245:65 | ... .as_bytes() | test_logging.rs:245:46:245:53 | password | test_logging.rs:245:20:245:65 | ... .as_bytes() | This operation writes $@ to a log file. | test_logging.rs:245:46:245:53 | password | password |
 | test_logging.rs:248:15:248:60 | ... .as_bytes() | test_logging.rs:248:41:248:48 | password | test_logging.rs:248:15:248:60 | ... .as_bytes() | This operation writes $@ to a log file. | test_logging.rs:248:41:248:48 | password | password |
 | test_logging.rs:251:15:251:60 | ... .as_bytes() | test_logging.rs:251:41:251:48 | password | test_logging.rs:251:15:251:60 | ... .as_bytes() | This operation writes $@ to a log file. | test_logging.rs:251:41:251:48 | password | password |
+| test_logging.rs:290:11:290:31 | MacroExpr | test_logging.rs:289:17:289:39 | get_password_or_error(...) | test_logging.rs:290:11:290:31 | MacroExpr | This operation writes $@ to a log file. | test_logging.rs:289:17:289:39 | get_password_or_error(...) | get_password_or_error(...) |
 edges
 | test_logging.rs:42:28:42:35 | password | test_logging.rs:42:12:42:35 | MacroExpr | provenance | Sink:MaD:11 |
 | test_logging.rs:43:28:43:35 | password | test_logging.rs:43:12:43:35 | MacroExpr | provenance | Sink:MaD:11 |
@@ -225,6 +226,8 @@ edges
 | test_logging.rs:251:23:251:48 | MacroExpr | test_logging.rs:251:23:251:48 | ...::format(...) | provenance | MaD:20 |
 | test_logging.rs:251:23:251:48 | { ... } | test_logging.rs:251:23:251:48 | ...::must_use(...) | provenance | MaD:21 |
 | test_logging.rs:251:41:251:48 | password | test_logging.rs:251:23:251:48 | MacroExpr | provenance |  |
+| test_logging.rs:289:9:289:13 | value | test_logging.rs:290:11:290:31 | MacroExpr | provenance | Sink:MaD:11 |
+| test_logging.rs:289:17:289:39 | get_password_or_error(...) | test_logging.rs:289:9:289:13 | value | provenance |  |
 models
 | 1 | Sink: <core::option::Option as log_err::LogErrOption>::log_expect; Argument[0]; log-injection |
 | 2 | Sink: <core::option::Option>::expect; Argument[0]; log-injection |
@@ -462,4 +465,7 @@ nodes
 | test_logging.rs:251:23:251:48 | MacroExpr | semmle.label | MacroExpr |
 | test_logging.rs:251:23:251:48 | { ... } | semmle.label | { ... } |
 | test_logging.rs:251:41:251:48 | password | semmle.label | password |
+| test_logging.rs:289:9:289:13 | value | semmle.label | value |
+| test_logging.rs:289:17:289:39 | get_password_or_error(...) | semmle.label | get_password_or_error(...) |
+| test_logging.rs:290:11:290:31 | MacroExpr | semmle.label | MacroExpr |
 subpaths

rust/ql/test/query-tests/security/CWE-312/test_logging.rs

@@ -271,6 +271,30 @@ fn test_collections(harmless: String) {
     password_queue.make_contiguous();
 }
 
+fn get_password_or_error() -> Result<String, std::io::Error> {
+    Ok("123456".to_string())
+}
+
+#[derive(Debug)]
+struct Outcome {
+    count: usize,
+}
+
+fn count_entries() -> Result<Outcome, std::io::Error> {
+    let _p = get_password_or_error()?;
+    Ok(Outcome { count: 1 })
+}
+
+fn test_try_operator() -> Result<(), std::io::Error> {
+    let value = get_password_or_error()?; // $ Source[rust/cleartext-logging]=try
+    info!("message = {}", value); // $ Alert[rust/cleartext-logging]=try
+
+    let outcome = count_entries()?;
+    info!("message = {:?}", outcome); // (the outcome does not contain the password)
+
+    Ok(())
+}
+
 fn main() {
     simple_logger::SimpleLogger::new().init().unwrap();
 
@@ -281,4 +305,5 @@ fn main() {
     );
     test_std("123456".to_string(), 0, None);
     test_collections("harmless".to_string());
+    _ = test_try_operator();
 }