Skip to content

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Sep 24, 2025

When encoding ereports in Hubris, the CBOR messages are generally
simple and consist of fixed-size data. However, if buffer sizes for
encoding those messages are just chosen arbitrarily by the programmer,
it is possible that subsequent changes to the ereport messages (such as
adding a new field...) will increase the encoded size beyond the chosen
buffer size, leading to encoding failures and data loss.

Therefore, this branch adds a new crate that provides traits and derive
macros for generating CBOR encoding implementations that determine the
maximum needed buffer size at compile time, using minicbor. While
this was primarily intended for use in the ereport system, it's fairly
general purpose, so I've named the new crate microcbor --- since
it's implemented using minicbor, and is designed for use on
microcontrollers.

I've also added a crate for representing strings as fixed-size arrays.
This is necessary since all field values in types deriving
microcbor::Encode must have a known maximum length at compile time,
which isn't the case for strings --- even &'static str constants.
This looks superficially similar to heapless::String, but it's
actually quite different: the primary purpose of this thing is to be
able to be constructable from a &'static str in a const fn,
failing if it's too long to fit in the max size buffer.
heapless::String, on the other hand, is designed to behave more like
the alloc::string::String type and allow the string to be
mutated/appended to at runtime. But, only empty heapless::Strings
can be const fn constructed. Also, my thing is backed by an array,
so we could potentially use it elsewhere, such as in hubpack or
zerocopy-encoded messages. Since this seemed generally usefulish
outside of the CBOR stuff, I stuck it in its own fixedstr crate.

The new libraries have a bunch of documentation explaining how they
work and how to use them, so I won't duplicate it all in the commit
message. Additionally, as a sort of trial balloon for the new stuff,
I've modified the ereports from drv_gimlet_seq_server to use it.
I'll update other tasks subsequently, but I'd like to make a
follow-up change to have build-i2c just generate FixedStrs for
component IDs instead, and I figured it was better to keep this diff
smaller for now.

Fixes #2241

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

license-eye has totally checked 603 files.

Valid Invalid Ignored Fixed
602 1 0 0
Click to see the invalid file list
  • lib/ereport/examples/test-expanded.rs

@hawkw
Copy link
Member Author

hawkw commented Sep 26, 2025

Okay, so the current approach as of e2e5106 seems to be more or less functional. But, there's one big flaw: it turns out there isn't really a way to take something like the current trait:

pub trait EreportData: Encode<()> {
/// The maximum length of the CBOR-encoded representation of this value.
///
/// The value is free to encode fewer than this many bytes, but may not
/// encode more.
const MAX_CBOR_LEN: usize;
}

and write a function like

pub fn encode_ereport<'buf, E: EreportData>(
    ereport: &E,
    buf: &'buf mut [u8; E::MAX_CBOR_LEN],
) -> &'buf [u8] {
    let mut encoder = Encoder::new(buf);
    todo!()
}

because this runs afoul of

error: generic parameters may not be used in const operations
  --> lib/ereport/src/lib.rs:22:25
   |
22 |     buf: &'buf mut [u8; E::MAX_CBOR_LEN],
   |                         ^^^^^^^^^^^^^^^ cannot perform const operation using `E`
   |
   = note: type parameters may not be used in const expressions
   = help: add `#![feature(generic_const_exprs)]` to allow generic const expressions

This feels pretty unfortunate, as a function like this is basically the UX I was hoping to be able to have. Currently, the user can get the type to tell it what the right length for the buffer is, and use it to construct a buffer, but we can't easily have an API that ensures you use the correct-length buffer when encoding, which is too bad.

We could just enable the nightly-only #![feature(generic_const_exprs)] and have it work, but I think we're trying to avoid adding new nightly features.

Another option I'm thinking about is changing the trait to something like:

pub trait EreportData: Encode<()> {
    /// The maximum length of the CBOR-encoded representation of this value.
    ///
    /// The value is free to encode fewer than this many bytes, but may not
    /// encode more.
    const MAX_CBOR_LEN: usize;
    type NeededBuf: AsRef<[u8]> + AsMut<[u8]>;
}

and having the derive macro generate a NeededBuf associated type that's [u8; MAX_CBOR_LEN]. We could then have an encode function that's like

pub fn encode_ereport<'buf, E: EreportData>(
    ereport: &E,
    buf: &'buf mut E::NeededBuf,
) -> &'buf [u8] {
   // ...
}

and you wouldn't be able to call it with a too-short array. But, this might be overthinking it...

@hawkw
Copy link
Member Author

hawkw commented Sep 29, 2025

Update: nope, the approach proposed in #2246 (comment) still ends up using a generic parameter in a const expression if the type is generic over another type implementing EreportData. So, we really just can't quite do that without #![feature(generic_const_exprs)]. I think the current state of this is still useful as it at least gives us a way to "manually" construct a correct-length buffer. You can choose to misuse the derived implementation with a wrong-length buffer, but...you could do this before, too. It would be nice to have a misuse-resistant API, but I can accept just having an API that does the Right Thing if you don't misuse it!

@hawkw hawkw requested a review from mkeeter October 6, 2025 18:16
@hawkw
Copy link
Member Author

hawkw commented Oct 6, 2025

@mkeeter i think i've addressed all your feedback! hoping to get a look from @cbiffle as well before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer-experience Fixing this would have a positive impact on developer experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

want some kind of build-time validation that ereports fit in encoding buffers
2 participants