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.