[rust] Generated enums can cause undefined behavior

[jean-airoldie]

Just remembered that a discriminant absent from an C-like enum is considered undefined behaviour in rust. This means that all currently generated enums could potentially cause undefined behavior if someone adds a new enum field and sends that buffer to someone using a older version of the schema.

I think the ideal solution would be to use a standard enums and add the appropriate TryFrom implementation (e.g. TryFrom<u8>). Maybe we should returned the unmatched integer as the error type.

[jean-airoldie]

I'll take a look at the code generator tomorrow if I find the time.

[rw]

@jean-airoldie Great! You think this is better than using the non-exhaustive attribute you mentioned in #5405 ?

[jean-airoldie]

I think the two should be used together. The problem with only using the non-exhaustive is that it does not prevent UB. As soon as an enum is created with a discriminant not in the type definition we get UB, before even matching it.

Using non-exhaustive is meant to "future-proof" enums so that they can add variants without it being a breaking change (because this is basically enforced by the compiler). So still I think this usage fits flatbuffers.

[rw]

Thanks for thinking this through.

I'm interested in your TryFrom idea. Want to write up some code (in a gist, maybe) that shows what you're thinking?

[jean-airoldie]

Sure I'll do that.

[jean-airoldie]

This is the simplest solution I can think of.

// schema.fbs

enum MsgType: uint8 {
    Notification,
    Data,
}

// This is the generated code
#[allow(non_camel_case_types)]
#[repr(u8)]
#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)]
enum MsgType {
    Notification = 0,
    Data = 1,
}

const ENUM_MIN_MSG_TYPE: u8 = 0;
const ENUM_MAX_MSG_TYPE: u8 = 1;

impl flatbuffers::EndianScalar for MsgType {
  #[inline]
  fn to_little_endian(self) -> Self {
    let n = u8::to_le(self as u8);
    let p = &n as *const u8 as *const MsgType;
    unsafe { *p }
  }
  #[inline]
  fn try_from_little_endian(self) -> Result<Self, u8> {
    let n = u8::from_le(self as u8);
    if n > ENUM_MAX_MSG_TYPE || n < ENUM_MIN_MSG_TYPE {
        Err(n)
    } else {
        let p = &n as *const u8 as *const MsgType;
        Ok(unsafe { *p })
    }
  }
}

[jean-airoldie]

Also we might want to use https://github.com/BurntSushi/byteorder for endianness. AFAIK its the goto library that.

So I think the ideal solution is to split EndianScalar into 2 layers:

• &[u8] -> scalar via byteorder crate
• scalar to enum via TryFrom trait

[theduke]

Is anyone working on this?

I already ran into this issue after adding a variant.

[jean-airoldie]

I'm not, so my guess is no one is. If you are interested a PR is welcomed, otherwise I'll implement it when I get the time (at last 1 month from now).

You can step around this by always adding a _ => () clause to your match statements, but its easy to forget, and not a good work around.

[jean-airoldie]

I think the ideal solution would be to use the FromPrimitive and ToPrimitive traits from the num crate. https://docs.rs/num-derive/0.3.0/num_derive/derive.FromPrimitive.html

[vglavnyy]

Is any progress with this issue?
I'm working with #5108 (connected issue #5194).
It is impossible to assign 0 in Rust, if zero isn't part of an enum.

enum Color:ubyte (bit_flags) { 
  Red = 0, // color Red = (1u << 0)
  Green, 
  Blue = 3, 
}

table Monster {
  color_mix: Color = "Red Blue"; // allowed by fbs grammar but will lead to an error in Rust
  color_none: Color; // error #5194 (0 isn't part of enum)
}

@jean-airoldie
I need advice on 0-default issues (#5194, #5108) .
One way is to inject an implicit 0-default field into the enum at code-generation time (like cpp_gen does).
Is there other solution?

[jean-airoldie]

Is any progress with this issue?

I'm not working on it atm.

It is impossible to assign 0 in Rust, if zero isn't part of an enum.

I'm not sure I'm following, are you talking about the generated code or a rust c enum?

What kind of error is produced by the "Red Blue" bitfield in your example in rust?

[jean-airoldie]

For a bitflag in rust you would want to use an integer as opposed to an enum. But ideally you would want something more convenient like the bitflag crate.

[vglavnyy]

I'm not sure I'm following, are you talking about the generated code or a rust c enum?

What kind of error is produced by the "Red Blue" bitfield in your example in rust?

I'm talking about the generated code.

Rust code generator fails with:

/flatbuffers/src/idl_gen_rust.cpp:738: std::__cxx11::string flatbuffers::rust::RustGenerator::GetDefaultScalarValue(const flatbuffers::FieldDef&): Assertion `ev' failed.

At generation time "Red Blue" is equal to ((1u << 0) | (1u << 3)) which isn't part of Color enum.

flatbuffers/src/idl_gen_rust.cpp

Lines 736 to 741 in 7f1af7c

	case ftEnumKey: {
	auto ev = field.value.type.enum_def->FindByValue(field.value.constant);
	assert(ev);
	return WrapInNameSpace(field.value.type.enum_def->defined_namespace,
	GetEnumValUse(*field.value.type.enum_def, *ev));
	}

The bitflag crate is a good idea but the 0-default issue is a general problem:

enum WithoutZero : ubyte { 
  Two = 2
}
table Monster {
  test: WithoutZero; // error, 0  isn't part of enum
}

What value should the program pass to GetEnumValUse if field.value.constant == "0" and 0 is not part of user-defined fbs-enum?