Soundness: FFI calling convention mismatch (ABI mismatch) in get_from_cstr

[Manishearth]

Note

This finding was identified during an agentic unsafe Rust code review performed by Gemini AI, followed by human review and verification.

The Issue

In android_system_properties v0.1, the safe public API AndroidSystemProperties::get and AndroidSystemProperties::get_from_cstr are used to retrieve system properties on Android. However, their implementation contains a soundness vulnerability due to a calling convention (ABI) mismatch in the FFI callback definition.

Specifically, the callback function property_callback and its type alias Callback are declared with the default Rust calling convention (the "Rust" ABI):

unsafe fn property_callback(
    payload: *mut String,
    _name: *const c_char,
    value: *const c_char,
    _serial: u32,
) { ... }

type Callback = unsafe fn(*mut String, *const c_char, *const c_char, u32);

However, this callback is passed directly to the Android libc function __system_property_read_callback, which expects a callback function using the "C" calling convention (extern "C").

Calling a "Rust" ABI function pointer using the "C" calling convention (or vice versa) is UB in Rust.

(FFI doesn't work with miri, but if you patch the code enough to make it run under miri with placeholder libc functions, it does indeed fail miri)

Suggested Fix

Explicitly declare both property_callback and the Callback type alias as extern "C" (or extern "C" fn(...)):

unsafe extern "C" fn property_callback(
    payload: *mut String,
    _name: *const c_char,
    value: *const c_char,
    _serial: u32,
) { ... }

type Callback = unsafe extern "C" fn(*mut String, *const c_char, *const c_char, u32);

Additionally, fix the signature of SystemPropertyReadCallbackFn to return () instead of *const c_void, to match the Android libc signature of __system_property_read_callback:

type SystemPropertyReadCallbackFn =
    unsafe extern "C" fn(*const c_void, Callback, *mut String);

Finally, to prevent panics from unwinding across the FFI boundary (which is Undefined Behavior), handle potential UTF-8 conversion errors gracefully in the callback (e.g., using to_string_lossy() or checking the result of to_str()):

unsafe extern "C" fn property_callback(
    payload: *mut String,
    _name: *const c_char,
    value: *const c_char,
    _serial: u32,
) {
    let cvalue = CStr::from_ptr(value);
    if let Ok(s) = cvalue.to_str() {
        (*payload) = s.to_string();
    }
}

---

Note

The full audit report below also contains additional minor findings (such as missing safety comments or undocumented FFI assumptions) that are probably worth fixing as well but not the primary goal of this issue. The audit report has not been human-reviewed, it may contain misleading claims.

Full Gemini Codebase Audit Report Appendix

Unsafe Rust Review: android_system_properties (v0_1)

Overall Safety Assessment

The crate provides a thin wrapper around Android's system properties libc APIs (__system_property_read_callback, __system_property_find, and __system_property_get). While the safe API surface is small and simple, the implementation contains critical unsoundness issues, including FFI calling convention (ABI) mismatches, return type mismatches, and potential panic unwinding across FFI boundaries. There is also a complete lack of # Safety documentation or // SAFETY: comments explaining the soundness of any unsafe operation.

Because of the critical FFI ABI mismatches, this crate is classified as Critical.

Critical Findings

1. Mismatched Calling Convention (ABI Mismatch) for Callback 🔴 🤦

• Severity: 🔴 High
• Threat Vector: 🤦 Accidental Misuse
• Bug Type: ABI Mismatch

• Location: src/lib.rs:47-57

• Description:
  The callback function property_callback and its type alias Callback are declared without an explicit calling convention:

  unsafe fn property_callback(
      payload: *mut String,
      _name: *const c_char,
      value: *const c_char,
      _serial: u32,
  ) { ... }
  
  type Callback = unsafe fn(*mut String, *const c_char, *const c_char, u32);

  In Rust, this defaults to the "Rust" ABI. However, this callback is passed to __system_property_read_callback (which is a C function in Android's libc.so). The C library expects a callback function using the "C" ABI.

  Calling a "Rust" ABI function pointer using the "C" calling convention (or vice versa) is Undefined Behavior due to mismatched calling conventions, which can result in stack corruption, register corruption, or crashes.

• Remedy:
  Explicitly declare both property_callback and the Callback type alias as extern "C":

  unsafe extern "C" fn property_callback(
      payload: *mut String,
      _name: *const c_char,
      value: *const c_char,
      _serial: u32,
  ) { ... }
  
  type Callback = unsafe extern "C" fn(*mut String, *const c_char, *const c_char, u32);

2. Mismatched Return Type in FFI Function Pointer 🔴 🤦

• Severity: 🔴 High
• Threat Vector: 🤦 Accidental Misuse
• Bug Type: FFI Signature Mismatch

• Location: src/lib.rs:61-62

• Description:
  The SystemPropertyReadCallbackFn type is defined as:

  type SystemPropertyReadCallbackFn =
      unsafe extern "C" fn(*const c_void, Callback, *mut String) -> *const c_void;

  However, the signature of __system_property_read_callback in Android's libc.so is:

  void __system_property_read_callback(
      const prop_info* pi,
      void (*callback)(void* cookie, const char* name, const char* value, uint32_t serial),
      void* cookie
  );

  The C function returns void (i.e. ()), but the Rust definition declares it as returning *const c_void. Declaring a function pointer with an incorrect return type and invoking it is Undefined Behavior in LLVM.

• Remedy:
  Change the return type to () (or omit it):

  type SystemPropertyReadCallbackFn =
      unsafe extern "C" fn(*const c_void, Callback, *mut String);

3. Panic Unwinding Across FFI Boundary 🔴 🚨

• Severity: 🔴 High
• Threat Vector: 🚨 Untrusted Input
• Bug Type: FFI Panic Unwinding

• Location: src/lib.rs:53-54

• Description:
  Within property_callback, the value is converted from a C string to a Rust string:

  let cvalue = CStr::from_ptr(value);
  (*payload) = cvalue.to_str().unwrap().to_string();

  If the property value is not valid UTF-8, to_str() will return an error, and unwrap() will panic. Because property_callback is invoked by foreign (C) code, this panic will attempt to unwind across the FFI boundary, which is Undefined Behavior in Rust.

• Remedy:
  Avoid panicking in the callback. You can use to_string_lossy() or handle the error without panicking, or catch the panic using std::panic::catch_unwind. For example:

  let cvalue = CStr::from_ptr(value);
  if let Ok(s) = cvalue.to_str() {
      (*payload) = s.to_string();
  }

Fishy Findings

None.

Missing Safety Comments

1. Send and Sync implementations 🟡 🤦

• Severity: 🟡 Low
• Threat Vector: 🤦 Accidental Misuse
• Bug Type: Missing Safety Comment

• Location: src/lib.rs:85-86

• Proposed Safety Comment:

  // SAFETY:
  // - `libc_so` is a raw library handle from `dlopen`. Once loaded, it is thread-safe to
  //   use and close via `dlclose`.
  // - The loaded function pointers (`get_fn`, `find_fn`, `read_callback_fn`) are read-only
  //   and point to thread-safe Android system property APIs in libc.
  // - Since `AndroidSystemProperties` is not `Clone`, the internal raw pointers are safely
  //   encapsulated and can only be dropped when the struct itself is dropped.
  unsafe impl Send for AndroidSystemProperties {}
  unsafe impl Sync for AndroidSystemProperties {}

2. Calling dlopen and dlsym in new 🟡 🤦

• Severity: 🟡 Low
• Threat Vector: 🤦 Accidental Misuse
• Bug Type: Missing Safety Comment

• Location: src/lib.rs:103, src/lib.rs:117, src/lib.rs:126-138

• Proposed Safety Comments:
  For dlopen:

  // SAFETY:
  // - The library name `libc.so\0` is a valid null-terminated C string.
  // - `RTLD_NOLOAD` is safe because it only queries the handle of an already loaded library
  //   without loading new code or causing side effects.
  let libc_so = unsafe { libc::dlopen(b"libc.so\0".as_ptr().cast(), libc::RTLD_NOLOAD) };

  For load_fn:

  // SAFETY:
  // - `libc_so` is a valid, non-null library handle.
  // - `name` is a null-terminated byte slice representing a valid C-style string.
  let fn_ptr = libc::dlsym(libc_so, name.as_ptr().cast());

  For the transmutes block:

  // SAFETY:
  // - The loaded function pointers from `libc.so` are transmuted to their matching
  //   function pointer signatures (`SystemPropertyReadCallbackFn`, `SystemPropertyFindFn`,
  //   and `SystemPropertyGetFn`).
  // - The signatures match the declarations in Android's `<sys/system_properties.h>`.
  unsafe {
      ...
  }

3. Calling dynamic functions in get_from_cstr 🟡 🤦

• Severity: 🟡 Low
• Threat Vector: 🤦 Accidental Misuse
• Bug Type: Missing Safety Comment

• Location: src/lib.rs:182, src/lib.rs:191, src/lib.rs:205

• Proposed Safety Comments:
  For find_fn:

  // SAFETY:
  // - `find_fn` is a valid function pointer loaded from `libc.so`.
  // - `cname` is a valid, null-terminated `CStr`, so its pointer is valid for the call.
  let info = unsafe { (find_fn)(cname.as_ptr()) };

  For read_callback_fn:

  // SAFETY:
  // - `read_callback_fn` is a valid function pointer loaded from `libc.so`.
  // - `info` is a valid non-null pointer returned by `find_fn`.
  // - `property_callback` is a valid callback matching the expected ABI.
  // - `&mut result` is a valid mutable reference to a `String`, which stays alive for
  //   the duration of this synchronous call.
  unsafe {
      (read_callback_fn)(info, property_callback, &mut result);
  }

  For get_fn:

  // SAFETY:
  // - `get_fn` is a valid function pointer loaded from `libc.so`.
  // - `cname` is a valid, null-terminated `CStr`.
  // - `raw` points to a buffer with capacity `PROPERTY_VALUE_MAX` (92 bytes).
  // - Android's `__system_property_get` is documented to write at most `PROP_VALUE_MAX`
  //   (92) bytes into the buffer (including the null terminator), so this call will not
  //   write out of bounds.
  let len = unsafe { (get_fn)(cname.as_ptr(), raw) };

4. Updating vector length via set_len in get_from_cstr 🟡 🤦

• Severity: 🟡 Low
• Threat Vector: 🤦 Accidental Misuse
• Bug Type: Missing Safety Comment

• Location: src/lib.rs:209-211

• Proposed Safety Comment:

  // SAFETY:
  // - `len` is the number of bytes written by `__system_property_get` (excluding the null
  //   terminator).
  // - `len as usize <= buffer.capacity()` is checked by the assert.
  // - The first `len` bytes were successfully written/initialized by `__system_property_get`.
  unsafe {
      buffer.set_len(len as usize);
  }

5. dlclose call in Drop 🟡 🤦

• Severity: 🟡 Low
• Threat Vector: 🤦 Accidental Misuse
• Bug Type: Missing Safety Comment

• Location: src/lib.rs:225-227

• Proposed Safety Comment:

  // SAFETY:
  // - `self.libc_so` is a valid library handle from `dlopen`.
  // - Since the struct is being dropped, no external references to the loaded functions
  //   exist, so unloading the library will not leave any dangling function pointers.
  unsafe {
      libc::dlclose(self.libc_so);
  }

[MarijnS95]

Keep in mind that this crate also hasn't seen any updates in close to 4 years, despite still being downloaded over half a million times each working day (https://crates.io/crates/android_system_properties)...

I've been working towards adding similar bindings to the canonical NDK abstraction crate in rust-mobile/ndk#495, to keep it all in one coherent place. Although feel disappointed about not getting a new release out in over two years too 🥹