Fix SIGILL on nightly rustc #101

ishitatsuyuki · 2019-07-19T01:52:20Z

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

bschwind · 2019-07-19T02:25:33Z

x11-dl/src/link.rs

@@ -55,12 +55,11 @@ macro_rules! x11_link {
        unsafe {
          let libdir = $crate::link::config::libdir::$pkg_name;
          let lib = try!($crate::link::DynamicLibrary::open_multi(libdir, &[$($lib_name),*]));
-          let mut this: ::std::mem::ManuallyDrop<$struct_name>
-              = ::std::mem::uninitialized();
+          let mut this = ::std::mem::MaybeUninit::<$struct_name>::uninit();
          let this_ptr = &mut this as *mut _ as *mut $struct_name;


Now that we're using MaybeUninit, it might be better to use this.as_mut_ptr() here

bschwind · 2019-07-19T02:29:06Z

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 · 2019-07-19T03:12:44Z

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 · 2019-07-19T03:19:33Z

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

bjorn3 · 2019-07-19T07:03:57Z

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 · 2019-07-19T09:23:20Z

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 · 2019-07-19T09:30:11Z

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 · 2019-07-21T12:08:40Z

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 · 2019-07-22T08:47:39Z

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

RalfJung · 2019-07-22T08:50:58Z

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 · 2019-07-22T09:43:12Z

@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 · 2019-07-22T09:53:17Z

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 · 2019-07-22T10:09:04Z

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

est31 · 2019-07-22T10:11:53Z

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 · 2019-07-22T11:44:37Z

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 · 2019-07-29T15:28:49Z

@daggerbot @erlepereira want to review this?

erlepereira · 2019-07-30T12:16:55Z

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 · 2019-08-03T23:53:57Z

@erlepereira weekend's here!

erlepereira · 2019-08-04T08:15:34Z

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 · 2019-08-06T21:30:46Z

@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 · 2019-08-07T04:23:24Z

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

est31 · 2019-08-10T00:20:00Z

@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.

RalfJung · 2019-08-11T20:31:44Z

x11-dl/src/link.rs

          ::std::ptr::write(&mut (*this_ptr).lib, lib);
          try!(Self::init(this_ptr));
-          Ok(::std::mem::ManuallyDrop::into_inner(this))
+          Ok(this.assume_init())


So, it is guaranteed then that Self::init will put non-NULL values into all these function pointers?

IMO that should be called out explicitly in a comment.

erlepereira · 2019-08-12T08:41:33Z

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

implement zeroed and uninitialized with MaybeUninit This is the second attempt of doing such a change (first PR: rust-lang#62150). The last change [got reverted](rust-lang#63343) because it [caused](rust-lang#62825) some [issues](rust-lang#52898 (comment)) in [code that incorrectly used these functions](AltF02/x11-rs#99). Since then, the [problematic code has been fixed](AltF02/x11-rs#101), and rustc [gained a lint](rust-lang#63346) that is able to detect many misuses of these functions statically and a [dynamic check that panics](rust-lang#66059) instead of causing UB for some incorrect uses. Fixes rust-lang#62825

x11-dl was using std::mem::uninitialized incorrectly and when rustlang added MaybeUninit and intrinsic panics on UB caused by improper use of uninitialized (see rust-lang/rust/pull/69922) it caused issues with X11 initialization. x11-dl pr AltF02/x11-rs/pull/101 updated x11-dl to use MaybeUninit correctly

bschwind reviewed Jul 19, 2019

View reviewed changes

Fix SIGILL on nightly rustc

c41da74

ishitatsuyuki mentioned this pull request Jul 19, 2019

[BUG] Multiple SIGILL occurences with nightly opt-level=1 amethyst/amethyst#1808

Closed

Swtich to maybe-uninit for backward compatibility

e9cc5bb

RalfJung reviewed Aug 11, 2019

View reviewed changes

erlepereira merged commit f47c280 into AltF02:master Aug 13, 2019

erlepereira mentioned this pull request Aug 13, 2019

Upgrade to later rust min version, revert MaybeUninit workaround #102

Closed

ishitatsuyuki deleted the sigill-fix branch August 15, 2019 00:44

RalfJung mentioned this pull request Mar 11, 2020

implement zeroed and uninitialized with MaybeUninit rust-lang/rust#69922

Merged

matthew-russo mentioned this pull request Mar 26, 2020

upgrades x11-dl to 2.18.5 to fix #376 rust-windowing/winit#1517

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix SIGILL on nightly rustc #101

Fix SIGILL on nightly rustc #101

ishitatsuyuki commented Jul 19, 2019

bschwind Jul 19, 2019

bschwind commented Jul 19, 2019

ishitatsuyuki commented Jul 19, 2019

bschwind commented Jul 19, 2019

bjorn3 commented Jul 19, 2019

RalfJung commented Jul 19, 2019

RalfJung commented Jul 19, 2019 •

edited

Loading

est31 commented Jul 21, 2019 •

edited

Loading

ishitatsuyuki commented Jul 22, 2019

RalfJung commented Jul 22, 2019

est31 commented Jul 22, 2019

RalfJung commented Jul 22, 2019

est31 commented Jul 22, 2019

est31 commented Jul 22, 2019

RalfJung commented Jul 22, 2019

est31 commented Jul 29, 2019

erlepereira commented Jul 30, 2019

est31 commented Aug 3, 2019

erlepereira commented Aug 4, 2019

ghost commented Aug 6, 2019 •

edited by ghost

Loading

ishitatsuyuki commented Aug 7, 2019

est31 commented Aug 10, 2019

RalfJung Aug 11, 2019 •

edited

Loading

erlepereira commented Aug 12, 2019 •

edited

Loading

Fix SIGILL on nightly rustc #101

Fix SIGILL on nightly rustc #101

Conversation

ishitatsuyuki commented Jul 19, 2019

bschwind Jul 19, 2019

Choose a reason for hiding this comment

bschwind commented Jul 19, 2019

ishitatsuyuki commented Jul 19, 2019

bschwind commented Jul 19, 2019

bjorn3 commented Jul 19, 2019

RalfJung commented Jul 19, 2019

RalfJung commented Jul 19, 2019 • edited Loading

est31 commented Jul 21, 2019 • edited Loading

ishitatsuyuki commented Jul 22, 2019

RalfJung commented Jul 22, 2019

est31 commented Jul 22, 2019

RalfJung commented Jul 22, 2019

est31 commented Jul 22, 2019

est31 commented Jul 22, 2019

RalfJung commented Jul 22, 2019

est31 commented Jul 29, 2019

erlepereira commented Jul 30, 2019

est31 commented Aug 3, 2019

erlepereira commented Aug 4, 2019

ghost commented Aug 6, 2019 • edited by ghost Loading

ishitatsuyuki commented Aug 7, 2019

est31 commented Aug 10, 2019

RalfJung Aug 11, 2019 • edited Loading

Choose a reason for hiding this comment

erlepereira commented Aug 12, 2019 • edited Loading

RalfJung commented Jul 19, 2019 •

edited

Loading

est31 commented Jul 21, 2019 •

edited

Loading

ghost commented Aug 6, 2019 •

edited by ghost

Loading

RalfJung Aug 11, 2019 •

edited

Loading

erlepereira commented Aug 12, 2019 •

edited

Loading