Merge #140

140: fix soundness issue: forbid early returns in init r=japaric a=japaric

TL;DR

1. v0.4.1 will be published once this PR lands

2. v0.4.0 will be yanked once v0.4.1 is out

3. v0.4.1 will reject code that contains early returns in `init` *and* contains
   late resources. Yes, this is a breaking change but such code is unsound /
   has undefined behavior.

4. as of v0.4.1 users are encouraged to use `fn init() -> init::LateResources`
   instead of `fn init()` when they make use of late resources.

---

This PR fixes a soundness issue reported by @RalfJung. Basically, early
returning from `init` leaves *late resources* (runtime initialized statics)
uninitialized, and this produces undefined behavior as tasks rely on those
statics being initialized. The example below showcases a program that runs into
this soundness issue.

``` rust
 #[rtfm::app(device = lm3s6965)]
const APP: () = {
    // this is actually `static mut UNINITIALIZED: MaybeUninit<bool> = ..`
    static mut UNINITIALIZED: bool = ();

    #[init]
    fn init() {
        // early return
        return;

        // this is translated into `UNINITIALIZED.set(true)`
        UNINITIALIZED = true; // the DSL forces you to write this at the end
    }

    #[interrupt(resources = [UNINITIALIZED])]
    fn UART0() {
        // `resources.UNINITIALIZED` is basically `UNINITIALIZED.get_mut()`

        if resources.UNINITIALIZED {
            // undefined behavior
        }
    }
};
```

The fix consists of two parts. The first part is producing a compiler error
whenever the `app` procedural macro finds a `return` expression in `init`. This
covers most cases, except for macros (e.g. `ret!()` expands into `return`) which
cannot be instrospected by procedural macros. This fix is technically a
breaking change (though unlikely to affect real code out there) but as per our
SemVer policy (which follows rust-lang/rust's) we are allowed to make breaking
changes to fix soundness bugs.

The second part of the fix consists of extending the `init` syntax to let the
user return the initial values of late resources in a struct. Namely, `fn() ->
init::LateResources` will become a valid signature for `init` (we allowed this
signature back in v0.3.x). Thus the problematic code shown above can be
rewritten as:

``` rust
 #[rtfm::app(device = lm3s6965)]
const APP: () = {
    static mut UNINITIALIZED: bool = ();

    #[init]
    fn init() -> init::LateResources {
        // rejected by the compiler
        // return; //~ ERROR expected `init::LateResources`, found `()`

        // initialize late resources
        init::LateResources {
            UNINITIALIZED: true,
        }
    }

    #[interrupt(resources = [UNINITIALIZED])]
    fn UART0() {
        if resources.UNINITIALIZED {
            // OK
        }
    }
};
```

Attempting to early return without giving the initial values for late resources
will produce a compiler error.

~~Additionally, we'll emit warnings if the `init: fn()` signature is used to
encourage users to switch to the alternative `init: fn() -> init::LateResources`
signature.~~ Turns out we can't do this on stable. Bummer.

The book and examples have been updated to make use of `init::LateResources`.

In the next minor version release we'll reject `fn init()` if late resources
are declared. `fn init() -> init::LateResources` will become the only way to
initialize late resources.

This PR also prepares release v0.4.1. Once that version is published the unsound
version v0.4.0 will be yanked.


Co-authored-by: Jorge Aparicio <jorge@japaric.io>

1 files changed, 268 insertions, 12 deletions

diff --git a/macros/src/check.rs b/macros/src/check.rs
index ae2262a..045d152 100644
--- a/macros/src/check.rs
+++ b/macros/src/check.rs
@@ -1,7 +1,7 @@
 use std::{collections::HashSet, iter};
 
 use proc_macro2::Span;
-use syn::parse;
+use syn::{parse, spanned::Spanned, Block, Expr, Stmt};
 
 use crate::syntax::App;
 
@@ -35,17 +35,20 @@ pub fn app(app: &App) -> parse::Result<()> {
         }
     }
 
