Skip to content
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.

Sign up for GitHub

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

Only link res_init() on GNU/*nix #47334

Merged
merged 1 commit into from
Jan 22, 2018
Merged

Only link res_init() on GNU/*nix #47334

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions src/libstd/sys/unix/l4re.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,9 +437,5 @@ pub mod net {
pub fn lookup_host(_: &str) -> io::Result<LookupHost> {
unimpl!();
}

pub fn res_init_if_glibc_before_2_26() -> io::Result<()> {
unimpl!();
}
}

20 changes: 13 additions & 7 deletions src/libstd/sys/unix/net.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@ pub fn cvt_gai(err: c_int) -> io::Result<()> {
if err == 0 {
return Ok(())
}

// We may need to trigger a glibc workaround. See on_resolver_failure() for details.
on_resolver_failure();

if err == EAI_SYSTEM {
return Err(io::Error::last_os_error())
}
Expand Down Expand Up @@ -377,21 +381,22 @@ impl IntoInner<c_int> for Socket {
// res_init unconditionally, we call it only when we detect we're linking
// against glibc version < 2.26. (That is, when we both know its needed and
// believe it's thread-safe).
pub fn res_init_if_glibc_before_2_26() -> io::Result<()> {
#[cfg(target_env = "gnu")]
fn on_resolver_failure() {
// 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) {
if let Some(version) = parse_glibc_version(version_str) {
if version < (2, 26) {
let ret = unsafe { libc::res_init() };
if ret != 0 {
return Err(io::Error::last_os_error());
}
unsafe { libc::res_init() };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason why you chose to prefer the original resolver error over any potential res_init error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered this but:

  1. That's what the code in master is doing. The error from res_init_if_glibc_before_2_26 is discarded and the original error from getaddrinfo() is returned.

  2. Callers are probably expecting getaddrinfo()-type errors from lookup_host() (e.g. EAI_FAIL) . This is especially true since this will only be happening on an increasingly small fraction of platforms in obscure cases so it won't get much testing. I think returning the res_init() result here would be surprising.

  3. res_init() does not return a well defined set of error codes. It's not clear that's it's setting errno at all from reading the glibc manage

  4. I'm not convinced that res_init() will always be the most proximate error. It's possible that res_init() is returning a useful report on why the resolution system isn't working (but I have my doubts due to point 3). However, it's also possible that the getaddrinfo() is returning more useful information and the res_init() is failing for some inconsequential reason.

I kept the error code return from on_resolver_failure in case some platform wants to modify the error on return but it's just directly returned at the moment.

If there's a strong opinion in the other direction I can change the error behaviour but I want to reiterate that would be a functional change from master.

}
}
}
Ok(())
}

#[cfg(not(target_env = "gnu"))]
fn on_resolver_failure() {}

#[cfg(target_env = "gnu")]
fn glibc_version_cstr() -> Option<&'static CStr> {
weak! {
fn gnu_get_libc_version() -> *const libc::c_char
Expand All @@ -405,6 +410,7 @@ fn glibc_version_cstr() -> Option<&'static CStr> {

// Returns Some((major, minor)) if the string is a valid "x.y" version,
// ignoring any extra dot-separated parts. Otherwise return None.
#[cfg(target_env = "gnu")]
fn parse_glibc_version(version: &str) -> Option<(usize, usize)> {
let mut parsed_ints = version.split(".").map(str::parse::<usize>).fuse();
match (parsed_ints.next(), parsed_ints.next()) {
Expand All @@ -413,7 +419,7 @@ fn parse_glibc_version(version: &str) -> Option<(usize, usize)> {
}
}

#[cfg(test)]
#[cfg(all(test, taget_env = "gnu"))]
mod test {
use super::*;

Expand Down
24 changes: 3 additions & 21 deletions src/libstd/sys_common/net.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,27 +166,9 @@ pub fn lookup_host(host: &str) -> io::Result<LookupHost> {
hints.ai_socktype = c::SOCK_STREAM;
let mut res = ptr::null_mut();
unsafe {
match cvt_gai(c::getaddrinfo(c_host.as_ptr(), ptr::null(), &hints, &mut res)) {
Ok(_) => {
Ok(LookupHost { original: res, cur: res })
},
#[cfg(unix)]
Err(e) => {
// If we're running glibc prior to version 2.26, the lookup
// failure could be caused by caching a stale /etc/resolv.conf.
// We need to call libc::res_init() to clear the cache. But we
// shouldn't call it in on any other platform, because other
// res_init implementations aren't thread-safe. See
// 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;
let _ = res_init_if_glibc_before_2_26();
Err(e)
},
// the cfg is needed here to avoid an "unreachable pattern" warning
#[cfg(not(unix))]
Err(e) => Err(e),
}
cvt_gai(c::getaddrinfo(c_host.as_ptr(), ptr::null(), &hints, &mut res)).map(|_| {
LookupHost { original: res, cur: res }
})
}
}

Expand Down