-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
replace libc::res_init with res_init_if_glibc_before_2_26 #44965
Conversation
r? @dtolnay (rust_highfive has picked a reviewer for you, use r? to override) |
00f9628
to
8614587
Compare
This looks like a sensible change. Two observations: first, it's a little bit unfortunate that the glibc version string has to be re-parsed each time; and second, if the glibc version fails to parse, the method should probably just return |
Yeah. I was worried that some future glibc version might decide that their version string will be As far as parsing goes, it doesn't do any allocation, and I think it should be fast enough that no one will ever notice. It just walks this tiny string a couple times to verify UTF8 and to find the dots, then parses a couple ints. But I haven't measured it. |
True, but it does have to access more memory, leading to some cache effects. Might not matter given that this is on a network I/O path though. There's something to be said for this making use of |
|
@oconnor0 no, in all likelihood, they will both incur the same number of cache misses, which is where the CPU spends most of its time anyway. that said, the current code will hold up more registers, which might have a minor effect on the resulting compiled code. should be minor though. |
8614587
to
99f191e
Compare
99f191e
to
347c466
Compare
Ok, I've updated the PR to be more concise, and added a couple tests. |
src/libstd/sys/unix/net.rs
Outdated
// Returns Some((major, minor)) if the string is a valid "x.y" version, | ||
// ignoring any extra dot-separated parts. Otherwise return None. | ||
fn parse_glibc_version(version: &str) -> Option<(u64, u64)> { | ||
let mut parsed_ints = version.split(".").map(str::parse::<u64>).fuse(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would parse this as usize
instead. u64
seems like overkill, and will be slower on non-64-bit platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my head this is a tradeoff between "performance differences too small to ever notice" and "inputs so pathological that we'll never see them". Does it feel the same way to you, or are there maybe situations I'm not thinking of where the performance might matter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm more thinking "why not use usize
"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be inclined to parse as usize
as well. The only relevant failure mode seems far-fetched.
- Glibc releases their first 1.x version since 1.09.5 in 1995 and
- The version number is >= 1.65536 and
- Has the bug that makes us want to call res_init.
Otherwise, as far as we are concerned it is safe to treat any version bigger than usize-max as not glibc.
src/libstd/sys/unix/net.rs
Outdated
("0.0", Some((0, 0))), | ||
("01.+2", Some((1, 2))), | ||
("3.4.5.six", Some((3, 4))), | ||
("9999999.9999999", Some((9999999, 9999999))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an interesting test-case? It wouldn't work on 16-bit platforms (if that's something we care about at all) if we move to usize
instead of u64
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That might be a good reason to prefer u32
or u64
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we ever expecting the version number to grow that large?
(I see @jonhoo's comments coming in above, but writing this in parallel.) Tested by hand with the following steps. First add print statements like this: diff --git a/src/libstd/sys/unix/net.rs b/src/libstd/sys/unix/net.rs
index 94f24889e6..fccf314849 100644
--- a/src/libstd/sys/unix/net.rs
+++ b/src/libstd/sys/unix/net.rs
@@ -375,8 +375,11 @@ impl IntoInner<c_int> for Socket {
pub fn res_init_if_glibc_before_2_26() -> io::Result<()> {
// If the version fails to parse, we treat it the same as "not glibc".
if let Some(Ok(version_str)) = glibc_version_cstr().map(CStr::to_str) {
+ println!("{:?}", version_str);
if let Some(version) = parse_glibc_version(version_str) {
+ println!("{:?}", version);
if version < (2, 26) {
+ println!("calling res_init");
let ret = unsafe { libc::res_init() };
if ret != 0 {
return Err(io::Error::last_os_error()); Then run something like this:
On my system (Arch Linux, glibc 2.26), I see this output:
If I forcibly downgrade myself to glibc 2.25, I see this output:
|
src/libstd/sys_common/net.rs
Outdated
// https://github.com/rust-lang/rust/issues/41570 and | ||
// https://github.com/rust-lang/rust/issues/43592. | ||
use sys::net::res_init_if_glibc_before_2_26; | ||
res_init_if_glibc_before_2_26()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewers: should we propagate res_init errors or suppress them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They were suppressed before, and as far as we know that is fine / not the bug we are trying to fix here, so I would prefer to keep errors suppressed.
@dtolnay let me know if I should tag a different reviewer? |
Thanks for the reminder! I left two comments on the existing threads but otherwise looks good to me. |
Very well, |
The previous workaround for gibc's res_init bug is not thread-safe on other implementations of libc, and it can cause crashes. Use a runtime check to make sure we only call res_init when we need to, which is also when it's safe. See rust-lang#43592.
347c466
to
9602fe1
Compare
@bors r+ |
📌 Commit 9602fe1 has been approved by |
replace libc::res_init with res_init_if_glibc_before_2_26 The previous workaround for gibc's res_init bug is not thread-safe on other implementations of libc, and it can cause crashes. Use a runtime check to make sure we only call res_init when we need to, which is also when it's safe. See #43592. ~This PR is returning an InvalidData IO error if the glibc version string fails to parse. We could also have treated that case as "not glibc", and gotten rid of the idea that these functions could return an error. (Though I'm not a huge fan of ignoring error returns from `res_init` in any case.) Do other folks agree with these design choices?~ I'm pretty new to hacking on libstd. Is there an easy way to build a toy rust program against my changes to test this, other than doing an entire `sudo make install` on my system? What's the usual workflow?
☀️ Test successful - status-appveyor, status-travis |
…acrum Don't link to `libresolv` in libstd on Darwin Currently we link `libresolv` into every Rust program on apple targets despite never using it (as of rust-lang#44965). I had thought we needed this for `getaddrinfo` or something, but we do not / cannot safely use it. I'd like to fix this for `libiconv` too (the other library we pull in. that's harder since it's coming in through `libc`, which is rust-lang/libc#2944)). --- This may warrant release notes. I'm not sure but I've added the flag regardless -- It's a change to the list of dylibs every Rust program pulls in, so it's worth mentioning. It's pretty unlikely anybody was relying on this being pulled in, and `std` does not guarantee that it will link (and thus transitively provide access to) any particular system library -- anybody relying on that behavior would already be broken when dynamically linking std. That is, there's an outside chance something will fail to link on macOS and iOS because it was accidentally relying on our unnecessary dependency. (If that *does* happen, that project could be easily fixed by linking libresolv explicitly on those platforms, probably via `#[link(name = "resolv")] extern {}`,` -Crustc-link-lib=resolv`, `println!("cargo:rustc-link-lib=resolv")`, or one of several places in `.config/cargo.toml`) --- I'm also going to preemptively add the nomination for discussing this in the libs meeting. Basically: Do we care about programs that assume we will bring libraries in that we do not use. `libresolv` and `libiconv` on macOS/iOS are in this camp (`libresolv` because we used to use it, and `libiconv` because the `libc` crate was unintentionally(?) pulling it in to every Rust program). I'd like to remove them both, but this may cause link issues programs that are relying on `std` to depend on them transitively. (Relying on std for this does not work in all build configurations, so this seems very fragile, and like a use case we should not support). More generally, IMO we should not guarantee the specific set of system-provided libraries we use (beyond what is implied by an OS version requirement), which means we'd be free to remove this cruft.
…acrum Don't link to `libresolv` in libstd on Darwin Currently we link `libresolv` into every Rust program on apple targets despite never using it (as of rust-lang#44965). I had thought we needed this for `getaddrinfo` or something, but we do not / cannot safely use it. I'd like to fix this for `libiconv` too (the other library we pull in. that's harder since it's coming in through `libc`, which is rust-lang/libc#2944)). --- This may warrant release notes. I'm not sure but I've added the flag regardless -- It's a change to the list of dylibs every Rust program pulls in, so it's worth mentioning. It's pretty unlikely anybody was relying on this being pulled in, and `std` does not guarantee that it will link (and thus transitively provide access to) any particular system library -- anybody relying on that behavior would already be broken when dynamically linking std. That is, there's an outside chance something will fail to link on macOS and iOS because it was accidentally relying on our unnecessary dependency. (If that *does* happen, that project could be easily fixed by linking libresolv explicitly on those platforms, probably via `#[link(name = "resolv")] extern {}`,` -Crustc-link-lib=resolv`, `println!("cargo:rustc-link-lib=resolv")`, or one of several places in `.config/cargo.toml`) --- I'm also going to preemptively add the nomination for discussing this in the libs meeting. Basically: Do we care about programs that assume we will bring libraries in that we do not use. `libresolv` and `libiconv` on macOS/iOS are in this camp (`libresolv` because we used to use it, and `libiconv` because the `libc` crate was unintentionally(?) pulling it in to every Rust program). I'd like to remove them both, but this may cause link issues programs that are relying on `std` to depend on them transitively. (Relying on std for this does not work in all build configurations, so this seems very fragile, and like a use case we should not support). More generally, IMO we should not guarantee the specific set of system-provided libraries we use (beyond what is implied by an OS version requirement), which means we'd be free to remove this cruft.
…acrum Don't link to `libresolv` in libstd on Darwin Currently we link `libresolv` into every Rust program on apple targets despite never using it (as of rust-lang#44965). I had thought we needed this for `getaddrinfo` or something, but we do not / cannot safely use it. I'd like to fix this for `libiconv` too (the other library we pull in. that's harder since it's coming in through `libc`, which is rust-lang/libc#2944)). --- This may warrant release notes. I'm not sure but I've added the flag regardless -- It's a change to the list of dylibs every Rust program pulls in, so it's worth mentioning. It's pretty unlikely anybody was relying on this being pulled in, and `std` does not guarantee that it will link (and thus transitively provide access to) any particular system library -- anybody relying on that behavior would already be broken when dynamically linking std. That is, there's an outside chance something will fail to link on macOS and iOS because it was accidentally relying on our unnecessary dependency. (If that *does* happen, that project could be easily fixed by linking libresolv explicitly on those platforms, probably via `#[link(name = "resolv")] extern {}`,` -Crustc-link-lib=resolv`, `println!("cargo:rustc-link-lib=resolv")`, or one of several places in `.config/cargo.toml`) --- I'm also going to preemptively add the nomination for discussing this in the libs meeting. Basically: Do we care about programs that assume we will bring libraries in that we do not use. `libresolv` and `libiconv` on macOS/iOS are in this camp (`libresolv` because we used to use it, and `libiconv` because the `libc` crate was unintentionally(?) pulling it in to every Rust program). I'd like to remove them both, but this may cause link issues programs that are relying on `std` to depend on them transitively. (Relying on std for this does not work in all build configurations, so this seems very fragile, and like a use case we should not support). More generally, IMO we should not guarantee the specific set of system-provided libraries we use (beyond what is implied by an OS version requirement), which means we'd be free to remove this cruft.
Don't link to `libresolv` in libstd on Darwin Currently we link `libresolv` into every Rust program on apple targets despite never using it (as of rust-lang/rust#44965). I had thought we needed this for `getaddrinfo` or something, but we do not / cannot safely use it. I'd like to fix this for `libiconv` too (the other library we pull in. that's harder since it's coming in through `libc`, which is rust-lang/libc#2944)). --- This may warrant release notes. I'm not sure but I've added the flag regardless -- It's a change to the list of dylibs every Rust program pulls in, so it's worth mentioning. It's pretty unlikely anybody was relying on this being pulled in, and `std` does not guarantee that it will link (and thus transitively provide access to) any particular system library -- anybody relying on that behavior would already be broken when dynamically linking std. That is, there's an outside chance something will fail to link on macOS and iOS because it was accidentally relying on our unnecessary dependency. (If that *does* happen, that project could be easily fixed by linking libresolv explicitly on those platforms, probably via `#[link(name = "resolv")] extern {}`,` -Crustc-link-lib=resolv`, `println!("cargo:rustc-link-lib=resolv")`, or one of several places in `.config/cargo.toml`) --- I'm also going to preemptively add the nomination for discussing this in the libs meeting. Basically: Do we care about programs that assume we will bring libraries in that we do not use. `libresolv` and `libiconv` on macOS/iOS are in this camp (`libresolv` because we used to use it, and `libiconv` because the `libc` crate was unintentionally(?) pulling it in to every Rust program). I'd like to remove them both, but this may cause link issues programs that are relying on `std` to depend on them transitively. (Relying on std for this does not work in all build configurations, so this seems very fragile, and like a use case we should not support). More generally, IMO we should not guarantee the specific set of system-provided libraries we use (beyond what is implied by an OS version requirement), which means we'd be free to remove this cruft.
The previous workaround for gibc's res_init bug is not thread-safe on
other implementations of libc, and it can cause crashes. Use a runtime
check to make sure we only call res_init when we need to, which is also
when it's safe. See #43592.
This PR is returning an InvalidData IO error if the glibc version string fails to parse. We could also have treated that case as "not glibc", and gotten rid of the idea that these functions could return an error. (Though I'm not a huge fan of ignoring error returns fromres_init
in any case.) Do other folks agree with these design choices?I'm pretty new to hacking on libstd. Is there an easy way to build a toy rust program against my changes to test this, other than doing an entire
sudo make install
on my system? What's the usual workflow?