Disallow panics from the start

[jettr]

I think it would be nice if this project disallowed panics from the very start of the implementation. There are a couple of tricks you can do with #[panic_handler] to help ensure there are no panics. See https://github.com/chipsalliance/caliptra-sw/blob/ce64c4a6/runtime/src/main.rs#L106 and https://github.com/chipsalliance/caliptra-sw/blob/ce64c4a6/fmc/test-fw/test-rt/tests/test_panic_missing.rs for some ideas.

Disallowing panics makes the implementation smaller since you do not have to carry around panic formatting code and it makes it more resilient.

The reason to start early with this, is because it often changes the way slices are accesses throughout the code. For example, buffer[..HEADER_SIZE] may or may not generate a panic under the hood depending on what kinds of checks precede that line and how much the optimizer can reason about the size of buffer. The compiler won't emit a panic if you first check the bounds (e.g. if buf.len() < HEADER_SIZE { return; }) before the access; otherwise you can access buffer with buffer.get(..HEADER_SIZE) to avoid a panic. Using get also forces the caller to handle the case when the buffer isn't large enough.

What thoughts does the team have on this topic?

[bradlitterell]

Excuse the rust neophyte question - what exactly does it mean to disallow panics? Does it mean the code won't compile if it might panic? (in which case I agree), or do panics get ignored?

[jettr]

No problem at all!

There really isn't a first-class way to disable panics, but you can have build-time checks to ensure that the binary being produced does not have panics in it (and fail the build if panics are detected). Whether the compiler will generate a panic or not does depend on how inlining and various other compiler optimization steps interact with each other; so we couldn't guarantee that there were no panics for all permutations of compiler optimization and compilers, but we could at least guarantee that there were no panics present in a "default" build (where we get to define what the default build is). Making the default build have no panics would suffice for pretty much all practical purposes.

[bradlitterell]

I am 110% in favor of having the compiler do as much checking as possible, but want to document the corner cases you alluded to... since it seems possible, though unlikely, for panics to still happen, what will a consumer experience if we use these disable panic schemes?

[jettr]

Is the consumer someone that is using this library downstream or is an end-user using a device that employs this library?

For downstream users of this library, they would get source code that had little to no panics for whichever architecture that they were ultimately building for. Each consumer would need to implement their own build-time panic check for their specific build though (if they were interested in that). They will likely not find any panics in the upstream tpm-rs code. If they do however find a panic in upstream tpm-rs code, we should encourage them to upstream a fix to removal that panic path as that should help others. Downstream consumers could also be compiling there code with debug optimizations, and in that scenario tpm-rs code may actually have panics (since in-lining, etc. changes quite a bit for debug) when they compile, but that downstream consumer wouldn't be enabling their no-panic check for a debug build as that doesn't really make sense.

For end-users, it would depend on what kind of checks the downstream user enabled for their build. It is possible that the final sw/fw image contains panics which would cause a crash. Having the upstream policy to prevent panics in the default build should still go a long way to preventing many panic paths even in the downstream built binary.

[bradlitterell]

From this project's perspective, I use user/consumer interchangeably to mean the developer using our library to create either a TPM device or a TPM client software stack.

that downstream consumer wouldn't be enabling their no-panic check for a debug build as that doesn't really make sense.

This makes me think I don't understand no-panic. However, when I looked up Panic, it seems to be in the std library, and I think we want to specify no_std. (see #6)

[jettr]

I definitely agree with specifying #[no_std], especially if we want to target downstream consumers that are using this library on embedded controllers.

The panic! macro is available in core as well as std. But this discussion is more about preventing the implicitly added panic paths by having a build-time failure if a panic is introduced into the code. For example, the following code produces an implicit panic path that would get expressed in the final binary

fn parse_header(buffer: &[u8]) -> u8 {
   buffer[0]
}

However the following examples do not have implicit panic paths

