Match on repr(C) enum returned from C library with unknown value leads to UB

[tcosprojects]

Summary

It is not uncommon for C libraries to add more values to an enum in their API while considering it a non-breaking change. However, with a rust library linked to the C library it causes undefined behavior when the change is made if the rust enum definition is not updated.

Examples

I've reduced the issue down to this sample code where get_bad_kind() represents a call into a C library that returns a new enum value unknown to rust which leads to a crash at runtime (signal: 11, SIGSEGV: invalid memory reference or stack overflow): https://play.rust-lang.org/?gist=f066d8e489a6e220866958065891a812&version=stable&backtrace=0

The C version of this code does not result in this issue, instead it hits the default case and returns -1: https://gist.github.com/tcosprojects/49d0c809d40b8f008e25dacf49c508bf

Tested on

rustc 1.12.0 (3191fba 2016-09-23)
binary: rustc
commit-hash: 3191fba
commit-date: 2016-09-23
host: x86_64-unknown-linux-gnu
release: 1.12.0

and

rustc 1.12.0 (3191fba 2016-09-23)
binary: rustc
commit-hash: 3191fba
commit-date: 2016-09-23
host: i686-pc-windows-msvc
release: 1.12.0

Discussion

Discussing the issue on the #rust channel led to the following:

A binding library would need to match the rust definition of the enum with the C definition exactly at all times when linked to a C library to prevent either this undefined behavior or passing unsupported flags to older versions of the C library. This could be done in the cargo build script that checks the C library version and enables feature flags for these enum values. It would still not not guarantee safety though when using dynamically linked libraries. Or it could be resolved by not using repr(C) enums and instead translating them at runtime with a function that handles invalid values. If the latter should be done, it would be helpful to have it mentioned in the docs somewhere. It also makes me uncertain what the purpose of repr(C) enum is, if not to be compatible with and used with C FFI calls.

In any case, as a C programmer this bug was surprising and took some significant debugging. It required going into the assembly output of the match statement to see it jump based on the constant of the highest value from the rust enum definition and then corrupt the stack to pin down the cause of the crash.

A warning about this compatibility issue regarding repr(C) enum in the FFI docs would be helpful. I suspect it is not uncommon for rust bindings libraries to use repr(C) enum this way while expecting it to work.

If the match expression could jump to unreachable!() in debug builds when encountering an unknown value on a repr(C) enum type it would help catch this issue without so much debugging.

Questions

1. In the discussion on #rust there was some disagreement over whether this is undefined behavior for C. Is it?
2. In the discussion there was also disagreement over whether this should be considered a breaking ABI change by the C library. In practice it does not seem uncommon for C libraries to do this while considering it non-breaking. Is it?
3. Should this be a concern for rustc? Or handled elsewhere?
4. Is it possible for rust to match the behavior of the C program's switch when an enum is repr(C)?

[nagisa]

match in Rust has exhaustive semantics and it is very prominently documented pretty much everywhere where match is mentioned. repr(C) only changes the representation of the enum (i.e. how it appears in memory) to match C representation, but no representational change affects (or should affect) the behaviour of rust expressions.

TL;DR: match is match, not switch by design, no matter what representation the enum has.

In the discussion there was also disagreement over whether this should be considered a breaking ABI change by the C library. In practice it does not seem uncommon for C libraries to do this while considering it non-breaking. Is it?

It is not a ABI breaking change, as adding a variant to a C enum (as long as the size of the actual integer which represents the enum in memory does not change) has no influence on the registers/memory locations where this enum gets placed when returned or passed as an argument.

It is, however, an API-breaking change.

Should this be a concern for rustc? Or handled elsewhere?

Rustc (its match) is behaving as intended IMO.

Is it possible for rust to match the behavior of the C program's switch when an enum is repr(C)?

Matching on enum as i64 gets you switch-like semantics. Though, it is by no means convenient.

[Aatch]

Ok, so some research suggests that 1) C and C++ have similar, but slightly different rules about the valid values of an enumeration type. However, it seems that neither are limited to the values defined by the declaration.

As far as I can tell, an enumeration type in C can take on any value of the underlying type. This means that adding an enum constant may-or-may-not be a breaking ABI change depending on the compiler and target (because of course it is). Fortunately, most platforms will default to i32 unless specifically fixed to another type.

With that in mind, we should probably do two things:

1. Not supply range metadata to LLVM for repr(C) enums, that eliminates one source of UB and allows for manual checking of the values if necessary by casting to an integer type.
2. Generate a default case if there isn't one already in the match when debug_assertions is enabled. I think that keeping exhaustiveness is valuable. It's also easier.

[retep998]

The fact that Rust enums cause UB when they hold a value that isn't a valid variant is the primary reason I don't use Rust enums in winapi and instead use simple integer constants _{(The other reason being that integer constants don't require any trait impls or methods and significantly reduce compile times)}. C/C++ will often even use enums as bitflags which definitely don't align with Rust enums.

In the discussion on #rust there was some disagreement over whether this is undefined behavior for C. Is it?

Windows API uses enums as bitflags in both C and C++ which indicates that it is not undefined behavior in MSVC C/C++.

[arielb1]

Sure. We purged all Rust enums from rustllvm unless we control the other side. Maybe prohibit repr(C) on enums (we would have to find another repr tag)?

[arielb1]

Matching on enum as i64 gets you switch-like semantics. Though, it is by no means convenient.

That's wrong. Creating a Rust enum with an out-of-range value is instant UB - we generate !range metadata, which would cause your switch to be optimized out.

[DemiMarie]

I think that there should be a lint against using an enum type in FFI, except for the case where the enum cannot have been created or modified by foreign code. It is way too easy to mess this up.

A newtype of an integer is a much better solution.

[retep998]

Perhaps we should allow extern functions which take an enum by value, but lint on any which return an enum or take a mutable pointer to an enum. And by perhaps, I mean we really need this because this is a giant footgun waiting to catch someone out.

[zackw]

/subscribe

If a C API takes an enum by value, there's a decent chance it's actually bitflags, i.e. you're expected to be able to do E_THIS|E_THAT|E_THEOTHER in the call.

[mjbshaw]

The usage of C enums as bitflags can be handled in Rust with things like the bitflags crate (or something similar). So that shouldn't be a big deal.

But this issue of undefined behavior for "out of range" enums is a real concern for me. bindgen even generates Rust enums from C enums. Describing this current state of affairs as a "foot gun" is a perfect description. Rust really needs an ergonomic solution to using C enums in a Rust FFI setting. Otherwise Rust's safety promises are moot.

[tonychain]

Wouldn't the simplest option be allowing the user to specify the default value to be selected in the event the enum is out-of-range? Something like:

#[repr(C, default = Invalid)]
enum Thing {
    Invalid,
    ...

At least for the enums I happen to be working with in this FFI, they all support a good choice of a default option.