637 Solaris getrandom() support breaks illumos #730
Conversation
|
The one failure seems odd -- it's for building android (which segfaults) while none of this code should touch anything for any android targets. Is that perhaps an unrelated issue? |
There was a problem hiding this comment.
Other than as noted, looks good.
Regarding the test failure, yes, you can ignore it: #701
Have you tested on Solaris and Illumos?
Could you also do the same for getrandom afterwards please? At the moment this is mostly duplicate code, but later we plan to make OsRng a wrapper around that, so for now need to keep both in sync (aside from the slightly different API).
|
Thanks! I'll follow up with a similar PR for rust-random/getrandom soon as well. |
|
Thank you for contributing. Are you happy to wait until we next update |
|
How long do you think it'll be before the next update? A few weeks I don't think would be a big deal, if it's a few months though it'd probably be useful to have something released sooner -- since rustc depends on the rand crate to build, currently building rust on illumos has required manually patching the rand crate, so it'd be nice to be able to stop doing that (though now at least we can also just patch Cargo.toml instead to pull master from GH instead as a better workaround). |
|
I don't know, but likely at least a couple of weeks more, so I guess it makes sense to release these changes now. |
On Solaris, we opt to use /dev/random source instead of /dev/urandom due to reasons explained in the comments and [in this Solaris blog post](https://blogs.oracle.com/solaris/post/solaris-new-system-calls-getentropy2-and-getrandom2). However, we haven't been making the same choice when getting randomness via the `getrandom(2)` function, as we just pass `0` for the flags. We [used to](https://github.com/rust-random/rand/pull/730/files#diff-694d4302a3ff2a976f2fbd34bc05ada22ee61a4e21d2d985beab27f7a809268fR151) always set `GRND_RANDOM`, but that was removed in the move from `OsRng` to this crate. For context, rust-random/rand#730, #9, and #51 are the major changes to the Solaris/Illumos implementation over the years. See the solaris documentation for: - [`getrandom(2)`](https://docs.oracle.com/cd/E88353_01/html/E37841/getrandom-2.html) - [`urandom(4)`](https://docs.oracle.com/cd/E88353_01/html/E37851/urandom-4d.html) I also updated the doucmentation to better reflect when [Illumos added the `getrandom(2)` function](https://www.illumos.org/issues/9971#change-23483). Signed-off-by: Joe Richey <[email protected]>
On Solaris, we opt to use /dev/random source instead of /dev/urandom due to reasons explained in the comments and [in this Solaris blog post](https://blogs.oracle.com/solaris/post/solaris-new-system-calls-getentropy2-and-getrandom2). However, we haven't been making the same choice when getting randomness via the `getrandom(2)` function, as we just pass `0` for the flags. We [used to](https://github.com/rust-random/rand/pull/730/files#diff-694d4302a3ff2a976f2fbd34bc05ada22ee61a4e21d2d985beab27f7a809268fR151) always set `GRND_RANDOM`, but that was removed in the move from `OsRng` to this crate. For context, rust-random/rand#730, #9, and #51 are the major changes to the Solaris/Illumos implementation over the years. See the solaris documentation for: - [`getrandom(2)`](https://docs.oracle.com/cd/E88353_01/html/E37841/getrandom-2.html) - [`urandom(4)`](https://docs.oracle.com/cd/E88353_01/html/E37851/urandom-4d.html) I also updated the doucmentation to better reflect when [Illumos added the `getrandom(2)` function](https://www.illumos.org/issues/9971#change-23483). Signed-off-by: Joe Richey <[email protected]>
On Solaris, we opt to use /dev/random source instead of /dev/urandom due to reasons explained in the comments and [in this Solaris blog post](https://blogs.oracle.com/solaris/post/solaris-new-system-calls-getentropy2-and-getrandom2). However, we haven't been making the same choice when getting randomness via the `getrandom(2)` function, as we just pass `0` for the flags. We [used to](https://github.com/rust-random/rand/pull/730/files#diff-694d4302a3ff2a976f2fbd34bc05ada22ee61a4e21d2d985beab27f7a809268fR151) always set `GRND_RANDOM`, but that was removed in the move from `OsRng` to this crate. For context, rust-random/rand#730, #9, and #51 are the major changes to the Solaris/Illumos implementation over the years. See the solaris documentation for: - [`getrandom(2)`](https://docs.oracle.com/cd/E88353_01/html/E37841/getrandom-2.html) - [`urandom(4)`](https://docs.oracle.com/cd/E88353_01/html/E37851/urandom-4d.html) I also updated the doucmentation to better reflect when [Illumos added the `getrandom(2)` function](https://www.illumos.org/issues/9971#change-23483). Signed-off-by: Joe Richey <[email protected]>
On Solaris, we opt to use /dev/random source instead of /dev/urandom due to reasons explained in the comments and [in this Solaris blog post](https://blogs.oracle.com/solaris/post/solaris-new-system-calls-getentropy2-and-getrandom2). However, we haven't been making the same choice when getting randomness via the `getrandom(2)` function, as we just pass `0` for the flags. We [used to](https://github.com/rust-random/rand/pull/730/files#diff-694d4302a3ff2a976f2fbd34bc05ada22ee61a4e21d2d985beab27f7a809268fR151) always set `GRND_RANDOM`, but that was removed in the move from `OsRng` to this crate. For context, rust-random/rand#730, #9, and #51 are the major changes to the Solaris/Illumos implementation over the years. See the solaris documentation for: - [`getrandom(2)`](https://docs.oracle.com/cd/E88353_01/html/E37841/getrandom-2.html) - [`urandom(4)`](https://docs.oracle.com/cd/E88353_01/html/E37851/urandom-4d.html) I also updated the doucmentation to better reflect when [Illumos added the `getrandom(2)` function](https://www.illumos.org/issues/9971#change-23483). Signed-off-by: Joe Richey <[email protected]>
On Solaris, we opt to use /dev/random source instead of /dev/urandom due to reasons explained in the comments and [in this Solaris blog post](https://blogs.oracle.com/solaris/post/solaris-new-system-calls-getentropy2-and-getrandom2). However, we haven't been making the same choice when getting randomness via the `getrandom(2)` function, as we just pass `0` for the flags. We [used to](https://github.com/rust-random/rand/pull/730/files#diff-694d4302a3ff2a976f2fbd34bc05ada22ee61a4e21d2d985beab27f7a809268fR151) always set `GRND_RANDOM`, but that was removed in the move from `OsRng` to this crate. For context, rust-random/rand#730, #9, and #51 are the major changes to the Solaris/Illumos implementation over the years. See the solaris documentation for: - [`getrandom(2)`](https://docs.oracle.com/cd/E88353_01/html/E37841/getrandom-2.html) - [`urandom(4)`](https://docs.oracle.com/cd/E88353_01/html/E37851/urandom-4d.html) I also updated the doucmentation to better reflect when [Illumos added the `getrandom(2)` function](https://www.illumos.org/issues/9971#change-23483). Signed-off-by: Joe Richey <[email protected]>
| -> libc::ssize_t; | ||
| #[cfg(target_os = "solaris")] | ||
| type GetRandomFn = unsafe extern fn(*mut u8, libc::size_t, libc::c_uint) | ||
| -> libc::c_int; |
There was a problem hiding this comment.
I stumbled upon this while investigating rust-lang/miri#3924. In this code, the return type is different between Illumos and Solaris. However, in today's libc, the return type is the same for both OSes. (This was added in rust-lang/libc#2236.)
Do you remember where you got the different return type from?
There was a problem hiding this comment.
It was like this since getrandom v0.1 days: https://docs.rs/crate/getrandom/0.1.0/source/src/solaris_illumos.rs#28-31
We probably just copied what std done at the time.
There was a problem hiding this comment.
In the current version of getrandom we use libc::getrandom, so it should no longer be an issue with up-to-date dependencies.
There was a problem hiding this comment.
Ah getrandom is older than this? Okay.
so it should no longer be an issue with up-to-date dependencies.
Yeah, I am just trying to figure out where the other return type comes from and which one is correct...
There was a problem hiding this comment.
rand_os is an older crate. It was eventually deprecated in favor of getrandom.
There was a problem hiding this comment.
There are other traces pointing at Solaris having a different return type, see rust-lang/miri#3924:
There was a problem hiding this comment.
I mean the version pre-change.
The getrandom() function was something that was added to both OSes after both forked. There is no cross-pollination between the two projects. Oracle does their own thing and the only notice you get about anything is when a release comes out. illumos does what we feel is best for us -- while we strive to not break existing stuff, when adding new interfaces, while compatibility with Solaris may be a factor, it is far from the only (or most important) factor.
In this case, getrandom() was added after illumos forked from Solaris. I'm not sure which one got it first, but the illumos version was modeled after FreeBSD. It appears that Oracle changed the function signature in Solaris 11.4 so it was the same as the illumos and FreeBSD version (you can look at the section 2 man pages of Solaris 11.3 and Solaris 11.4).
The use of dlsym was to better support the existing is_getrandom_available test. Issuing unsupported syscalls on Solaris and illumos systems (as noted in the comments) generates a SYSSIG signal which is usually unhandled by a program (and thus usually crashes it). Checking for the libc function is less likely to cause problems.
There was a problem hiding this comment.
I don't know why you are even talking about Illumos. My question is entirely and exclusively about Solaris. For the purpose of this discussion, I am treating the two as completely independent OSes.
EDIT: I probably wasn't sufficiently clear in the original question here. That's why I linked to rust-lang/miri#3924 for context. :)
There was a problem hiding this comment.
Specifically, the question is, what is the return type for getrandom on Solaris? This PR made it c_int. But today in the libc crate, it is ssize_t. Who is right? Or did Solaris change the return type of this function?
There was a problem hiding this comment.
It appears that Oracle changed the function signature in Solaris 11.4
That's our best running theory. It seems very strange to me that one can just change such a function signature but maybe that works for Oracle for some reason?
That hypothesis explains basically everything, except for https://www.gnu.org/software/gnulib/manual/html_node/getrandom.html which says
This function has a different return type on some platforms: Solaris 11.4.
This should fix getrandom(2)(#637 ) both illumos and Solaris. While there is no official term to collectively refer to Solaris and it's open-source derivatives, the term 'Solarish' is the closest thing to one, so I've adopted that to indicate both Solaris and Solaris derivatives.
It also includes support for an illumos OS target in support of rust-lang/rust#55553 -- since the rust compiler depends on the rand crate, one cannot build a new OS target until the rand crate (amongst a few other crates) supports the OS, so I'm submitting this one first.