logo

Rust crates reviews

Cryptographically verifiable, distributed dependency reviews

reviewer: phgsng

https://lib.rs/phgsng

$ cargo crev repo fetch url https://gitlab.com/phgsng/crev-proofs
$ cargo crev id trust pt_he2sLPg2w2u4YN7lj-6Gvu25R8aN6ZCcuQFzxC1g

repo: https://gitlab.com/phgsng/crev-proofs

crate version
rating
date
reviewer
thoroughness, understanding
positive
2019-10-10
phgsng
low, medium

No “unsafe” code. Trivial functionality. Basic functionality tests.

positive
2022-06-30
phgsng
low, medium

Safety

unsafe limited to interaction with the Netlink socket.

Uses of unsafe

  • All uses of unsafe are warranted and blocks span the
    minimum necessary amount of code.

  • unsafe blocks wrap syscalls on the socket descriptor
    (fcntl, socket, setsockopt, getsockopt,
    send, recv, close). Arguments in general don’t
    depend on dynamically sized objects, and where they do, it is
    safe buffer types whose lengths are handled correctly.

    • line 65: socket(), args: only integer values, return
      ok.

    • line 82: fcntl() (two calls), args: integer values,
      return ok.

    • line 96: fcntl() (two calls), args: integer values,
      return ok.

    • line 110: fcntl(), args: integer values, return ok.

    • line 121: mem::zeroed(), ok because a sockaddr_nl
      can be safely zero initialized.

    • line 125: bind(), args: integer and pointer values,
      correct size supplied for struct sockaddr pointer arg,
      return ok.

    • line 144: setsockopt(), args: integer values, arg size
      ok, return ok.

    • line 163: setsockopt(), args: integer values, arg size
      ok, return ok.

    • line 183, 197: getsockopt(), args: integer and pointer
      values, out-pointer arg is zero initialized Vec, arg
      size ok, return ok.

    • line 220: send(), args: integer and pointer values, arg
      size ok, return ok.

    • line 238: recv(), args: integer and pointer values, arg
      size ok, return ok.

    • line 282: close(), arg: integer value, return discarded
      but is harmless in Drop impl.

    • line 851: NlSocket::from_raw_fd(), arg: dummy value,
      ok for unit test.

  • unsafe fn in trait impls for FromRawFd. → Nothing
    actually unsafe going on in there; wrapped assigment is plain
    Copy data only except for heap allocation via safe
    interface.

Pros

  • Comprehensive wrapper for talking Netlink and various dialects
    thereof (rtnetlink, generic netlink, …) to the Kernel.

  • Active development.

  • Typed APIs allow for intuitive use of Netlink APIs that is
    superior to the everything-is-an-int C analogues (libnl*,
    libmnl),

  • Provides macros for defining idiomatic wrappers of other
    Netlink based interfaces.

Cons

  • v0.x versioned, frequent API breakage.

  • Depends itself on various zerover crates.

positive
2019-09-30
phgsng
medium, medium

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.
strong
2019-09-23
phgsng
medium, medium

No build.rs. No use of unsafe code or calls into C libraries. String accesses using bounds checked APIs. No IO.

positive
2019-11-20
phgsng
medium, medium

errors.rs

  • Definitions used with the error_chain crate; no unsafe.

facility.rs

  • Defines a type for Syslog facilties and the associated
    constants. No unsafe.

format.rs

  • Message formatting incl. structured data. No unsafe.
  • Formatting looks conformant to RFC 5424.

lib.rs

  • Initialization code, socket stuff, and user facing wrappers.
  • One unsafe call to getpid(2); harmless.
positive
2019-10-04
phgsng
medium, medium

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

Cons

  • “unsafe”, “unsafe” everywhere; 32× as of version 0.5.0. Not
    really avoidable when working closely with C APIs though.
  • C APIs can still be handled incorrectly in the public API, e.
    g. calling shutdown() twice on one socket. The result is
    handled safely though even under programmer errors.
  • As of 2019 the code is a bit stale, e. g. it uses the soon to
    be deprecatedtry!() and thus may not build with a future
    version of Rust.

Pros

  • Excellent API documentation.
  • Minimal dependencies (libc, cfg-if).
  • Use of unsafe is never gratuitous.
  • Public API exposes safe interfaces that ensure correctness on
    the type level.
  • Thoroughly unit-tested (all tests ok).

lib.rs

general considerations

    • Unix syscalls are handled using two wrappers that convert
      return values to io::Result.
    • Most uses of unsafe use safe lower level constructs.

unsafe code

