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.

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

Make uname always safe #1672

Merged
merged 1 commit into from
Mar 24, 2022
Merged

Make uname always safe #1672

merged 1 commit into from
Mar 24, 2022

Conversation

koute
Copy link
Contributor

@koute koute commented Mar 10, 2022

Currently uname doesn't check for errors and just blindly assumes that it always succeeds. According to the manpage this function can fail, even though no actual errors are defined:

RETURN VALUE
       Upon successful completion, a non-negative value shall be returned.  Otherwise, -1 shall be returned and errno set to indicate the error.

ERRORS
       No errors are defined.

       The following sections are informative.

Looking at the glibc's sources we can see that it indeed could fail if the internal gethostname call fails for some reason.

This code also assumes that every field of utsname is going to be initialized by the call to uname, which apparently is also not true. Even though the interface doesn't expose this field so it's not a problem in practice (although it might be UB since we do call assume_init on the whole struct) the utsname does have a domainname field which glibc doesn't initialize.

The code also assumes that every field is a valid UTF-8 string, which is also technically not guaranteed.

The code also assumes that every field will be null terminated, which might not be true if any of the strings are too long (since glibc uses strncpy which will not null-terminate the string if it ends up running out of space).

This PR should fix all of these problems.

This is a breaking change.

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

Do you know of any cases where the strings aren't valid UTF-8 ? It is certainly more convenient for the user if we return a &str than a &[u8]. Perhaps do a checked conversion to utf8?

src/sys/utsname.rs Outdated Show resolved Hide resolved
src/sys/utsname.rs Outdated Show resolved Hide resolved
@koute
Copy link
Contributor Author

koute commented Mar 16, 2022

Do you know of any cases where the strings aren't valid UTF-8 ?

Not specifically, but e.g. someone could set a non-UTF8 hostname with sethostname which will end up in the nodename returned here.

It is certainly more convenient for the user if we return a &str than a &[u8]. Perhaps do a checked conversion to utf8?

Well, I see three possibilities here:

  1. Return a &[u8].
  2. Return a &str and panic if it can't be converted.
  3. Return a Result<&str, Utf8Error>.

Converting the &[u8] to string on the user's side is just a matter of changing utsname.nodename() to std::str::from_utf8(utsname.nodename())? so it's not that big of a deal, and it's not like this is a super frequently used function. I'd rather not go for (2) because I think libraries shouldn't panic on errors, and (3) feels like a halfway measure.

@asomers
Copy link
Member

asomers commented Mar 19, 2022

What if it returns an OsString? Those are allowed to be non-UTF8. See fcntl::readlink for an example. Would that work?

@koute
Copy link
Contributor Author

koute commented Mar 20, 2022

What if it returns an OsString? Those are allowed to be non-UTF8. See fcntl::readlink for an example. Would that work?

OsString allocates so that's not great, but I guess &OsStr should indeed work. Done!

@rtzoeller
Copy link
Collaborator

LGTM; can you squash and rebase?

This fixes several issues with the current `uname` bindings:

  - Do not ignore `uname` errors; at least on glibc `uname` can fail,
    so now it returns a `Result` instead of assuming that the call
    will always succeed.

  - Do not assume `uname` will initialize every member of `utsname`;
    not every implementation initializes every field, so internally
    the struct is now zero-initialized.

  - Do not blindly assume strings returned by `uname` will always be valid UTF-8;
    `UtsName`'s accessors will now return `&OsStr`s instead of `&str`s.
@koute
Copy link
Contributor Author

koute commented Mar 23, 2022

LGTM; can you squash and rebase?

Done!

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

bors r+

@bors bors bot merged commit 4ccc6c6 into nix-rust:master Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants