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

Maybe unsound in StringData #1062

Open
lwz23 opened this issue Dec 6, 2024 · 1 comment
Open

Maybe unsound in StringData #1062

lwz23 opened this issue Dec 6, 2024 · 1 comment

Comments

@lwz23
Copy link

lwz23 commented Dec 6, 2024

Hello, thank you for your contribution to this project. I'm currently scanning rust projects for unsound issues, and I notice the following code.

pub struct StringData {
    pub content: *const u8,
    pub len: u32,
}

impl StringData {
    ...................................
    pub fn to_string(&self) -> String {
        unsafe {
            let utf8 = std::slice::from_raw_parts(self.content, self.len as usize);
            String::from_utf8(utf8.to_vec()).unwrap()
        }
    }
}

I am concerned that there may be potential unsound problems here, json_interface is a pub mod, StringData is a pub struct, and content is a pub field. I take this to mean that an external user can call and modify the content field directly, and that to_string calls from_raw_parts without any checking, which I fear could lead to a potential UB (eg.null pointer), So I thought maybe it would be a more appropriate choice to

  1. mark to_string as unsafe or
  2. make these fields and functions private if there is no external use.
@lwz23
Copy link
Author

lwz23 commented Dec 6, 2024

here is my PoC and result:

use std::ptr::null;

pub struct StringData {
    pub content: *const u8,
    pub len: u32,
}

impl StringData {
    pub fn new(s: &String) -> Self {
        Self {
            content: s.as_ptr(),
            len: s.len() as u32,
        }
    }

    pub fn default() -> Self {
        Self {
            content: null(),
            len: 0,
        }
    }

    pub fn to_string(&self) -> String {
        unsafe {
            let utf8 = std::slice::from_raw_parts(self.content, self.len as usize);
            String::from_utf8(utf8.to_vec()).unwrap()
        }
    }
}
fn main() {
   let a= StringData::default();
   a.to_string();
}

result:

PS E:\Github\lwz> cargo run
   Compiling lwz v0.1.0 (E:\Github\lwz)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.52s
     Running `target\debug\lwz.exe`
thread 'main' panicked at core\src\panicking.rs:223:5:
unsafe precondition(s) violated: slice::from_raw_parts requires the pointer to be aligned and non-null, and the total size of the slice not to exceed `isize::MAX`
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread caused non-unwinding panic. aborting.
error: process didn't exit successfully: `target\debug\lwz.exe` (exit code: 0xc0000409, STATUS_STACK_BUFFER_OVERRUN)

It seems that you don't even need to manually modify the content field, just use the provided default function to trigger UB.

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