Fix SIGILL on nightly rustc

[ishitatsuyuki]

Fix #99, I only fixed the very issue interrupting my workflow, and I'll leave other unsoundness as-is.

[bschwind]

You beat me to it! I made essentially the same change which fixed the particular issue I was hitting in #99, with the difference being that I used .as_mut_ptr()

I'm not sure if &mut this as *mut _ as *mut $struct_name; counts as "taking a reference to uninitialized memory", but if it does I would assume that also implies undefined behavior.

[ishitatsuyuki]

Yeah, that was a good catch! I adopted that suggestion.

(The code you mention only takes reference of MaybeUninit which is completely safe, and then it's turned into a raw pointer which the compiler won't mess with.)

[bschwind]

Looks good to me, and fixes the SIGILL I was encountering earlier.

[bjorn3]

I'm not sure if &mut this as mut _ as mut $struct_name; counts as "taking a reference to uninitialized memory"

No, this is of type MaybeUninit<Self> which is explicitely allowed to be uninitialized (thats the purpose of it), taking a reference to it is thus allowed.

[RalfJung]

I'm not sure if &mut this as mut _ as mut $struct_name; counts as "taking a reference to uninitialized memory", but if it does I would assume that also implies undefined behavior.

This is very subtle, and I first thought it does, but indeed the type of &mut this is &mut MaybeUnint and that reference can point to anything -- "initialized" is a type-dependent notion; an "initialized" bool must be 0x00 or 0x01 but OTOH a MaybeUninit is always "initialized" even if the memory is not.

Maybe we should pick different terms, not sure...

But either way, raw pointer casts should always be avoided, so as_mut_ptr is definitely preferable here.

[RalfJung]

Note that MaybeUninit is very recent. I definitely recommend to use it, but it means you need at least Rust 1.36.

As an interim solution, it might help to (a) use mem::zeroed, that's always preferable over mem::uninitialized (and if you are seeing the opposite effect, you should debug your code to find out where you are zero-initializing something like a reference or a function pointer that may never be NULL), and (b) fix this to use Option:

https://github.com/erlepereira/x11-rs/blob/7fe454564799c22e7bc1069758bd99f372aaf095/x11-dl/src/link.rs#L30-L31

A struct consisting only of Option<fn> and raw pointers may safely be zero-initialized, so at that point this code has well-defined behavior and works with all versions of Rust.

[est31]

it means you need at least Rust 1.36.

If this is a problem, I recommend adopting my maybe-uninit crate: It falls back to std::mem::uninitialized on older rustcs while just reexporting MaybeUninit on newer ones: https://crates.io/crates/maybe-uninit

Edit: maybe-uninit needs at least Rust 1.20.0, but as you seem to require 1.24.1 at least this doesn't seem to be a problem :).

[ishitatsuyuki]

Here you go. Maintainers: remove the latest commit if you don't like the extra dependency.

[RalfJung]

However, the maybe-uninit crate does not really avoid the UB in pre-Rust-1.36. (I think it uses ManuallyDrop there?) So I'd still recommend also turning the function pointers into Option to avoid making false promises to the optimizer.

[est31]

@RalfJung yes, you are right if fn pointers always have to be valid then maybe-uninit::MaybeUninit is using UB on older Rust versions. However, on older Rust versions, which won't change any more, this seems to not have been much of an issue, at least not in this particular instance. It only started to be an issue once Rust started changing stuff about mem::uninitialized.

In other words: you look at it in a non-constructive way and say "on older rustcs this crate is just as bad as not using it!" while I look at it in a constructive way and say "on newer rustcs this crate is doing the right thing while it doesn't regress older rustcs support".

In another thread you said that it took you two years to figure out this is a problem and two more years to provide a fix for it. And now the ecosystem should switch within six weeks? IMO the order of magnitude is wrong here.

That being said, I agree that mem::zeroed + Option is better than using maybe-uninit, at least in this instance.

[RalfJung]

I would say I look at it in a foundational/theoretical way and you look at it in a pragmatic way. ;) I think it is unfair to call my approach "non-constructive".

