Habitat: hab-sup erroneously reports missing group, fails to load service

[stevendanna]

On hab 0.90.6 (and possibly older), hab-sup can fail to start a service with the following error:

hab-sup(MR): Starting chef/deployment-service (chef/deployment-service/0.1.0/20191211163131)
hab-sup(MR): Can't create directory /hab/svc/deployment-service: Package requires group hab to exist, but it doesn't
hab-sup(MR): If this service is running as non-root, you'll need to create /hab/svc/deployment-service and give the curre...ccess to it

even when the hab group exists. This is because neither our code or the underlying library we use to get the group by name surfaces errors from getgrnam_r.

Our code always reports this error whenever the library returns None:

https://github.com/habitat-sh/habitat/blob/master/components/core/src/os/users/linux.rs#L77-L89

but the underlying library returns None in the case of an error:

https://github.com/ogham/rust-users/blob/master/src/base.rs#L484-L488

We've recently seen such an error occur because the user had an /etc/groups entry that was too large for the hard-coded buffer value of 2048 bytes. (I can provide a reproduction if you'd like to see the problem locally. Note that because of the way glibc's getgrname works, _any_ entry that precedes the entry you are looking for that requires a larger buffer will cause a failure.):

https://github.com/ogham/rust-users/blob/master/src/base.rs#L474

In our own code, we cannot simply check last_os_error() because the library does not give us access to the error code from the underlying system call. Since errno is not reset to 0 on success so we have no way to distinguish an error in this system call from an error in a previous system call on this thread.

While we could fix part of this upstream, completely fixing the problems with this call would require a breaking change to their library. Namely, this api should have a return value of
Result<Option<Group>> rather than Option<Group>.

Alternatively, I think we should consider making the nix crate our default system-interface dependency for linux in any case where we don't want to call libc directly. For instances, the nix version of this function already has the correct return value:

https://github.com/nix-rust/nix/blob/5cb526d55df223735602d2f50cae4c9b43120d9c/src/unistd.rs#L2724

and already handles buffer resizing:

https://github.com/nix-rust/nix/blob/5cb526d55df223735602d2f50cae4c9b43120d9c/src/unistd.rs#L2564-L2566

[christophermaier]

I think the idea of switching over to the nix crate for a lot of our lower-level system interaction has been floated before; I think it makes sense to investigate that further.

[stevendanna]

Yeah, I think this is a great first use case to pull it in as a dep since the bug here is really just the result of the fact that we didn't use a rather crufty API correctly.

[stevendanna]

I have a branch that moves us to nix and hit: https://github.com/nix-rust/nix/pull/1205

The branch is here: https://github.com/stevendanna/habitat/commit/4e9e3bb68d2a1ecc553fcd4262c92c9f768c79e5

As I explain in that commit, because nix imposes a maximum buffer size, we still will hit this problem at 16k. We could either accept this limitation and fix up the API here to return an error here rather than None, or we could just use libc directly.

[stevendanna]

I think we should probably keep this open for a bit. Nix still has an upper bound on the buffer size.

[davidMcneil]

Looks like nix version 0.18.0 includes the fix. #7875 updates the dependencies.

[stevendanna]

@davidMcneil I _believe_ we were already pulling in this fix via git. Note that even with the fix the library still uses a fixed size buffer, it is just much bigger now.

I think if we want to fully close this we need to either:

1) Document the limitation in group size
2) Implement a feature in nix that allows us to pass larger buffer sizes if necessary.

[davidMcneil]

I believe we were already pulling in this fix via git.

Yes, we were. I was under the wrong impression that this issue was left open to track when the fix made it to a nix stable release. I should have read more closely, thanks!

[davidMcneil]

@stevendanna Here is an attempt at a true fix #7879. Do you think improving the API to return an Err instead of None in the case of the buffer being undersized is sufficient?

[stevendanna]

@davidMcneil Looks great, thanks again!