Stream: general

Topic: Enum memory layout, ffi, uninitialized values and UB


weiznich (Mar 31 2020 at 21:39, on Zulip):

I'm not sure if that's the right place to ask this kind of question, so if I should ask such questions somewhere else I would be happy to hear where.

I'm currently reviewing a diesel PR that implements support for custom aggregate functions for sqlite. At FFI boundary such functions are provided as set of function pointers. As an aggregate function needs to handle some state, therefore sqlite provides a function to receive the corresponding state for the current function call. This function should be called in of the the callbacks/functions provided as implementation to sqlite. Now their documentation states

The first time the sqlite3_aggregate_context(C,N) routine is called for a particular aggregate function, SQLite allocates N bytes of memory, zeroes out that memory, and returns a pointer to the new memory. On second and subsequent calls to sqlite3_aggregate_context() for the same aggregate function instance, the same buffer is returned.

so it's basically a malloc kind function. To save some indirection here and because we do need to handle a certain wrap around all state either way one idea was to have those N bytes of memory directly represent basically an Option of our state. This leads me to the following questions regarding to what guarantees are made by rust(c) and what's undefined behaviour:

  1. I'm right to assume that the enum discriminates of std::option::Option are considered to be an implementation detail? So I should not assume that Option::None has a zero discriminant?
  2. Building on top of 1: For the following enum it's fine to say that the discriminate of MyOption::None is zero?
#[repr(u8)]
enum MyOption<A> {
    // Discriminant is 0
    #[allow(dead_code)]
    None,
    Some(A),
}
  1. As we don't have any information about the inner type of the Some variant: It's fine that this would point to potential uninitialized memory, because 2 would guarantee us that we have the None variant that does not hold any other information?
ecstatic-morse (Mar 31 2020 at 22:40, on Zulip):

@weiznich The reasoning about the code in the second example is not correct. MyOption is not a C-like enum (one of its variants has a field), so its discriminant is implementation defined. repr only affects "C-like" enums.

centril (Mar 31 2020 at 22:42, on Zulip):

@ecstatic-morse what do you mean by "implementation defined"?

ecstatic-morse (Mar 31 2020 at 22:44, on Zulip):

What the OP referred to as an implementation detail.

centril (Mar 31 2020 at 22:45, on Zulip):

So "implementation defined" typically means that the language doesn't specify what it is, but the implementation must. In this case it would be unspecified behavior, and the implementation is not free to specify it

ecstatic-morse (Mar 31 2020 at 22:46, on Zulip):

Sure.

weiznich (Apr 01 2020 at 07:21, on Zulip):

@ecstatic-morse Maybe I'm misreading RFC-2195 and the nomicon but I think they state a different behaviour.

RalfJ (Apr 01 2020 at 07:37, on Zulip):

just to be clear, you have "uninitialized values" in the title but you are not actually dealing with truly uninitialized memory and all its madness? everything is actually zero-initialized?

RalfJ (Apr 01 2020 at 07:37, on Zulip):

Remember however that memory can become uninitialized again, when other uninitialized memory is copied into it or when it is inside padding

RalfJ (Apr 01 2020 at 07:38, on Zulip):

@weiznich the proper stream for this question would have been #t-lang/wg-unsafe-code-guidelines but this should do as well. ;)
Cc @WG-unsafe-code-guidelines

RalfJ (Apr 01 2020 at 07:39, on Zulip):

coming back to the enums, the nomicon says

If the enum has fields, the effect is similar to the effect of repr(C) in that there is a defined layout of the type.

so indeed I agree that this should be unspecified behavior.

RalfJ (Apr 01 2020 at 07:40, on Zulip):

likely, the reference was just never updated for repr on dataful enums

weiznich (Apr 01 2020 at 07:41, on Zulip):

Right, zero-initialized is a better wording here.

weiznich (Apr 01 2020 at 07:43, on Zulip):

On the other hand the corresponding section in the RFC says that #[repr(C)] on a enum with fields in equivalent with having a tag enum without fields that is #[repr(Int)] and a union with the field values. That said I'm not sure how far the corresponding RFC is implemented yet.

RalfJ (Apr 01 2020 at 07:44, on Zulip):

that's not what the RFC says -- the RFC says its a union where each variant starts with a tag enum without fields

RalfJ (Apr 01 2020 at 07:45, on Zulip):

that is different from having a struct, where first we have the tag and then a union for each variant

RalfJ (Apr 01 2020 at 07:45, on Zulip):

(IIRC the difference arises when considering alignment constraints)

RalfJ (Apr 01 2020 at 07:45, on Zulip):

repr(u8) would not be accepted on dataful enums if the RFC was not implemented

RalfJ (Apr 01 2020 at 07:46, on Zulip):

but it is rather strange that the RFC does not link to a tracking issue like it usually should... hm

RalfJ (Apr 01 2020 at 07:46, on Zulip):

so given that RFC, I agree that it is okay to transmute a "bunch of zero bytes" to MyOption.
however, notice that MyOption contains padding, so if you transmute it back those padding bytes will be uninitialized.

weiznich (Apr 01 2020 at 07:49, on Zulip):

The corresponding code in diesel does basically only check if it's None and if so it writes a new fully initialized value there, so I would assume it cannot ever read any uninitialised values?

RalfJ (Apr 01 2020 at 16:43, on Zulip):

I dont know what to look at in that code, but yes -- turning the value back into the enum type and then checking for None should work

RalfJ (Apr 01 2020 at 16:43, on Zulip):

what would not work is memcmp with a bunch of zeroes

RalfJ (Apr 01 2020 at 16:43, on Zulip):

@weiznich ^

weiznich (Apr 01 2020 at 17:09, on Zulip):

The important part is this piece of code:

// This call returns a ptr to std::mem::size_of::<OptionalAggregator<A>> zeroed bytes on the first call
let aggregate_context = ffi::sqlite3_aggregate_context(ctx, std::mem::size_of::<OptionalAggregator<A>>() as i32);
let mut aggregate_context = NonNull::new(aggregate_context as *mut OptionalAggregator<A>);
let aggregator = match aggregate_context.map(|a| &mut *a.as_ptr()) {
    // we do something else with agg below, it cannot leak from this function
    Some(&mut OptionalAggregator::Some(ref mut agg)) => agg,
    // If we get back a ptr to zeroed bytes we want to write our own struct to this place.
    // This will initialize the value for future calls.
    Some(mut a_ptr @ &mut OptionalAggregator::None) => {
        ptr::write_unaligned(a_ptr as *mut _, OptionalAggregator::Some(A::default()));
        if let &mut OptionalAggregator::Some(ref mut agg) = a_ptr {
            agg
        } else {
            unreachable!("We've written the aggregator above to that location, it must be there")
        }
    },
    None => unimplemented!("Does not matter to understand the part above"),
}

Otherwise thanks for taking the time to answer this :+1:

ecstatic-morse (Apr 01 2020 at 17:52, on Zulip):

weiznich said:

ecstatic-morse Maybe I'm misreading RFC-2195 and the nomicon but I think they state a different behaviour.

Whoops. I missed RFC 2195. Sorry. Ignore me.

RalfJ (Apr 03 2020 at 09:47, on Zulip):

RalfJ said:

likely, the reference was just never updated for repr on dataful enums

so is there a follow-up here where we should update the reference?

centril (Apr 04 2020 at 14:16, on Zulip):

@RalfJ maybe file an issue to start with :slight_smile:

RalfJ (Apr 04 2020 at 14:37, on Zulip):

fair: https://github.com/rust-lang/reference/issues/786

Last update: May 29 2020 at 16:55UTC