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

Possible undefined behavior in out parameter handling #245

Closed
jsha opened this issue Dec 8, 2021 · 2 comments · Fixed by #256
Closed

Possible undefined behavior in out parameter handling #245

jsha opened this issue Dec 8, 2021 · 2 comments · Fixed by #256

Comments

@jsha
Copy link
Collaborator

jsha commented Dec 8, 2021

A lot of our functions take out parameters, e.g.:

rustls_result rustls_connection_read(struct rustls_connection *conn,
                                     uint8_t *buf,
                                     size_t count,
                                     size_t *out_n);

The implementation is usually like:

            let out_n: &mut size_t = try_mut_from_ptr!(out_n);
            ...
            *out_n = n_read;

I think this creates undefined behavior if the caller passes a pointer to an uninitialized size_t, since we're then creating a reference to uninitialized memory. It would be better to keep out_n as a raw pointer, do the null check at the top of the function, and then dereference it in an unsafe block at the end of the function.

Another possibility would be to define an OutParam wrapper type, something like:

#[repr(transparent)]
struct OutParam<T> {
  inner: NonNull<MaybeUninit<T>>
}

impl<T> OutParam<T> {
  fn from_ptr(p: *mut T) -> Option<Self> {...}
  fn write(T) { ... }
}

That would allow us to use the type system to ensure we've checked for null at the top. And would also allow us, on the Rust side, to clearly mark out parameters (though they are already marked rather clearly by *mut and occurring at the end of a function signature).

@jsha
Copy link
Collaborator Author

jsha commented Apr 13, 2022

I started work on this in #256. I concluded that defining an OutParam type added complexity without corresponding benefit. In particular, fn write would need to be unsafe, so we'd need an unsafe block at the end of the function anyhow.

@jsha
Copy link
Collaborator Author

jsha commented Apr 18, 2022

Related links:

https://lucumr.pocoo.org/2022/1/30/unsafe-rust/
rust-lang/miri#1240
https://github.com/CAD97/pointer-utils/pull/45/files

In particular those last two made me aware of ptr::slice_from_raw_parts (vs slice::from_raw_parts):

Forms a raw slice from a pointer and a length.

This function is safe, but actually using the return value is unsafe. See the documentation of slice::from_raw_parts for slice safety requirements.

@jsha jsha closed this as completed in #256 Apr 19, 2022
jsha added a commit that referenced this issue Apr 19, 2022
Creating a reference to uninitialized memory is undefined
behavior. Callers of this library are likely to pass pointers to
uninitialized memory as out params. We should support that by not turning
out params into references.

Instead, keep them as raw pointers and use an unsafe block where we
actually write the param.

Fixes #245
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

Successfully merging a pull request may close this issue.

1 participant