Note that using ManuallyDrop to hold invalid values is known to cause problems also in old compilers; IIRC SmallVec ran into that. So these concerns are not just theoretical.

In another thread you said that it took you two years to figure out this is a problem and two more years to provide a fix for it. And now the ecosystem should switch within six weeks? IMO the order of magnitude is wrong here.

I (constructively) proposed a way to fix the problem here without having to depend on Rust 1.36. I am not sure what your problem with the proposal I made is -- or if your problem is not with the proposal, then I do not know what you are even criticizing here.

Sure, without Option it might still work, but we seem to agree that it is more correct with Option. (And that has been clear for more than two years, so there was ample time. My interpretation is that the maintainer just was not aware that fn() types are non-NULL. This is understandable -- that invariant is not talked about very often, as fn() types are not widely used in Rust, and the two lists of UB that we have both forgot to list it as well.)

[est31]

I don't have a problem with the Option proposal. I guess it requires some unwraps here and there. It's fine.

[est31]

As for unconstructive vs theoretical, I just think that dismissing the entire maybe-uninit crate because it doesn't shine well in the theoretical light (which admittedly is true) is unfair because it does have pragmatic benefits. Overall there is a way out that we both agree is valid.

[RalfJung]

I didn't mean to dismiss it -- sorry if my comment came across as such. I was just pointing out that using the Option thing can still help if x11-rs decides to use that crate.

[est31]

@Daggerbot @erlepereira want to review this?

[erlepereira]

hey thanks for the ping and the comments/discussion everyone. Been meaning to jump onto this code.

Just a short note, will be looking into this over this weekend.

[est31]

@erlepereira weekend's here!

[erlepereira]

The mem::zeroed + Option does look the ideal fix here. On the MaybeUninit front, a bit reluctant to introduce it just yet (min version needed) and reluctant to introduce a new dependency, it would be effectively forcing it on everyone else who use this. Though of course, if we did at this point it would likely be an interim measure anyway.

@Daggerbot any preferences here? -- you know a lot more of the history around this code here

I cannot see any errors from switching to mem::zeroed on my end, still working on a proper solution using Option (some test code breaks...). Continuing to look deeper for a workable solution on that front.

I do realize there is a ticking timeline and this crate has not been updated in a while. Hope to have a fix soon, else this patch + dependency might have to be the way forward for now.

[ghost]

@erlepereira That sounds like our best bet. Unfortunately, x11-dl is one giant hack, and all we can seem to do is replace hacks with other hacks until undefined behavior changes in another future rust version.

x11-dl was intended to be an ergonomic way to call X functions loaded at runtime (e.g. (xlib.XOpenDisplay)(ptr::null())), but in the long run it might be worth it to break the API and bump the major version and not rely on such hacks.

Maybe it would be better to have a global lazy_init struct and expose the function pointers in Option. This would be less ergonomic to use, but could potentially prevent hacks that only work temporarily, and also another previous problem with older versions of the libs not exposing certain symbols (#35). The other drawback would be no way to unload the library.

I'm curious how gl_generator solved some of these problems.

[ishitatsuyuki]

I think with MaybeUninit the UB problem is solved, modulo rustc version requirement - @Daggerbot does this address your concern?

[est31]

@Daggerbot @erlepereira is the current solution any good? The new code is only a "hack" on versions of Rust where there are no problems and on any further versions it uses the recommended MaybeUninit. I'd love to have a fix for this because it makes my app crash at startup :/. The additional dependency is very small, quick to compile and doesn't have any dependencies of its own. And it's only temporary until you choose to drop support for versions older than 1.36.0.

[erlepereira]

As stated earlier, this code-base needs some attention. Yeah, for the interim I guess this works fine.

For the longer term, need to update this code, bringing up the supported version while incorporating the various ideas raised here and in other issues. Will bring all these ideas together in another thread / issues, and find a workable way forward for this/other issues that maybe related.

Leaving this issue open until I merge the patch, release an updated crate with this patch. Eta later tonight/tomorrow.

wiki reference, placeholder issue created for future removal of this interim solution #102