Generated tests use UB in their calculations

[Lokathor]

Test code for struct offsets run checks like this:

    assert_eq!(
        unsafe { &(*(::core::ptr::null::<timeval>())).tv_sec as *const _ as usize },
        0usize,
        concat!(
            "Offset of field: ",
            stringify!(timeval),
            "::",
            stringify!(tv_sec)
        )
    );

But it's UB to dereference a null pointer, and it's also UB to create a &T with numeric value 0.

There is actually trivial to fix. Instead of using a null pointer, just create a value on the stack (using core::mem::zeroed(), which is safe since all C structs can be created as zeroed-memory), and then use a reference to that stack value, the fields of that stack value, and so on.

[emilio]

This is known, but thanks for filing. I've always hoped that rust would have an offsetof macro, but... ;)

Creating structs on the stack with zeroed memory is ok for C, but not so sure for C++, where what bindgen may generate is not POD, or may have destructors or other side effects.

[Lokathor]

Can go by if the type is Copy or not. Also can use ManuallyDrop if the Rust target is new enough.

I'm not sure what most of the point of those tests is anyway, most of them can't ever fail so it seems kinda silly.

[emilio]

How not? Those tests are important, if bindgen generates something that has the wrong size or alignment or anything, those tests catch it.

[Lokathor]

Replied in a separate issue because it's not really related to this "offset calculation is UB" issue.

[gnzlbg]

@Lokathor whatever you come up with will be UB anyways, because rust-bindgen needs to compute the offset of #[repr(packed)] struct fields, and there is no way to do that in Rust without UB.

[RalfJung]

@gnzlbg that is true, but it has nothing to do with packed. You can't do it for non-packed struct either, and once we have fixed that (using &raw) the fix will work for packed structs, too.

https://github.com/Gilnaa/memoffset/ implements the currently best way we have to do this. The most important part is to avoid NULL references; you could use NonNull::dangling instead.

[RalfJung]

I've always hoped that rust would have an offsetof macro, but... ;)

Given that library solutions cannot (it seems) soundly support nested fields and arrays, I think indeed this should become a built-in macro. But I don't have a thick enough skin for the syntax bikeshed that will undoubtedly trigger. ;)

Until then, we have that macro as a library.

[gnzlbg]

You can't do it for non-packed struct either, and once we have fixed that (using &raw) the fix will work for packed structs, too.

Notice that rust-bindgen derives Default for its types, so at least when it does that it could actually construct a value of a type by using Default::default and then compute the field offsets on that value without any null dereferences. That computation still requires creating a reference to a field, and since those fields can be unaligned for #[repr(packed)] structs, that would still be UB.

[Lokathor]

@gnzlbg why did i get pinged?

[gnzlbg]

Because you proposed a solution:

There is actually trivial to fix. Instead of using a null pointer, just create a value on the stack (using core::mem::zeroed(), which is safe since all C structs can be created as zeroed-memory), and then use a reference to that stack value, the fields of that stack value, and so on.

so I thought you might work on it, but that solution doesn't work, because of repr(packed).

I think the mem::zeroed part happens to work because rust-bindgen never generates types (at least by default, or at least it shouldn't) for which it doesn't work, but that's not the only source of UB.

[Lokathor]

Oh, well (1) I'm not going to work on this myself I'm just reporting the problem, (2) bindgen knows when it will emit repr(packed) so just don't do the simplistic version in that case (3) right the main problem is dereferencing a null pointer.

[gnzlbg]

(2) bindgen knows when it will emit repr(packed) so just don't do the simplistic version in that case

What should it do in that case then?

(3) right the main problem is dereferencing a null pointer.

The main problem is undefined behavior, but there are no solutions without undefined behavior.

---

There is nothing for rust-bindgen to do here: whatever it does, it will be UB, and someone will open an issue to report that, and maybe change that, into something else that also has UB.

This should either be "blocked on" some rust-lang/rust tracking issue, or closed as "non-actionable", but right now there is no way to fix on rust-bindgen's side.

[Lokathor]

Structs that aren't repr(packed) can be fixed.

Further, structs that are repr(packed) can still at least be tested against a zeroed value on the stack instead of on a null pointer and at least eliminate one of the problems, even if it's not every problem being solved.

It's okay to solve part of a problem at a time.

[gnzlbg]

I don't see the point yet.

If this is a problem because it is UB, there are currently not solutions to this problem. But if "UB is ok as long as it works", then the current code, which works, is as good as anything else. Time would be better spent into trying to provide a way to do this without any UB, but that cannot happen here.

[Lokathor]

Well, most structs aren't repr(packed), so actually it can happen here.

[gnzlbg]

Well, most structs aren't repr(packed), so actually it can happen here.

Even for non repr(packed) structs, this would still be UB if the type has zero as a niche, which I don't think happens by default (except maybe for C++), but happens often when using the opaque-type feature, for example.

[Lokathor]

...okay but you get my point that this can be fixed for the vast majority of C struct bindings I hope.

[gnzlbg]

I disagree with vast majority (all C bindings I work on use repr(packed)), but yes, for some subset of C bindings, some of rust-bindgen tests could become UB free.

[RalfJung]

@gnzlbg there's still a big difference between "theoretical UB" (that works because it's spec-only UB but we currently -- as of the latest nightly rustc -- never exploit it) and UB that rustc/LLVM might actually exploit, but they happen to not do that in the cases you checked (like NULL references).

