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, 59 insertions, 5 deletions

diff --git a/macros/src/syntax.rs b/macros/src/syntax.rs
index 85f3caaa..ad7d8bde 100644
--- a/macros/src/syntax.rs
+++ b/macros/src/syntax.rs
@@ -596,6 +596,7 @@ impl Parse for InitArgs {
     }
 }
 
+// TODO remove in v0.5.x
 pub struct Assign {
     pub attrs: Vec<Attribute>,
     pub left: Ident,
@@ -608,32 +609,83 @@ pub struct Init {
     pub unsafety: Option<Token![unsafe]>,
     pub statics: HashMap<Ident, Static>,
     pub stmts: Vec<Stmt>,
+    // TODO remove in v0.5.x
     pub assigns: Vec<Assign>,
+    pub returns_late_resources: bool,
 }
 
 impl Init {
     fn check(args: InitArgs, item: ItemFn) -> parse::Result<Self> {
-        let valid_signature = item.vis == Visibility::Inherited
+        let mut valid_signature = item.vis == Visibility::Inherited
             && item.constness.is_none()
             && item.asyncness.is_none()
             && item.abi.is_none()
             && item.decl.generics.params.is_empty()
             && item.decl.generics.where_clause.is_none()
             && item.decl.inputs.is_empty()
-            && item.decl.variadic.is_none()
-            && is_unit(&item.decl.output);
+            && item.decl.variadic.is_none();
+
+        let returns_late_resources = match &item.decl.output {
+            ReturnType::Default => false,
+            ReturnType::Type(_, ty) => {
+                match &**ty {
+                    Type::Tuple(t) => {
+                        if t.elems.is_empty() {
+                            // -> ()
+                            true
+                        } else {
+                            valid_signature = false;
+
+                            false // don't care
+                        }
+                    }
+
+                    Type::Path(p) => {
+                        let mut segments = p.path.segments.iter();
+                        if p.qself.is_none()
+                            && p.path.leading_colon.is_none()
+                            && p.path.segments.len() == 2
+                            && segments.next().map(|s| {
+                                s.arguments == PathArguments::None && s.ident.to_string() == "init"
+                            }) == Some(true)
+                            && segments.next().map(|s| {
+                                s.arguments == PathArguments::None
+                                    && s.ident.to_string() == "LateResources"
+                            }) == Some(true)
+                        {
+                            // -> init::LateResources
+                            true
+                        } else {
+                            valid_signature = false;
+
+                            false // don't care
+                        }
+                    }
+
+                    _ => {
+                        valid_signature = false;
+
+                        false // don't care
+                    }
+                }
+            }
+        };
 
         let span = item.span();
 
         if !valid_signature {
             return Err(parse::Error::new(
                 span,
-                "`init` must have type signature `[unsafe] fn()`",
+                "`init` must have type signature `[unsafe] fn() [-> init::LateResources]`",
             ));
         }
 
         let (statics, stmts) = extract_statics(item.block.stmts);
-        let (stmts, assigns) = extract_assignments(stmts);
+        let (stmts, assigns) = if returns_late_resources {
+            (stmts, vec![])
+        } else {
+            extract_assignments(stmts)
+        };
 
         Ok(Init {
             args,
@@ -642,6 +694,7 @@ impl Init {
             statics: Static::parse(statics)?,
             stmts,
             assigns,
+            returns_late_resources,
         })
     }
 }
@@ -1207,6 +1260,7 @@ fn extract_statics(stmts: Vec<Stmt>) -> (Statics, Vec<Stmt>) {
     (statics, stmts)
 }
 
+// TODO remove in v0.5.x
 fn extract_assignments(stmts: Vec<Stmt>) -> (Vec<Stmt>, Vec<Assign>) {
     let mut istmts = stmts.into_iter().rev();