Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unsound Use of CStr::from_ptr in new Function #204

Open
lwz23 opened this issue Nov 30, 2024 · 1 comment
Open

Unsound Use of CStr::from_ptr in new Function #204

lwz23 opened this issue Nov 30, 2024 · 1 comment

Comments

@lwz23
Copy link

lwz23 commented Nov 30, 2024

Description
The new function uses unsafe { CStr::from_ptr(pointer) } to interpret a raw C string pointer (*mut i8) as a CStr. This operation assumes that the pointer meets the following safety requirements:

  1. The pointer is non-null.
  2. The pointer is properly aligned.
  3. The pointer references a null-terminated string.
  4. The memory pointed to is valid and accessible.
    If any of these conditions are violated, the program invokes undefined behavior (UB). The function does not validate the pointer argument or its contents, making it unsound.
    CStr::from_ptr(pointer).to_owned()
pub fn new(pointer: *mut i8) -> Self {
    let string = unsafe {
        CStr::from_ptr(pointer).to_owned()
    };
    Self { string }
}

Problems:
this function is a pub function, so I assume user can control the pointer field, it cause some problems.

  1. Unchecked Pointer Validity:
    The pointer parameter is assumed to be valid and non-null, but there is no validation to enforce this assumption.
    If pointer is null, accessing it with CStr::from_ptr causes undefined behavior.
  2. Null-Termination Requirement:
    CStr::from_ptr requires that the string is null-terminated. If the input pointer does not point to a null-terminated string, the function will read out of bounds, leading to undefined behavior.
  3. No Documentation for Safety Contract:
    The function does not document the safety requirements for the pointer parameter, leaving the caller unaware of the preconditions necessary to avoid UB.
  4. Invalid or Dangling Pointers:
    If pointer points to memory that is invalid, already freed, or not properly aligned, the program will exhibit undefined behavior.

Suggestion

  1. mark this function as unsafe and provide safety doc.
  2. add some check in the function body.

Additional Context:
Rust's unsafe blocks require careful handling to uphold safety guarantees. The current implementation of new assumes the validity of the pointer without enforcing this requirement, making the function unsound. By adding input validation, the function can prevent undefined behavior while maintaining its functionality.

@lwz23
Copy link
Author

lwz23 commented Dec 2, 2024

here is my PoC:

use std::ffi::{CString, CStr};

pub struct FFIString {
    string: CString
}

impl FFIString {
    pub fn new(pointer: *mut i8) -> Self {
        let string = unsafe {
            CStr::from_ptr(pointer).to_owned()
        };
        Self { string }
    }
}
use std::ptr;

fn main() {
    let invalid_pointer: *mut i8 = ptr::null_mut(); // 空指针,模拟无效输入

    let ffi_string = FFIString::new(invalid_pointer);

    println!("Created FFIString: {:?}", ffi_string.string);
}

and output:

PS E:\Github\lwz> cargo run
   Compiling lwz v0.1.0 (E:\Github\lwz)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.22s
     Running `target\debug\lwz.exe`
error: process didn't exit successfully: `target\debug\lwz.exe` (exit code: 0xc0000005, STATUS_ACCESS_VIOLATION)

So, I think I can confirm this is a unsound problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant