This review is from Crev, a distributed system for code reviews. To add your review, set up cargo-crev.

The current version of Openat is 0.1.21.

0.1.18 (older version) Rating: Positive Thoroughness: Medium Understanding: Medium

by gitlab.com/phgsng on 2019-09-30

Disclaimer: as far evaluating syscall usage is concerned, this review considers only the behavior on Linux.

Pros

  • Self-contained, bloat free.
  • Only one unsafe public API (trait FromRawFD).
  • Wrappers don't rely on complicated contracts (like manually calling free); if necessary, cleanup takes place in drop().

Cons

  • Still considered beta according to the readme.
  • No API to pass custom flags to syscalls. Issue is acknowledged by upstream (#18, #20).
  • 23 uses of unsafe (one in test), but that is unavoidable FFI.

dir.rs

Unsafe

    • Dir::_open() calling libc::open(): return check ok; pointer arg obtained from safe Rust type.
  • +1 Dir::_sub_dir() calling libc::openat(): return check ok; pointer arg obtained from safe Rust type.
    • Dir::_read_link() calling libc::readlinkat(): return check ok; pointer arg from safe, zeroed Vec; size passed properly; result then resized to return value..
    • Dir::new_unnamed_file() calling CStr::from_bytes_with_nul_unchecked(): argument is static constant and null terminated.
    • Dir::_open_file() calling libc::openat(): return check ok; pointer arg from safe Rust type.
    • Dir::_open_file() calling File::from_raw_fd(): arg was obtained via ok syscall immediately above.
    • Dir::_symlink() calling libc::symlinkat(): return check ok; pointer args obtained from safe Rust types.
    • Dir::_create_dir() calling libc::mkdirat(): return check ok; pointer arg obtained from safe Rust type.
    • Dir::_unlink() calling libc::unlinkat(): return check ok; pointer args are sane.
    • Dir::_stat() calling mem::zeroed(): used on stack allocated struct type.
    • Dir::_stat() calling libc::fstatat(): return check ok; pointer arg path obtained from safe Rust type; struct stat obtained from zeroed buffer.
    • _rename() calling libc::renameat(): return check ok; pointer args from safe Rust types.
    • _hardlink() calling libc::linkat(): return check ok; pointer args from safe Rust types.
    • _rename_flags() calling libc::syscall() for renameat(2): return check ok; pointer args from safe Rust types; (non-impl'd syscall wrapper, related to libc issue #1508).
    • impl FromRawFd for Dir {}: unsafe API.
    • impl Drop for Dir {} calling libc::close(): no checks for result, which is ok in dtor that must not fail. Checks for libc::AT_FDCWD which is used occasionally in arguments to internal APIs.
  • ± Test uses unsafe API.

Other gotchas

  • Symlink traversal:
      • O_NOFOLLOW in calls to openat(2), fstatat(2).
      • No constructor function accepts flags, so not possible to enforce selectively.
  • ± Use of O_TMPFILE in Dir::new_unnamed_file(): ok-ish and issues documented.
  • ± Some unsafe blocks extend over non-unsafe calls e. g. to last_os_error().
  • ± Casting libc::mode_t to libc::c_uint for calls to openat(); apparently necessary on Freebsd; the rationale should be documented (see #21).
  • ± Dir::symlink() reverses order of argument of the syscall. This is unexpected but documented.

list.rs

Unsafe

  • ± Platform specific errno accessors.
    • DirIter::next_entry(): unsafe due to writes to errno and general MT unsafety of wrapped call to readdir(3); ok due to errno residing in TLS. Result pointer is wrapped in option type, cannot point to an invalid object, not shared across threads (DirIter is neither Send nor Sync), dropped properly, and not exposed publicly.
    • impl Iterator for DirIter {}: calls unsafe next_entry() (see above); calls unsafe CStr::from_ptr()onconst charpointer obtained earlier by call toreaddir(3)`` which guarantees null termination.
  • impl Drop for DirIter {} calling libc::closedir(): is only reached for valid objects.

name.rs

    • No unsafe.
    • Implements a trait AsPath for converting various types to something useable with C APIs that take paths (CStr, CString``). Lifetime bounds ensure this can be used efficiently and safely. No fishy casts.

filetype.rs

    • No unsafe.
    • Trivially safe, if metadata.rs is.

metadata.rs

    • No unsafe.
    • Owns the struct stat, so no issue here with lifetimes.

Lib.rs has been able to verify that all files in the crate's tarball, except Cargo.lock, are in the crate's repository with a git tag matching the version. Please note that this check is still in beta, and absence of this confirmation does not mean that the files don't match.

Crates in the crates.io registry are tarball snapshots uploaded by crates' publishers. The registry is not using crates' git repositories, so there is a possibility that published crates have a misleading repository URL, or contain different code from the code in the repository.

To review the actual code of the crate, it's best to use cargo crev open openat. Alternatively, you can download the tarball of openat v0.1.21 or view the source online.