-    // Check that all late resources have been initialized in `#[init]`
-    for res in app
-        .resources
-        .iter()
-        .filter_map(|(name, res)| if res.expr.is_none() { Some(name) } else { None })
-    {
-        if app.init.assigns.iter().all(|assign| assign.left != *res) {
-            return Err(parse::Error::new(
-                res.span(),
-                "late resources MUST be initialized at the end of `init`",
-            ));
+    // Check that all late resources have been initialized in `#[init]` if `init` has signature
+    // `fn()`
+    if !app.init.returns_late_resources {
+        for res in app
+            .resources
+            .iter()
+            .filter_map(|(name, res)| if res.expr.is_none() { Some(name) } else { None })
+        {
+            if app.init.assigns.iter().all(|assign| assign.left != *res) {
+                return Err(parse::Error::new(
+                    res.span(),
+                    "late resources MUST be initialized at the end of `init`",
+                ));
+            }
         }
     }
 
@@ -112,5 +115,258 @@ pub fn app(app: &App) -> parse::Result<()> {
         }
     }
 
+    // Check that `init` contains no early returns *if* late resources exist and `init` signature is
+    // `fn()`
+    if app.resources.values().any(|res| res.expr.is_none()) {
+        if !app.init.returns_late_resources {
+            for stmt in &app.init.stmts {
+                noreturn_stmt(stmt)?;
+            }
+        }
+    } else if app.init.returns_late_resources {
+        return Err(parse::Error::new(
+            Span::call_site(),
+            "`init` signature must be `[unsafe] fn()` if there are no late resources",
+        ));
+    }
+
+    Ok(())
+}
+
+// checks that the given block contains no instance of `return`
+fn noreturn_block(block: &Block) -> Result<(), parse::Error> {
+    for stmt in &block.stmts {
+        noreturn_stmt(stmt)?;
+    }
+
+    Ok(())
+}
+
+// checks that the given statement contains no instance of `return`
+fn noreturn_stmt(stmt: &Stmt) -> Result<(), parse::Error> {
+    match stmt {
+        // `let x = ..` -- this may contain a return in the RHS
+        Stmt::Local(local) => {
+            if let Some(ref init) = local.init {
+                noreturn_expr(&init.1)?
+            }
+        }
+
+        // items have no effect on control flow
+        Stmt::Item(..) => {}
+
+        Stmt::Expr(expr) => noreturn_expr(expr)?,
+
+        Stmt::Semi(expr, ..) => noreturn_expr(expr)?,
+    }
+
+    Ok(())
+}
+
+// checks that the given expression contains no `return`
+fn noreturn_expr(expr: &Expr) -> Result<(), parse::Error> {
+    match expr {
+        Expr::Box(b) => noreturn_expr(&b.expr)?,
+
+        Expr::InPlace(ip) => {
+            noreturn_expr(&ip.place)?;
+            noreturn_expr(&ip.value)?;
+        }
+
+        Expr::Array(a) => {
+            for elem in &a.elems {
+                noreturn_expr(elem)?;
+            }
+        }
+
+        Expr::Call(c) => {
+            noreturn_expr(&c.func)?;
+
+            for arg in &c.args {
+                noreturn_expr(arg)?;
+            }
+        }
+
+        Expr::MethodCall(mc) => {
+            noreturn_expr(&mc.receiver)?;
+
+            for arg in &mc.args {
+                noreturn_expr(arg)?;
+            }
+        }
+
+        Expr::Tuple(t) => {
+            for elem in &t.elems {
+                noreturn_expr(elem)?;
+            }
+        }
+
+        Expr::Binary(b) => {
+            noreturn_expr(&b.left)?;
+            noreturn_expr(&b.right)?;
+        }
+
+        Expr::Unary(u) => {
+            noreturn_expr(&u.expr)?;
+        }
+
+        Expr::Lit(..) => {}
+
+        Expr::Cast(c) => {
+            noreturn_expr(&c.expr)?;
+        }
+
+        Expr::Type(t) => {
+            noreturn_expr(&t.expr)?;
+        }
+
+        Expr::Let(l) => {
+            noreturn_expr(&l.expr)?;
+        }
+
+        Expr::If(i) => {
+            noreturn_expr(&i.cond)?;
+
+            noreturn_block(&i.then_branch)?;
+
+            if let Some(ref e) = i.else_branch {
+                noreturn_expr(&e.1)?;
+            }
+        }
+
+        Expr::While(w) => {
+            noreturn_expr(&w.cond)?;
+            noreturn_block(&w.body)?;
+        }
+
+        Expr::ForLoop(fl) => {
+            noreturn_expr(&fl.expr)?;
+            noreturn_block(&fl.body)?;
+        }
+
+        Expr::Loop(l) => {
+            noreturn_block(&l.body)?;
+        }
+
+        Expr::Match(m) => {
+            noreturn_expr(&m.expr)?;
+
+            for arm in &m.arms {
+                if let Some(g) = &arm.guard {
+                    noreturn_expr(&g.1)?;
+                }
+
+                noreturn_expr(&arm.body)?;
+            }
+        }
+
+        // we don't care about `return`s inside closures
+        Expr::Closure(..) => {}
+
+        Expr::Unsafe(u) => {
+            noreturn_block(&u.block)?;
+        }
+
+        Expr::Block(b) => {
+            noreturn_block(&b.block)?;
+        }
+
+        Expr::Assign(a) => {
+            noreturn_expr(&a.left)?;
+            noreturn_expr(&a.right)?;
+        }
+
+        Expr::AssignOp(ao) => {
+            noreturn_expr(&ao.left)?;
+            noreturn_expr(&ao.right)?;
+        }
+
+        Expr::Field(f) => {
+            noreturn_expr(&f.base)?;
+        }
+
+        Expr::Index(i) => {
+            noreturn_expr(&i.expr)?;
+            noreturn_expr(&i.index)?;
+        }
+
+        Expr::Range(r) => {
+            if let Some(ref f) = r.from {
+                noreturn_expr(f)?;
+            }
+
+            if let Some(ref t) = r.to {
+                noreturn_expr(t)?;
+            }
+        }
+
+        Expr::Path(..) => {}
+
+        Expr::Reference(r) => {
+            noreturn_expr(&r.expr)?;
+        }
+
+        Expr::Break(b) => {
+            if let Some(ref e) = b.expr {
+                noreturn_expr(e)?;
+            }
+        }
+
+        Expr::Continue(..) => {}
+
+        Expr::Return(r) => {
+            return Err(parse::Error::new(
+                r.span(),
+                "`init` is *not* allowed to early return",
+            ));
+        }
+
+        // we can not analyze this
+        Expr::Macro(..) => {}
+
+        Expr::Struct(s) => {
+            for field in &s.fields {
+                noreturn_expr(&field.expr)?;
+            }
+
+            if let Some(ref rest) = s.rest {
+                noreturn_expr(rest)?;
+            }
+        }
+
+        Expr::Repeat(r) => {
+            noreturn_expr(&r.expr)?;
+            noreturn_expr(&r.len)?;
+        }
+
+        Expr::Paren(p) => {
+            noreturn_expr(&p.expr)?;
+        }
+
+        Expr::Group(g) => {
+            noreturn_expr(&g.expr)?;
+        }
+
+        Expr::Try(t) => {
+            noreturn_expr(&t.expr)?;
+        }
+
+        // we don't care about `return`s inside async blocks
+        Expr::Async(..) => {}
+
+        Expr::TryBlock(tb) => {
+            noreturn_block(&tb.block)?;
+        }
+
+        Expr::Yield(y) => {
+            if let Some(expr) = &y.expr {
+                noreturn_expr(expr)?;
+            }
+        }
+
+        // we can not analyze this
+        Expr::Verbatim(..) => {}
+    }
+
     Ok(())
 }