There is only one source file, lib.rs, so all uses of
“unsafe” are found there.

    • fn sun_path_offset(): calculates the offset of a struct
      member using pointer arithmetic. Circumnavigates possible UB
      (cf. https://internals.rust-lang.org/t/9273/127).
      offsetof() is still an unsolved problem in Rust.
    • impl Drop for Inner: obligatory dtor.
    • fn Inner::new(): wraps socket(2); return value
      handled ok.
    • fn Inner::new_pair(): wraps socketpair(2); return
      value handled ok; out parameter handled correctly.
    • fn Inter::try_clone(): wraps dup(2); return value
      handled ok.
    • fn Inner::shutdown(): wraps shutdown(2); return value
      handled ok; API safe by accepting an enum wrapper over the
      int how argument.
    • fn Inner::timeout(): wraps getsockopt(2); return
      value handled ok; out parameter (struct timeval) handled
      ok.
    • fn Inner::set_timeout(): wraps setsockopt(2); return
      value handled ok; correct input validation, catching out of
      range values for time_t argument using saturating
      arithmetic.
  • ± fn Inner::set_nonblocking(): wraps ioctl(2) with
    FIONBIO; return value handled ok. Why not use fcntl(2)
    with O_NONBLOCK instead? Probably to save one syscall? This
    warrants an explantory comment.

    • fn Inner::take_error(): wraps getsockopt(2); return
      value handled ok; out parameter ok.
    • unsafe fn sockaddr_un(): initializes a struct sockaddr_un; inputs validated appropriately; raw pointer
      access provably within bounds.
    • fn SocketAddr::new(): obtains a socket address; for use
      with getpeername() / getsockname() etc.; return value
      handled ok (assuming the passed function returns the syscall
      return value).
    • fn SocketAddr::address(): casts memory of a wrapped type
      from [char] to [u8]; array bounds are preserved thus
      subsequent accesses return slices with valid bounds.
    • fn UnixStream::connect()`: public API wrapping socket creation and connect(2)``; arguments obtained from internal
      APIs deemed safe; return value handled ok.
    • fn UnixStream::local_addr()`: wraps getsockname(2); return value and arguments handled by SocketAddr::new()``.
    • fn UnixStream::peer_addr()`: wraps getpeername(2); return value and arguments handled by SocketAddr::new()``.
    • impl Read for UnixStream, fn read(): wraps recv(2);
      return value handled ok; argument values obtained from safe
      rust type.
    • impl Write for UnixStream, fn write(): wraps send(2);
      return value handled ok; argument values obtained from safe
      rust type.
    • impl FromRawFd for UnixStream, fn from_raw_fd(): unsafe
      trait; constructs a UnixStream without validating the
      arugment.
  • ± fn UnixListener::bind(): wraps socket creation,
    bind(2) and listen(2; return values handled ok; backlog
    of socket queue hard-coded to the default Linux maximum of 128
    which is reasonable but deserves mention in the docs.

    • fn UnixListener::accept(): wrapper for accept(2);
      return value and argument checks deferred to
      SocketAddr::new().
    • fn UnixListener::local_addr(): wrapper for
      getsockname(2); return value and arguments handled by
      SocketAddr::new().
    • impl FromRawFd for UnixListener, fn from_raw_fd(): unsafe
      trait, constructs UnixListener without validating anything.
    • fn UnixDatagram::bind(): wraps bind(2) for
      SOCK_DGRAM type sockets; return value handled ok; args
      obtained from safe wrappers.
    • fn UnixDatagram::connect(): wraps connect(2); return
      value handled ok; args obtained from safe wrappers.
    • fn UnixDatagram::local_addr()`: wraps getsockname(2); return value and arguments handled by SocketAddr::new()``.
    • fn UnixDatagram::peer_addr()`: wraps getpeername(2); return value and arguments handled by SocketAddr::new()``.
    • fn UnixDatagram::recv_from()`: wraps recvfrom(2)``;
      return value handled ok; args obtained from safe wrappers.
    • fn UnixDatagram::recv()`: wraps recv(2)``; args obtained
      from safe types; return value handled ok.
    • fn UnixDatagram::send_to()`: wraps send_to(2)``; args
      obtained from safe types; return value handled ok.
    • fn UnixDatagram::send()`: wraps send_to(2)``; args
      obtained from safe types; return value handled ok.
    • impl FromRawFd for UnixDatagram, fn from_raw_fd(): unsafe
      trait, constructs UnixDatagram without validating anything.
positive
2022-06-29
phgsng
low, medium
positive
2022-06-29
phgsng
medium, medium

Safety

Zero uses of unsafe by virtue of the safe APIs provided by
neli.

Pros

  • Safe APIs for the Wireguard Netlink interface, does eliminate
    the need for unsafe primitives talking to the kernel.

  • Exposes a foolproof API wrapping more low-level components like
    Netlink sockets.

  • Under active development.

Cons

  • Currently (v2.0.5) dependent on an ancient version of the
    neli library.

  • Depends on numerous v0.x versioned crates (neli, libc).

positive
2022-06-29
phgsng
medium, medium

Safety

Zero uses of unsafe by virtue of the safe APIs provided by
neli.

Pros

  • Safe APIs for the Wireguard Netlink interface, does eliminate
    the need for unsafe primitives talking to the kernel.

  • Exposes a foolproof API wrapping more low-level components like
    Netlink sockets.

  • Under active development.

Cons

  • Currently (v2.0.5) dependent on an ancient version of the
    neli library.

  • Depends on numerous v0.x versioned crates (neli, libc).

© bestia.dev 2023, MIT License, Version: 2023.608.1636

Open source repository for this web app: https://github.com/bestia-dev/cargo_crev_web/