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

Hazard in the specification of std::slice::from_raw_parts, since other languages can represent empty slices with a null pointer #120243

Closed
daira opened this issue Jan 22, 2024 · 7 comments · Fixed by #120594
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues.

Comments

@daira
Copy link
Contributor

daira commented Jan 22, 2024

The documentation is clear that std::slice::from_raw_parts(std::ptr::null, 0) is UB.

This creates a significant hazard for FFI code interfacing with C++ (and very likely other languages). Although C++ doesn't have a built-in or standard-library type for a slice, the most common way of representing slices is as a start pointer and a length, or as start and end pointers. As explained in this post, start is conventionally allowed to be null (i.e. the representation of an empty slice can be (nullptr, 0) or (nullptr, nullptr)), and this is consistent with the behaviour of C++ standard library APIs such as std::span.

The stated rationale for this call being UB is:

data must be non-null and aligned even for zero-length slices. One reason for this is that enum layout optimizations may rely on references (including slices of any length) being aligned and non-null to distinguish them from other data. You can obtain a pointer that is usable as data for zero-length slices using ptr::NonNull::dangling().

This rationale is unconvincing to me. It's unnecessary for the detail of the Rust ABI's internal representation of slices to result in exposing this hazard. The full signature is

pub const unsafe fn from_raw_parts<'a, T>(data: *const T, len: usize) -> &'a [T]

Null slices are a niche-value optimization, they're not part of the domain of &'a [T]. When Rust programmers write a use of this function, they are most likely to be writing FFI code. We don't know where data and len are taken from; they could be using either the Rust or the C++ convention for an empty slice. But there is no ambiguity: neither of these representations can mean anything else than an empty slice. So, I argue that the implementation of slice::from_raw_parts should accept either, converting a null pointer into an aligned "dangling" pointer as necessary.

The fact that this involves a representation change because of differences between C++ and Rust slice conventions shouldn't matter: returning a valid Rust representation of an empty slice is the only thing that could be correct. Note that it is fine for std::slice::from_raw_parts(ptr::null, len) still to be UB when len > 0.

To correctly convert a data and len that could be using the C++ convention, with the current definition of slice::from_raw_parts you would need to do something like:

if data.is_null() {
    slice::from_raw_parts(ptr::NonNull::<T>::dangling(), 0)
} else {
    slice::from_raw_parts(data, len)
}

which is verbose and easy to miss the need for.

What about the cost of the null pointer check? Well, in many cases the compiler will be able to statically determine that data is non-null. In those cases, when it inlines slice::from_raw_parts it will optimize out the null check, and the cost will be zero. This includes both invocations in the code above. It even includes complicated cases such as this in bitvec:

/// Views the bit-vector as a slice of its underlying memory elements.
#[inline]
pub fn as_raw_slice(&self) -> &[T] {
    let (data, len) = (self.bitspan.address().to_const(), self.bitspan.elements());
    unsafe { slice::from_raw_parts(data, len) }
}

Here self.bitspan.address().to_const() returns the result of as_ptr() on a NonNull value (via this code in wyz which is #[inline(always)]), which allows the null check to be optimized out. Many of the other examples I looked at are like this.

The only cases where correct code can incur an additional cost for the null check is when the programmer knows —and is correct— that data cannot be null, but the compiler does not. And we only care about that in cases where it causes a performance regression. I would suggest that this is probably very rare, and that it is likely to be much more common that programmers take a (start, len) or (start, end) slice according to the C++ convention and just don't think of the corner case where start is null.

What about the possibility of code written for the new specification being run on an older Rust version that requires data to be non-null? That's okay as long as we document when the requirement was changed. Then:

  • If a programmer checked the previous documentation and wrote previously correct code, their code will still be correct.
  • If they check the new documentation, they will see that a null pointer wasn't previously allowed. Either their MSRV is already after the version where it changed; or they bump their MSRV; or they check for a null pointer.
  • If they wrote previously incorrect code, the code may work correctly under a later version of Rust. This is fine and what we intend. Eventually they will update the MSRV and the code will be correct.

Aside: String::from_raw_parts and Vec::from_raw_parts_in are similar, but the case for changing those is much weaker, since they impose strong constraints on the allocated region that would be unlikely to be met by a random slice coming from C++ or another language.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jan 22, 2024
@the8472
Copy link
Member

the8472 commented Jan 22, 2024

If an FFI function expects to receive nullable pointers it should be using Option<NonNull<_>>, which has guaranteed NPO so it should be abi-compatible with the plain pointer version.

@daira
Copy link
Contributor Author

daira commented Jan 22, 2024

If an FFI function expects to receive nullable pointers it should be using Option<NonNull<_>>, which has guaranteed NPO so it should be abi-compatible with the plain pointer version.

Sure, if you have the slice as (start: Option<NonNull<T>>, len: usize) then you can use

slice::from_raw_parts(start.unwrap_or_else(|| ptr::NonNull::<T>::dangling(), len)

which is slightly more elegant, I guess. But it doesn't really address the point that you need to know that this is a problem in the first place in order to see that it could be useful to do that. Unless you explicitly think about the fact that the convention in C++ is to represent empty slices using a null pointer, you're very likely to miss it, and Option<NonNull<T>> is very far from being the most obvious representation of a nullable pointer to T.

I am not saying that you can't write correct code with the API as it is; clearly you can.

@the8472
Copy link
Member

the8472 commented Jan 22, 2024

I don't understand the scenario under which this can happen.
Sure, the C++ programmer might pass a disassembled span to some underspecified FFI function that just takes a pointer and a length.
But someone still has to write the rust half of that code which does involve writing some unsafe blocks which should involve checking the safety preconditions of each function.

And the function does have debug asserts. So if one runs such incorrect code under cargo careful it would be detected.

@fmease fmease added C-discussion Category: Discussion or questions that doesn't represent real issues. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jan 23, 2024
@saethlin
Copy link
Member

I am working on a compiler change that will detect this problem via a special kind of debug assertion, without any need to use an external tool like cargo-careful. Since such a change is possible, I think it would be a grave mistake to change the API of this function so that it accepts more calls by doing a runtime check in all build configurations.

@the8472
Copy link
Member

the8472 commented Jan 23, 2024

The blog post does suggest improving the documentation (how? It's not clear to me how the current docs are ambiguous or misleading) and adding separate FFI-specific conversion helpers.

Null slices are a niche-value optimization, they're not part of the domain of &'a [T].

It's more than a niche optimization. We're telling LLVM that the pointers are non-null and dereferencable. Passing a null pointer is instant UB, not merely on-access UB.

When Rust programmers write a use of this function, they are most likely to be writing FFI code.

That's quite subjective. In std those methods are mostly in collections and iterators to convert between raw pointer representations and safe slices which always follow the Rust semantics.

@workingjubilee
Copy link
Member

Why would we assume this transformation would bite people less often than our destruction of the following equivalence?

let slice = core::slice::from_raw_parts(ptr, len);
let (ptr, len) = (slice.as_ptr(), slice.len());

@workingjubilee
Copy link
Member

I do not find David Ben's post to be convincing, because I do work with FFI in a context where making sure one gets the semantics of slices right is quite important... and the most common representation of a slice-like type does not match his preferred formats (it does not even use nullptr to indicate validity or non!). The second uses nullptr for a 0-len version because it used to be a LinkedList. Only the third and onwards do.

C programmers can be relentless in their eagerness to bitpack things, and I feel that arguing from "but there's a standard slice representation!" requires C2y to add _Slice in order to make it true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants