Skip to content

Commit

Permalink
switch getpwuid -> getpwuid_r
Browse files Browse the repository at this point in the history
I've always been uncomfortable with the use of getpwuid
and errno. It feels gross to directly mutate a global
variable even though that is what the man page for
getpwuid says you are supposed to do. I think the old
code was ok in terms of errno (though I was forgetting to check
for NULL return values) because errno is thread-local, but I still
don't like it. More worrying is that the man page says
"The return value may point to a static area, and may be
overwritten by subsequent calls to getpwent(3), getpwnam(), or getpwuid()."
This could be an issue because user::info() is called within
spawn_subshell, which itself is called right at the start of a new
connection. Each connection gets its own dedicated thread, so there
could potentialy be a race between two calls to user::info().
In most cases this won't be an issue since they always use the
same argument (uid for the current user), but there is the potential
for read/write shear on some of the `char*`s in the passwd struct.
I doubt this would cause issues in practice since the kernel probably
just re-uses the same pointers for a given user, but it could be
a problem in theory, and would be if the kernel builds these strings
at all dynamically.

Using the API the plunks the memory for the returned passwd right
on the stack is much safer, and does not require gross looking
global accesses.
  • Loading branch information
ethanpailes committed Mar 13, 2024
1 parent a8b3754 commit c224d83
Showing 1 changed file with 34 additions and 13 deletions.
47 changes: 34 additions & 13 deletions libshpool/src/user.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use std::ffi::CStr;
use std::{ffi::CStr, io, ptr};

use anyhow::anyhow;

Expand All @@ -24,26 +24,47 @@ pub struct Info {
}

pub fn info() -> anyhow::Result<Info> {
// Safety: we immediately copy the data into an owned buffer and don't
// use it subsequently.
let mut passwd_str_buf: [libc::c_char; 1024 * 4] = [0; 1024 * 4];
let mut passwd = libc::passwd {
pw_name: ptr::null_mut(),
pw_passwd: ptr::null_mut(),
pw_uid: 0,
pw_gid: 0,
pw_gecos: ptr::null_mut(),
pw_dir: ptr::null_mut(),
pw_shell: ptr::null_mut(),
};
let mut passwd_res_ptr: *mut libc::passwd = ptr::null_mut();
unsafe {
*libc::__errno_location() = 0;
let passwd = libc::getpwuid(libc::getuid());
let errno = nix::errno::errno();
if errno != 0 {
return Err(anyhow!("error getting passwd: {:?}", nix::errno::from_i32(errno)));
// Safety: pretty much pure ffi, passwd and passwd_str_buf correctly
// have memory backing them.
let errno = libc::getpwuid_r(
libc::getuid(),
&mut passwd,
passwd_str_buf.as_mut_ptr(),
passwd_str_buf.len(),
&mut passwd_res_ptr as *mut *mut libc::passwd,
);
if passwd_res_ptr.is_null() {
if errno == 0 {
return Err(anyhow!("could not find current user, should be impossible"));
} else {
return Err(anyhow!(
"error resolving user info: {}",
io::Error::from_raw_os_error(errno)
));
}
}

// Safety: these pointers are all cstrings
Ok(Info {
default_shell: String::from(String::from_utf8_lossy(
CStr::from_ptr((*passwd).pw_shell).to_bytes(),
CStr::from_ptr(passwd.pw_shell).to_bytes(),
)),
home_dir: String::from(String::from_utf8_lossy(
CStr::from_ptr((*passwd).pw_dir).to_bytes(),
)),
user: String::from(String::from_utf8_lossy(
CStr::from_ptr((*passwd).pw_name).to_bytes(),
CStr::from_ptr(passwd.pw_dir).to_bytes(),
)),
user: String::from(String::from_utf8_lossy(CStr::from_ptr(passwd.pw_name).to_bytes())),
})
}
}

0 comments on commit c224d83

Please sign in to comment.