I don't understand why you are fighting back so hard against what is clearly a strict improvement in language compliance. Why this black-and-white attitude?

[gnzlbg]

This issue reports that the generic code that bindgen uses is broken, but such code can’t be fixed, and this issue can’t be closed. The code has currently UB in the spec and in LLVM, and the proposed “fix” does so as well. We all agree that this reliance on UB is brittle, and Id rather not change something that’s known to work for bindgen users to something that doesn’t really fixes the issue and isn’t known to work at the bindgen scale (many more crates use bindgen than offsetof, reliance on the behavior of UB on many different toolchain versions, etc.). This also only impacts UB in #[test]s, and that code is not even compiled when using bindgen libraries as dependencies. Once &raw is available, fixing this issue would be a no brainer, but right now there are IMO dozens of other open issues with a better value proposition than what putting any work on this one would deliver.

[RalfJung]

So, let me rephrase: "if it can't be perfect (doesn't really fix the issue) then we don't care how awful it is".

many more crates use bindgen than offsetof

offsetof is used in crossbeam, so if you really want to compare numbers...
memoffset recent downloads: 1,592,597
bindgen recent downloads: 310,642

But also, this is clearly not leading anywhere, so I'll just stop here.

[gnzlbg]

"if it can't be perfect (doesn't really fix the issue) then we don't care how awful it is".

That's not what I said.

[Lokathor]

Update: there is a fully sound offset_of! macro available in the bytemuck crate:
https://docs.rs/bytemuck/1.2.0/bytemuck/macro.offset_of.html

The "catch" is that you have to pass in an instance of the type to the macro. So either the type needs to have default or makeable with all 0s or something like that.

[anp]

Would the addition of addr_of make this easier to implement without UB?

FYI, the recent addition of a new lint for deref'ing a null ptr surfaces this:

error: dereferencing a null pointer
  --> ../../src/lib/usb_bulk/rust/src/usb.rs:59:19
   |
59 |         unsafe { &(*(::std::ptr::null::<usb_ifc_info>())).dev_vendor as *const _ as usize },
   |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this code causes undefined behavior when executed
   |
   = note: `-D deref-nullptr` implied by `-D warnings`

[emilio]

Perhaps? But for that we'd also need to create an instance of the type, which we might not know how to create, and which may have other side effects... Can we get an address into a MaybeUninit::<NonTrivial>::uninit() without it being UB somehow?

I don't see how off-hand but that doesn't necessarily mean it doesn't exist... In any case I think we'd still be working around the lack of a standard offset_of! macro...

[Lokathor]

any C type can be constructed of zeroed memory at least.

[emilio]

Not if you implement Drop on the rust side or what not, which might be a reasonable thing to do.

[simlay]

It looks like rust-lang/rust#95372 caused these warnings are now errors in nightly which might means it'll error on rust 1.62 (in ~10 weeks).

For the next person, it looks like quick work around (but definitely not the correct long term solution) is to use layout_tests(false) similar as seen with the iOS cross compilaion of boring.

[koutheir]

Disabling layout tests is only useful to remove the warning and undefined behavior, at the expense of more risks with regard to ABI changes. That's a compromise one needs to think about. The layout_tests(false) workaround simply hides the issue instead of solving it.

[Lokathor]

The layout tests can't detect that to begin with so there's no difference if you turn them off.

[koutheir]

If the layout tests can't detect ABI changes, then what are they supposed to validate?

[Lokathor]

They don't validate much because they don't call into C. It's just checking rust code against other rust code. All they actually end up doing is a clumsy check that your C type aliases are correct. Which you can do once per crate, instead of once per type, and get just as much benefit.

EDIT: even then, checking the type aliases once per crate is excessive probably. Really they should be checked once when defined (eg: in cty or std or wherever else) and then no downstream crate needs to double check after that.

[koutheir]

Does this mean that one way of actually solving this issue would be to remove the generation of these checks entirely?

[Lokathor]

that's my suggestion, yes.

[emilio]

This is not right. The layout tests are useful, because they test the actual generated rust code with the offsets that we get from libclang.

[emilio]

So even though it's not the same as calling into C and back, it's pretty close, and they've caught ABI issues in Firefox in the past, for example.

[Lokathor]

the tests could certainly detect some problems, but if all C type aliases are correct then that's about the extent of what they can check. For example, if a conditional compilation adds or changes a field in a C struct in a new C lib version, or on a different target, the rust code has no way to notice, and these tests won't help you. They are extremely un-portable tests.