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, 91 insertions, 16 deletions

diff --git a/macros/src/codegen.rs b/macros/src/codegen.rs
index 6c1baeca..c7adbd67 100644
--- a/macros/src/codegen.rs
+++ b/macros/src/codegen.rs
@@ -94,7 +94,7 @@ pub fn app(app: &App, analysis: &Analysis) -> TokenStream {
 
     let (dispatchers_data, dispatchers) = dispatchers(&mut ctxt, &app, analysis);
 
-    let init_fn = init(&mut ctxt, &app, analysis);
+    let (init_fn, has_late_resources) = init(&mut ctxt, &app, analysis);
     let init_arg = if cfg!(feature = "timer-queue") {
         quote!(rtfm::Peripherals {
             CBP: p.CBP,
@@ -123,6 +123,30 @@ pub fn app(app: &App, analysis: &Analysis) -> TokenStream {
         })
     };
 
+    let init = &ctxt.init;
+    let init_phase = if has_late_resources {
+        let assigns = app
+            .resources
+            .iter()
+            .filter_map(|(name, res)| {
+                if res.expr.is_none() {
+                    let alias = &ctxt.statics[name];
+
+                    Some(quote!(#alias.set(res.#name);))
+                } else {
+                    None
+                }
+            })
+            .collect::<Vec<_>>();
+
+        quote!(
+            let res = #init(#init_arg);
+            #(#assigns)*
+        )
+    } else {
+        quote!(#init(#init_arg);)
+    };
+
     let post_init = post_init(&ctxt, &app, analysis);
 
     let (idle_fn, idle_expr) = idle(&mut ctxt, &app, analysis);
@@ -147,7 +171,6 @@ pub fn app(app: &App, analysis: &Analysis) -> TokenStream {
     let assertions = assertions(app, analysis);
 
     let main = mk_ident(None);
-    let init = &ctxt.init;
     quote!(
         #resources
 
@@ -185,7 +208,7 @@ pub fn app(app: &App, analysis: &Analysis) -> TokenStream {
 
             #pre_init
 
-            #init(#init_arg);
+            #init_phase
 
             #post_init
 
@@ -290,10 +313,11 @@ fn resources(ctxt: &mut Context, app: &App, analysis: &Analysis) -> proc_macro2:
     quote!(#(#items)*)
 }
 
-fn init(ctxt: &mut Context, app: &App, analysis: &Analysis) -> proc_macro2::TokenStream {
+fn init(ctxt: &mut Context, app: &App, analysis: &Analysis) -> (proc_macro2::TokenStream, bool) {
     let attrs = &app.init.attrs;
     let locals = mk_locals(&app.init.statics, true);
     let stmts = &app.init.stmts;
+    // TODO remove in v0.5.x
     let assigns = app
         .init
         .assigns
@@ -334,12 +358,47 @@ fn init(ctxt: &mut Context, app: &App, analysis: &Analysis) -> proc_macro2::Toke
         analysis,
     );
 
+    let (late_resources, late_resources_ident, ret) = if app.init.returns_late_resources {
+        // create `LateResources` struct in the root of the crate
+        let ident = mk_ident(None);
+
+        let fields = app
+            .resources
+            .iter()
+            .filter_map(|(name, res)| {
+                if res.expr.is_none() {
+                    let ty = &res.ty;
+                    Some(quote!(pub #name: #ty))
+                } else {
+                    None
+                }
+            })
+            .collect::<Vec<_>>();
+
+        let late_resources = quote!(
+            #[allow(non_snake_case)]
+            pub struct #ident {
+                #(#fields),*
+            }
+        );
+
+        (
+            Some(late_resources),
+            Some(ident),
+            Some(quote!(-> init::LateResources)),
+        )
+    } else {
+        (None, None, None)
+    };
+    let has_late_resources = late_resources.is_some();
+
     let module = module(
         ctxt,
         Kind::Init,
         !app.init.args.schedule.is_empty(),
         !app.init.args.spawn.is_empty(),
         app,
+        late_resources_ident,
     );
 
     #[cfg(feature = "timer-queue")]
@@ -365,25 +424,30 @@ fn init(ctxt: &mut Context, app: &App, analysis: &Analysis) -> proc_macro2::Toke
     let unsafety = &app.init.unsafety;
     let device = &app.args.device;
     let init = &ctxt.init;
-    quote!(
-        #module
+    (
+        quote!(
+            #late_resources
 
-        #(#attrs)*
-        #unsafety fn #init(mut core: rtfm::Peripherals) {
-            #(#locals)*
+            #module
 
-            #baseline_let
+            #(#attrs)*
+            #unsafety fn #init(mut core: rtfm::Peripherals) #ret {
+                #(#locals)*
 
-            #prelude
+                #baseline_let
 
-            let mut device = unsafe { #device::Peripherals::steal() };
+                #prelude
 
-            #start_let
+                let mut device = unsafe { #device::Peripherals::steal() };
 
-            #(#stmts)*
+                #start_let
 
-            #(#assigns)*
-        }
+                #(#stmts)*
+
+                #(#assigns)*
+            }
+        ),
+        has_late_resources,
     )
 }
 
@@ -440,6 +504,7 @@ fn module(
     schedule: bool,
     spawn: bool,
     app: &App,
+    late_resources: Option<Ident>,
 ) -> proc_macro2::TokenStream {
     let mut items = vec![];
     let mut fields = vec![];
@@ -572,6 +637,12 @@ fn module(
         Kind::Task(_) => "Software task",
     };
 
+    if let Some(late_resources) = late_resources {
+        items.push(quote!(
+            pub use super::#late_resources as LateResources;
+        ));
+    }
+
     quote!(
         #root
 
@@ -950,6 +1021,7 @@ fn idle(
             !idle.args.schedule.is_empty(),
             !idle.args.spawn.is_empty(),
             app,
+            None,
         );
 
         let unsafety = &idle.unsafety;
@@ -1004,6 +1076,7 @@ fn exceptions(ctxt: &mut Context, app: &App, analysis: &Analysis) -> Vec<proc_ma
                 !exception.args.schedule.is_empty(),
                 !exception.args.spawn.is_empty(),
                 app,
+                None,
             );
 
             #[cfg(feature = "timer-queue")]
@@ -1083,6 +1156,7 @@ fn interrupts(
             !interrupt.args.schedule.is_empty(),
             !interrupt.args.spawn.is_empty(),
             app,
+            None,
         ));
 
         #[cfg(feature = "timer-queue")]
@@ -1245,6 +1319,7 @@ fn tasks(ctxt: &mut Context, app: &App, analysis: &Analysis) -> proc_macro2::Tok
             !task.args.schedule.is_empty(),
             !task.args.spawn.is_empty(),
             app,
+            None,
         ));
 
         let attrs = &task.attrs;