Skip to content
This repository has been archived by the owner on Jan 25, 2024. It is now read-only.

Gethostbyname: use proper glibc function #9

Merged
merged 1 commit into from
Oct 24, 2023
Merged

Conversation

picnoir
Copy link
Member

@picnoir picnoir commented Oct 8, 2023

We internally used getai to restpond to the gethostbyname operations. Sadly, it did not behave as expected and broke some tools (like hostname --fqdn, see #4.

We FFI the right Glibc gethostbyname_2r (now deprecated) function and use it to back the GETHOSTBYNAME and GETHOSTBYNAME6 Nscd interfaces.

Fixes #4

src/ffi.rs Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Oct 8, 2023

Codecov Report

❗ No coverage uploaded for pull request base (host-lookups@94f54bb). Click here to learn what that means.
The diff coverage is n/a.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@               Coverage Diff               @@
##             host-lookups       #9   +/-   ##
===============================================
  Coverage                ?   58.29%           
===============================================
  Files                   ?        6           
  Lines                   ?      458           
  Branches                ?        0           
===============================================
  Hits                    ?      267           
  Misses                  ?      191           
  Partials                ?        0           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@picnoir picnoir force-pushed the nin/ghbn branch 2 times, most recently from 0eaa3ad to c175727 Compare October 8, 2023 15:09
@picnoir picnoir marked this pull request as ready for review October 8, 2023 17:19
@picnoir picnoir requested a review from flokli October 8, 2023 17:19
src/ffi.rs Outdated Show resolved Hide resolved
src/ffi.rs Outdated Show resolved Hide resolved
src/ffi.rs Outdated Show resolved Hide resolved
src/ffi.rs Outdated Show resolved Hide resolved
src/handlers.rs Outdated Show resolved Hide resolved
src/handlers.rs Outdated Show resolved Hide resolved
src/handlers.rs Outdated Show resolved Hide resolved
src/handlers.rs Outdated Show resolved Hide resolved
src/handlers.rs Outdated Show resolved Hide resolved
src/ffi.rs Show resolved Hide resolved
src/ffi.rs Outdated Show resolved Hide resolved
src/ffi.rs Outdated Show resolved Hide resolved
@picnoir
Copy link
Member Author

picnoir commented Oct 16, 2023

Adressed Flokli's nits. Seems to work. I'll use that on my systems the whole day to see if it breaks more stuff.

@picnoir
Copy link
Member Author

picnoir commented Oct 16, 2023

I tested that for the day. 2 issues with this:

  1. there's something wrong with gethostbyname (both v4 and v6). Most of the function calls are failing with "HOST_NOT_FOUND".
  2. there's also something wrong with the error number we're getting back from the glibc functions. This is a h_errno, not a errno.

Here's the h_errno definition:

#define HOST_NOT_FOUND 1
#define TRY_AGAIN      2
#define NO_RECOVERY    3
#define NO_DATA        4
#define NO_ADDRESS     NO_DATA

@picnoir
Copy link
Member Author

picnoir commented Oct 19, 2023

Turns out, a lot of things were wrong. I ended up rewriting this PR from scratch twice since last commit…

I think I finally found the right abstraction for hostent... ...and the right defaults wrt. error management. More information in the commit message.

According to sockburp, we're bit-to-bit identical to Nscd for these two new operations now.

[Edit]: hmm, seems like the CI setup has more aliases than my NixOS desktop for localhost. This is annoying, trying to find a more "stable" test case across different hosts.

src/ffi.rs Outdated Show resolved Hide resolved
src/handlers.rs Outdated Show resolved Hide resolved
@picnoir picnoir force-pushed the nin/ghbn branch 2 times, most recently from 3f7da03 to 0d96cf3 Compare October 20, 2023 07:04
@picnoir
Copy link
Member Author

picnoir commented Oct 20, 2023

I've been using this PR for the last 12h. Sockburp is happy, it seems to behave correctly.

@flokli
Copy link
Collaborator

flokli commented Oct 20, 2023

This fails to build on aarch64-linux:

error[E0308]: mismatched types
   --> src/ffi.rs:209:17
    |
204 |             glibcffi::gethostbyaddr_r(
    |             ------------------------- arguments to this function are incorrect
...
209 |                 buf.as_mut_ptr() as *mut i8,
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `*mut u8`, found `*mut i8`
    |
    = note: expected raw pointer `*mut u8`
               found raw pointer `*mut i8`
note: function defined here
   --> src/ffi.rs:65:16
    |
65  |         pub fn gethostbyaddr_r (
    |                ^^^^^^^^^^^^^^^

error[E0308]: mismatched types
   --> src/ffi.rs:253:17
    |
249 |             glibcffi::gethostbyname2_r(
    |             -------------------------- arguments to this function are incorrect
...
253 |                 buf.as_mut_ptr() as *mut i8,
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `*mut u8`, found `*mut i8`
    |
    = note: expected raw pointer `*mut u8`
               found raw pointer `*mut i8`
note: function defined here
   --> src/ffi.rs:55:16
    |
55  |         pub fn gethostbyname2_r (
    |                ^^^^^^^^^^^^^^^^

For more information about this error, try `rustc --explain E0308`.
error: could not compile `nsncd` (bin "nsncd") due to 2 previous errors

@picnoir
Copy link
Member Author

picnoir commented Oct 20, 2023

~/code-root/github.com/nix-community/nsncd (hande*) » cargo build                                          101 ↵ ninjatrappeur@framework
   Compiling nsncd v1.4.1 (/home/ninjatrappeur/code-root/github.com/nix-community/nsncd)
error[E0308]: mismatched types
   --> src/ffi.rs:209:17
    |
204 |             glibcffi::gethostbyaddr_r(
    |             ------------------------- arguments to this function are incorrect
...
209 |                 buf.as_mut_ptr() as *mut u8,
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `*mut i8`, found `*mut u8`
    |
    = note: expected raw pointer `*mut i8`
               found raw pointer `*mut u8`
note: function defined here
   --> src/ffi.rs:65:16
    |
65  |         pub fn gethostbyaddr_r (

^ on x86_64.

What the hell.

EDIT: got it. c_char is u8 on arm64, i8 on x86. Fixing that.

We internally used getai to restpond to the
gethostbyname/gethostbyaddr operations. Sadly, it does not behave as
expected and breaks some tools (like hostname --fqdn, see
#4.

We FFI the right Glibc gethostbyname_2r/gethostbyaddr_2r (now
deprecated) functions and use it to back the GETHOSTBYNAME,
GETHOSTBYNAME6, GETHOSTBYADDR, and GETHOSTBYADDR6 Nscd interfaces.

Using sockburp, we realized the hostent serialization function was
bogus: we totally forgot to serialize the aliases. This commit fixes
this and makes sure we're producing bit-to-bit identical results with
Nscd for gethostbyname/getaddrinfo.

Took me three try to get this right. This is actually the third
full rewrite.

The Nscd behaviour for these two legacy functions is *really*
confusing. We're supposed to ignore the herrno (herrno != errno!!) and
set it to 0 if gethostbyaddr/name returns a non-null hostent. If we
end up with a null hostent, we return the herrno together with a dummy
hostent header.

I tried to keep things as safe as possible by extracting the glibc
hostent to a proper rust structure. This structure mirrors the libc
hostent in a Rust idiomatic way. We should probably try to upstream
the FFI part of this commit to the Nix crate at some point.

Fixes #4
@picnoir
Copy link
Member Author

picnoir commented Oct 20, 2023

@flokli: PTAL on aarch64

@flokli
Copy link
Collaborator

flokli commented Oct 20, 2023

Yes, now builds and works on aarch64-linux!

@flokli
Copy link
Collaborator

flokli commented Oct 24, 2023

We both tested this for an extended period of time, and also validated with sockburp comparing responses from nsncd with glibc-nscd. Other than DNS servers returning responses sometimes in different order, there seems to be no differences for host-related lookups. Let's get this in!

@flokli flokli merged commit 77fd58d into host-lookups Oct 24, 2023
9 checks passed
@Mic92 Mic92 deleted the nin/ghbn branch October 24, 2023 14:06
picnoir added a commit to picnoir/nixpkgs that referenced this pull request Oct 26, 2023
Note: we decided to rewrite the history of the fork who somehow got
out of hand. Feature-wise, this version bump fixes the various host
faulty behaviour. See the
nix-community/nsncd#9 and
nix-community/nsncd#10 PRs for more details.

We're in the process of upstreaming this change to twosigma/nsncd,
however, upstream has been pretty slow to review our PRs so far. Since
the hostname bug surfaces quite regularly in the Nixpkgs issue
tracker, we decided to use the nix-community fork as canon for Nixpkgs
for now.

Fixes: NixOS#132646
Fixes: NixOS#261269
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants