Am I safely handling this C FFI pointer

help
1

I would like to use setuid(2) to drop root permissions. I'd like to do this via a username which means I need to first call getpwnam(3). Anyway, is the below code safe and does it avoid a memory leak?

libc links:

extern crate alloc;
use alloc::ffi::{CString, NulError};
use core::ffi::c_void;
pub fn get_uid_from_pwnam(name: String) -> Result<Option<u32>, NulError> {
    CString::new(name.into_bytes()).map(|n| {
        let ptr = n.as_ptr();
        // SAFETY:
        // `getpwnam` is an FFI binding; thus requires unsafe code.
        let entry = unsafe { libc::getpwnam(ptr) };
        if entry.is_null() {
            None
        } else {
            // SAFETY:
            // `entry` is not null, so it's safe to dereference.
            let pwd_ent = unsafe { *entry };
            let uid = pwd_ent.pw_uid;
            let void_ptr = entry.cast::<c_void>();
            // SAFETY:
            // `entry` is not null, so `void_ptr` isn't either.
            // This means it's safe to free it.
            unsafe { libc::free(void_ptr) }
            Some(uid)
        }
    })
}

Edit

Do I need to be sure to drop pwd_ent before freeing void_ptr? For example:

            let uid = {
                // SAFETY:
                // `entry` is not null, so it's safe to dereference.
                let pwd_ent = unsafe { *entry };
                pwd_ent.pw_uid
            };
            let void_ptr = entry.cast::<c_void>();
            // SAFETY:
            // `entry` is not null, so `void_ptr` isn't either.
            // This means it's safe to free it.
            unsafe { libc::free(void_ptr) }
2

if all you need is the uid field, making a copy of the entire entry is not ideal in terms of efficiency, but it does no harm either. you can get the uid without copying the entire entry:

let uid = unsafe { (*entry).pw_uid };

the check for null ptr and access the uid field can be implemented more concisely with ptr::as_ref() and Option::map() like this:

fn name_to_uid(name: &CStr) -> Option<libc::uid_t> {
	// SAFETY:
	// CStr guarantees the pointer is NUL terminated
	// returned pointer is either valid or null
	unsafe { libc::getpwnam(name.as_ptr()).as_ref().map(|e| e.pw_uid) }
}

this is wrong, you MUT NOT free the pointer returned by getpwnam(), the pointer is not heap allocated but (probably) points to static memory.

2 Likes
3

getpwnam(3) clearly states:

Do not pass the returned pointer to free(3)

Thanks for the save. Oops.

4

@nerditation, apologize for necromancing this; but I ended up making a few changes that I'd like reviewed. I actually want to also call setgid(2); thus I need to capture both the uid and gid from libc::passwd. Additionally, I want to distinguish between an error and a missing entry in passwd(5).

  1. Do you happen to know if pattern matching on the struct fields is more efficient?
// SAFETY:
// `entry` is not null, so it's safe to dereference.
let libc::passwd { pw_uid, pw_gid, .. } = unsafe { *entry };
  1. I couldn't find any information about freeing the pointer returned from __errno_location. I'm guessing I'm not supposed to do that either? According to getpwnam(3), "if one wants to check errno after the call, it should be set to zero before the call"; thus I can't simply rely on std::io::Error::last_os_error.
CString::new(name.into_bytes())
        .map_err(GetpwnamErr::Nul)
        .and_then(|n| {
            let ptr = n.as_ptr();
            // We set `errno` to 0 that way we can distinguish a missing entry
            // from an error after calling `getpwnam`.
            // SAFETY:
            // `__errno_location` is an FFI binding; thus requires unsafe code.
            let err_ptr = unsafe { libc::__errno_location() };
            if err_ptr.is_null() {
                Err(GetpwnamErr::ErrnoNull)
            } else {
                // SAFETY:
                // `err_ptr` is not null; thus it's safe to dereference.
                unsafe {
                    *err_ptr = 0;
                }
                // SAFETY:
                // `getpwnam` is an FFI binding; thus requires unsafe code.
                // `ptr` is not null.
                let entry = unsafe { libc::getpwnam(ptr) };
                if entry.is_null() {
                    // We already checked this above, but better err on the
                    // side of caution and verify it's still not null.
                    if err_ptr.is_null() {
                        Err(GetpwnamErr::ErrnoNull)
                    } else {
                        // SAFETY:
                        // `err_ptr` is not null; thus it's safe to dereference.
                        let code = unsafe { *err_ptr };
                        if code == 0 {
                            Ok(None)
                        } else {
                            Err(GetpwnamErr::Io(io::Error::from_raw_os_error(code)))
                        }
                    }
                } else {
                    // SAFETY:
                    // `entry` is not null, so it's safe to dereference.
                    let libc::passwd { pw_uid, pw_gid, .. } = unsafe { *entry };
                    Ok(Some(UserInfo {
                        uid: pw_uid,
                        gid: pw_gid,
                    }))
                    // We _don't_ `libc::free` `entry` since it may—even likely—point to static memory.
                }
            }
        })
5

don't over-think about it, do whatever feels natural to you. this kind of "optimization" makes absolutely no difference in practice. I wrote ptr::as_ref() in the example because I'm very used to it and it feels familiar.

when you match on *entry, the default binding mode is "move", it is equivalent to make a copy first (the struct is Copy, like most libc types). at the end of day, I'm pretty sure it won't make any difference in terms of efficiency, it should generate the same machine code.

for one, the passwd struct is very small, even you copy the whole struct, it is not a big deal; what's more, the optimizer should be able to generate the optimal code easily, since such POD structs are perfect candidates for the "Scalar Replacement of Aggregates" pass, and then data flow analysis can just eliminates unused memory loads.

DOT NOT free it! __errno_location() returns the address of the thread local errno, it is not a heap pointer.

if you are not familiar with libc, or uncomfortable with unsafe in general, you can use safe and "higher level" system interface crates. one of the most popular ones for linux and other posix systems is nix.

for example, nix has an Errno type which is a wrapper for libc's errno, you can use Errno::last() to read errno, and Errno::set() to write to errno. there's also Errno::last_raw() and Errno::set_raw() too.

but you don't have to deal with lib::getpwnam() yourself, nix has a high level safe wrapper for that too [1]:

let user = "root";
match User::from_name(user) {
    Err(e) => {
        println!("error: {}", e);
    }
    Ok(None) => {
        println!("no entry");
    }
    Ok(Some(User { uid, gid, .. })) => {
        println!("uid: {}, gid: {}", uid, gid);
    }
}

  1. actually for the thread safe version libc::getpwnam_r(), instead of libc::getpwname() ↩︎

3 Likes