fn parse_header_1(buffer: &[u8]) -> Result<u8> {
  buffer.get(0).ok_or(Err(())
}

fn parse_header_2(buffer: &[u8]) -> Result<u8> {
   if buffer.len() < 1 {
      return Err(());
   }
  Ok(buffer[0])
}

fn parse_header_3(buffer: &[u8 : 16]) -> u8 {
   buffer[0]
}

If should be much easier to start off this project by enforcing, with a build-time check, that we don't introduce implicit (or explicit) panics, than to come back later and try to remove implicit/explicit panics from the codebase. Since as you can see from the above examples, handling the out-of-bound case explicitly can change the function signature.

[bradlitterell]

Thanks for the examples that helps. I like the suggestion of making the error handling explicit. How exactly do we specify the no-panic enforcement for the example you gave?

[bradlitterell]

... and to go back to the question of what we want, and what the optimizing compiler actually gives us...

Could/How would optimization create a panic-path in any of the 3 good examples?

[jettr]

The examples I gave were a little small to see how the optimizer can affect things. See below:

fn main() {
  let buffer = [0u8; 16];
  let _x = parse_inline_never(& buffer);
  let _x = parse_inline_always(& buffer);
  let _x = parse(& buffer);
}

#[inline(never)]
fn parse_inline_never(b: &[u8]) -> u8 {
  b[0]
}

#[inline(always)]
fn parse_inline_always(b: &[u8]) -> u8 {
  b[0]
}

fn parse(b: &[u8]) -> u8 {
  b[0]
}

The parse_inline_always should never create a panic path because the compiler knows the size is 16 and that accessing 0 will be in bounds, while the parse_inline_never should create an implicit panic path. The parse may or may not have an implicit panic path depending on the inline threshold and how many times parse is called versus how much code is in parse, etc. For most release builds, the above simple parse function above would actually be inlined and a panic would not be generated. If the function were more complicated and called from multiple locations, then it would not be inlined (and therefore a panic would be implicitly added). The build-time check to ensure there are no panics would catch this and require the developer to update the function signature or refator the code to ensure there was no implicit panic path.

[bradlitterell]

Seems you're saying that the optimizer can dispense with the panic when inlining, so did you mean to write "For most release builds, the parse function above would actually be inlined and a panic would NOT be generated."?

[jettr]

Whoops. I edited my above comment. You are correct. I left out the not and completely changed the meaning :)

[bradlitterell]

Blame it on auto-correct; I do. 😁😁

[bradlitterell]

thanks for the explanation.

[daprilik]

I'm the author of gdbstub, which is a Rust #[no_std] project that works really hard to avoid introducing additional panic paths into projects that use the library, primarily for binary-size reasons.

In my experience, the biggest hurdle to enforcing a no_panic policy is that it's almost impossible to guarantee a particular binary won't have any panic paths in it across multiple optimization levels / architectures / compiler versions.

That is to say: unless you forgo using many of the helpful built-in APIs that ship with the Rust core library (not talking about std, since this would be a no_std project), and opt to roll every single helper method from scratch to be panic-free... you're almost certainly going to run into core code that includes assert!, debug_assert!, unwrap(), and explicit panic!() calls.

Depending on the optimization level + how clever the compiler happens to be that day, these panicking calls may be optimized out, but there's no way to know for-sure (outside of building the code + using something like #[no_panic], or examining the generated asm) to ensure these panics get optimized out.

Working on gdbstub, I often run into cases where updating to a new version of Rust introduces new panicking code into my test binaries, often due to a subtle tweak in how a core method was implemented, or a compiler optimization pass regression. Tracking down the root cause of these regressions is always a manual, time consuming process...

---

All that is to say: if no-panic is a property tpm-rs is seriously interested in, be aware of the many knock-on effects the decision will have:

• Guaranteeing no-panic for all platforms isn't feasible with the state of the Rust language + tooling today, so a set of "tier 1" configurations should be established from the outset which will have CI enforce no-panic on
• tpm-rs will not be able to use any of the built-in Rust format!() machinery, as (last I checked), it will insert unoptimizable panic paths into the binary. Some alternative formatting lib will need to be used. e.g: ufmt
• tpm-rs will be limited as to what third-party dependencies it can use (though, I would have expected this to be the case regardless)
• Compiler updates will likely require playing panic whack-a-mole, where the only way to get the panic to go away is be "contorting" the code to something rustc + LLVM can reason about

Lastly, it should be noted that no-panic code may end up looking quite "alien" when compared to the equivalent idiomatic Rust code.

It would be wise to get consensus from interested parties around whether or not such a strict no-panic policy is even required, as opposed to adopting less extreme methods of avoiding panics (e.g: fuzzing all public entry points into the TPM to ensure no payload can trigger a panic path, enforcing proper error-handling etiquette in code review, etc...)

[bradlitterell]

Hi Daniel, Thanks a ton for the detailed input here!

At the end of the day, the behavior we need from a TPM is either:

(a) panics are impossible - i.e. build fails, or
(b) any panics easily get routed to a single place where we can put the TPM into "Failure Mode".

I had hoped (a) was possible to guarantee, but sounds like there is not, so is there an idiomatic way we can guarantee that all panics get handled and wind up safely, reliably triggering failure mode on every platform?

Can Rust guarantee there are no unhandled panic paths?

[daprilik]

Well... it's not that (a) is "impossible", but rather that keeping (a) as an ongoing property of the project will require quite a bit of effort on the part of both new-contributors, and ongoing maintainers.

Thankfully, (b) is totally possible, and to answer your question: it is impossible to have an "unhandled" panic paths. All panics in Rust end up getting routed to a #[panic_handler] method.

...that said, there is a bit of nuance here, as the #[panic_handler] is a global handler, that all panics (not just those from the tpm-rs Rust crate!) would wind up at.

This is an interesting point in and of itself, as it kicks off the discussion about how best to handle that aspect of things...

• is it OK that "unexpected panics" (i.e: an assert!() call failing) percolate all the way up to the global handler, while "expected panics" use some kind of tpm-rs specific unwind mechanism (e.g: have a separate tpm_assert!() macro)?
• if not... does that mean that tpm-rs will need to be compiled into a separate compiled object file + linked into codebases separately?
  • this can get a bit hairy, as linking statically-linked Rust code to other Rust code can result in all sorts of compilation errors due to duplicate symbols...

[daprilik]

One somewhat unorthadox suggestion (that I've actually toyed with in the past, with the existing ms-tpm-20-ref implementation) is to have a tpm-rs-core crate which compile down to a .wasm module (with its own, local, #[panic_handler]), which is then ingested by a tpm-rs crate that includes a tiny WASM interpreter that runs the TPM code (with other languages having their own tpm library that executes the tpm-rs-core WASM blob)

Note that with this approach, you can avoid the combinatorial-explosion of ensuring no-panic across compilers / architectures, since the bulk of the code would target a single architecture - WASM! Sure, you'd still have to ensure that your WASM harness doesn't have its own panics... but that might actually be easier than ensuring the tpm-rs code doesn't panic. After all, WASM interpreters can get pretty small + tight...

Food for thought :)

[bradlitterell]

easy part first - I don't think it's a goal to produce a statically linkable binary. IME, embedded environments likely to consume this project are going to be small, and therefore will want link-time-optimization to minimize their size - and depending on the toolchain that probably means compiling the library with their own application/customization with precisely the same toolchain.

IOW, I think it's reasonable to expect consumers of the library to compile it with their own toolchain.

[jettr]

Thank you Daniel for the very detailed explanation of what a no-panic style would require for the tpm-rs project! You were definitely able to articulate the points much better than I original did :)

I agree that the decision to keep the tpm-rs project in the "no panic" realms will have a continual maintenance cost. I think starting off the project with the aim for no panic makes the cost more palatable and easier to keep in maintenance mode.

I also agree that we could only guarantee the no panic statement for "tier 1" compiler toolchains, and that there would probably only be one (maybe two) tier 1 toolchains that CI would check. Having at least the tier 1 toolchain produce no panic paths goes a long way for all the other toolchains combinations for reducing the number of overall panics.

I also agree that we should probably use ufmt or some other smaller formatting library if needed, since the core format library is just a code size glutton and panic producer -- neither of which I think the tpm-rs project wants. Also do we even want the tpm-rs library to output any text? I would think that any error information should be returned in Results as many embedded applications are headless.

[bradlitterell]

Also do we even want the tpm-rs library to output any text? I would think that any error information should be returned in Results as many embedded applications are headless.

Only for debugging I think.