hecatia-elegua / bilge

Use bitsized types as if they were a feature of rust.
Apache License 2.0
171 stars 17 forks source link

`#[derive(Default)` introduces unsoundness in structs #50

Closed pickx closed 1 year ago

pickx commented 1 year ago

you can #[derive(Default)] on bitsized structs, since arbitray_int::UInt is Default. this has the same effect as setting the inner value to 0. this can introduce unsoundness:

use bilge::prelude::*;

#[bitsize(2)]
#[derive(TryFromBits, Debug)]
enum Unfilled {
    A = 1,
    B = 2,
    C = 3,
}

#[bitsize(2)]
#[derive(TryFromBits, Default, DebugBits)]
struct Unsound {
    field1: Unfilled,
}

fn main () {
    let via_try_from = Unsound::try_from(u2::new(0));
    assert!(via_try_from.is_err());

    let via_default = Unsound::default(); // via_default.field1 is now a value of `Unfilled` that is not allowed
    dbg!(via_default); // this crashes at runtime (panicked at 'unreachable')
}
hecatia-elegua commented 1 year ago

wonderful

hecatia-elegua commented 1 year ago

People already use derive(Default), so we would have to figure out the full picture if we have to break stuff. Spitting out a const _: () = assert!(Unsound::try_from(u2::new(0)).is_ok(), "deriving Default for Unsound is unsound"); only works for old nightly rn. So, DefaultBits would have to do the same dance I used for TryFromBits and/or wait for nightly.

Oh and to not break them we could rewrite "Default" to "DefaultBits" as well lol. But that's a bit too hidden for me.

pickx commented 1 year ago

here are the use cases I can think of forDefault::default() with a bilge struct as of today (I apologize if I missed anything)

  1. the user has a specific default bit pattern in mind, and has added appropriate #[default] attributes for every enum which appears in the struct's fields
  2. deriving it because the user would like an all-zero bit pattern
  3. a user needs a new instance of a complicated type, but does not feel like spelling out every single field for new()
  4. deriving it because it's easy to do so, and seems useful/harmless

I argue that cases 3 and 4 might be common, and furthermore that the user might not be aware of the possible implications of deriving Default. in use case 2 (and arguably 3), what the person really wants is an all-zero constructor.

so maybe what we want is to discourage (or even disallow) derive(Default) and suggest something like zerocopy::FromBytes, or the less strict bytemuck::Zeroable for users who want an all-zero constructor. Unsound::zeroed() is more explicit than Unsound::default().

hecatia-elegua commented 1 year ago

One thing: if Filled, you're allowed to zero, currently. So, maybe we generate "Default is forbidden" if we see TryFromBits + Default? edit: this would still break some code (which uses TryFromBits and Default)

ohhh, how about we emit a warning "change Default to DefaultBits" when deriving Default until the next major version, but still convert Default to DefaultBits internally?

Also, I wonder if there are registers which want to be initialized to zero but then don't allow you to write zero to their first field. That would open up another can of worms.