Handle ZST

[elichai]

Almost all zst passes to FFI is undefined behavior
(i.e. memcpy(NULL,NULL,0) is UB, same for getrandom)

This checks that the slice isn't a ZST. otherwise rust doesn't promise anything about the pointer from slice.as_mut_ptr()

I do not think this is actually a vulnerability/deserve advisory, but if someone disagrees I would report this to https://github.com/RustSec/advisory-db (@tarcieri)
because most implementations will probably be fine with it. though I had problems in the past with Intel's libraries that passing NULL+0 to memset resulted in SEGFAULT.

[newpavlov]

Firstly, and most importantly, passing an empty slice does not mean that we have a null pointer on our hands. Granted you could construct an empty zero-sized slice from a null pointer using unsafe code, but you on your own after that. Secondly, can you point us to any documentation that forbids passing null pointers with a zero length on our supported targets? In the worst case scenario I would expect OS to throw some kind of error (like EFAULT), and if it's a decent one to do nothing with a buffer if its length is zero. And indeed looks like Linux does the latter.

[elichai]

@newpavlov I didn't say it returns null, I said "doesn't promise anything about the pointer" (meaning it can be null, or even dangling)
https://doc.rust-lang.org/nomicon/exotic-sizes.html#zero-sized-types-zsts
https://doc.rust-lang.org/nomicon/vec-zsts.html

I'm not a C expert, but from what I know stuff that aren't explicit are undefined, not OK. i'll try to research other OS to see what all of them currently do, but I would be warry here.

[elichai]

I know getrandom is a syscall and not part of the C standard. but sadly there's no equivalent for unix.
if we look at the C standard:

Each of the following statements applies unless explicitly stated otherwise in the detailed descriptions that follow: If an argument to a function has an invalid value (such as a value outside the domain of the function, or a pointer outside the address space of the program,or a null pointer, or a pointer to non-modifiable storage when the corresponding parameter is not const-qualified) or a type (after promotion) not expected by a function with variable number of arguments, the behavior is undefined.

7.1.4.p1.
http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1570.pdf

[newpavlov]

Well, interpreting it like this would make Rust std unsafe as well, since File::read is essentially just a tiny wrapper around libc::read, AFAIK without any checks for zero-length slices.

cc @RalfJung

[RalfJung]

@Aaron1011 landed a PR in Miri that explicitly allows any pointer if the length is 0. The authority to contact here would be the Linux syscall docs and maybe the glibc docs. The C standard has no bearing here, if anything we'd have to figure out what LLVM's rules are.

Cc @gnzlbg

I had problems in the past with Intel's libraries that passing NULL+0 to memset resulted in SEGFAULT.

That's because its argument is marked as nonnull and maybe even aligned (in LLVM).
Slices are aligned and never NULL, so they are always fine for things like memset.

This checks that the slice isn't a ZST. otherwise rust doesn't promise anything about the pointer from slice.as_mut_ptr()

It certainly promises that the ptr is non-NULL. Beyond that, most likely the pointer can also not be "dangling" in the sense that it points to a deallocated location. It can, however, most definitely be something like the constant 0x100 -- there does not have to be actually allocated memory there. (It is unclear whether LLVM makes a difference between "random constant cast to an integer" and "pointer that used to point to a valid allocation" -- but chances are there those are not the same, and the latter is "more dangerous" than the former.)

[elichai]

@RalfJung I agree that the C standard is not the right authority here. as i've said before, if you know anyone from glibc/linux/llvm to tag on this I would love to hear if there are some docs/guidelines specifying what is right here.

and btw, also passing ZST pointers to intel's memset/memcpy gave me problems.

about the nullness, the nomicon says:

standard allocators may return null when a zero-sized allocation is requested

what am I missing?

[RalfJung]

The Nomicon is wrong (and the fix landed recently).

Slices are never NULL.

passing ZST pointers to intel's memset/memcpy gave me problems.

Would be interesting if you can reproduce that. References are guaranteed to always be safe to pass to functions like memset/memcpy as they always are non-null, aligned, and dereferencable. It is UB to create a reference that violates this (even if the reference is never used).

Raw pointers are a different matter, of course. I am not sure what exactly you mean by "ZST pointer".

[gnzlbg]

http://man7.org/linux/man-pages/man2/getrandom.2.html

Says that it writes size bytes to the buffer, so getrandom(NULL, 0, ...) should be ok (nothing is written to the output buffer). It also says that if the pointer passed is outside the address space, it returns an error code.

[gnzlbg]

I mean, doesn't this crate has tests for this corner case?

EDIT: apparently not..

[elichai]

@RalfJung
A. good to know the nomicon is wrong :). I used this for a long time.
B. I can try. it happened to me with SGX, can't find the issue describing it but here's the commit "fixing" the problem in the project I worked at back then: scrtlabs/enigma-core@7cb0060

[newpavlov]

@gnzlbg

I mean, doesn't this crate has tests for this corner case?

Yes, it has. But we don't try corner cases like constructing zero-sized array on the heap with custom allocators. Also if it's indeed UB for some reason, then optimizations can break code unexpectedly one day.

[elichai]

@RalfJung We also had this problem here: rust-bitcoin/rust-secp256k1#141 (comment)
I think there somehow the ZST was identified as NULL in emscripten i'll need to reproduce to try and pinpoint exactly what was it (and it's pretty late here so idk if i'll be able to do it today)

[RalfJung]

it happened to me with SGX, can't find the issue describing it but here's the commit "fixing" the problem in the project I worked at back then: scrtlabs/enigma-core@7cb0060

The commit comment talks about 0x1 though, not 0x0.
A ZST pointer will in many cases be obtained by just casting the alignment to a pointer. So, &[]: &[u8] can be 0x1 (playground). That pointer is nonnull, aligned and dereferencable, so all I said above is true. Note that "dereferencable" is a property that depends on the size; the size here is 0 so for a ZST, all we demand is that a pointer be "dereferencable for accesses of size 0", which is trivially satisfied.

Not sure what SGX does that breaks this, but in "normal" environments this works fine. Or maybe the actual bug is elsewhere.

[elichai]

@RalfJung I'm not sure what is your "dereferencable for accesses of size 0" definition. if I'll go back to memset/memcpy which is the SGX example then again the C standard states:(7.24.1p2)

Where an argument declared a ssize_t n specifies the length of the array for a function, n can have the value zero on a call to that function. Unless explicitly stated otherwise in the description of a particular function in this subclause, pointer arguments on such a call shall still have valid values.

[elichai]

in particular llvm defines for memcpy:

If “len” is 0, the pointers may be NULL or dangling. However, they must still be appropriately aligned.

https://llvm.org/docs/LangRef.html#id435
but this is only correct for llvm, not part of the standard.

Edit: After thinking about this I don't think llvm has anything to do with this discussion. only linux/glibc docs for getrandom and the C standard for memcpy. As the .so lib that we FFI to could've been compiled with gcc/something else

[gnzlbg]

While it is true that memcpy(NULL, NULL, 0) is UB because the pointer arguments of memcpy are nonnull in C (not necessarily in LLVM-IR), I really have no idea what that has to do with the getrandom system call being used here.

[elichai]

@gnzlbg right. back to the topic :) I probably made a mistake by comparing to the C standard. the problem is that linux doesn't have docs/guidelines, at least non that I could find. and it seems better to go on the safe side and not provide NULL/0x01/pointer to random data location to the getrandom syscall.

As for actual definitions here and evidence i'm not sure how to check this without sending an email to the linux mailing list asking about it.

[gnzlbg]

In particular, we guarantee that assert!(!&[T].as_ptr().is_null()) always passes, because T has always an alignment requirement > 0 in Rust , and therefore null wouldn't be a valid pointer to it. Also &T to a ZST or a DST can't be null, because references can't be null.

[elichai]

I'm sorry but both you and @RalfJung keep repeating that, but is there an official declaration to what is null? i'm unsure null always equal 0, could casting the alignment to a pointer result in NULL in some weird architecture? idk.

My point is that even an aligned pointer that points to nowhere might not be valid (as it's not valid for memcpy i.e. memcpy(1,1,0) isn't valid AFAICT)

[gnzlbg]

I'm sorry but both you and @RalfJung keep repeating that, but is there an official declaration to what is null?

Null is all bits set to zero.

[gnzlbg]

could casting the alignment to a pointer result in NULL in some weird architecture? idk.

Not in Rust.

[elichai]

@gnzlbg but we're passing an FFI boundry to C here.

[gnzlbg]

My point is that even an aligned pointer that points to nowhere might not be valid (as it's not valid for memcpy i.e. memcpy(1,1,0) isn't valid AFAICT)

It doesn't really matter what C requires for memcpy. The only thing that matters is what the getrandom syscall requires. I linked its documentation above.

[gnzlbg]

@gnzlbg but we're passing an FFI boundry to C here.

If there is a platform where NULL is never all bits zero Rust cannot support it, so you can't write Rust code that calls C there.

[elichai]

right and the docs don't specify if the pointer should be valid and readable when the length is 0.

[gnzlbg]

The docs say that the function writes n bytes. I don't have any reason to believe that if n is zero the pointer is used for anything at all.

[gnzlbg]

The function declaration provided by kernel docs does not make the pointer nonnull either (or something else).

[gnzlbg]

If you have a reason to believe that this API does something with the pointer when n is zero, please share.

[josephlr]

I think we have enough information to resolve this one way or another. Here are the important questions:

1. Does the use of zero-length slices cause Undefined Behavior in getrandom's Rust code? The answer here is clearly no, as we don't to any invalid manipulations to the slice and don't manually dereference the slice pointer. Thanks @RalfJung and @gnzlbg for your help here.

2. When given a length of zero, can any of the system APIs return an error? This is less clear. APIs like the POSIX read(2) and Linux's getrandom(2) allow for the length to be zero, but can still check that the buffer pointer is "valid" (i.e. on Linux, that the address is inside the accessible address space) even if the passed length is zero. As we don't observe this issue in our tests, it looks like Rust slices don't contain "invalid" pointers (whatever that means), so the answer to this question is No. Also, as mentioned above, if this issue actually occurred, libstd would also have issues with libc::read.

3. Should we explicitly do nothing on zero-length slices for performance and consistency reasons? On some targets (Windows, WASM, Solaris, SGX, etc...), we already don't perform syscalls if the input slice is zero-length. An argument could be made that we should always do nothing on zero-length slices. I'm not sure what the answer to this question is.

@newpavlov, @dhardy I would say (3) is the only potential reason to merge this PR. What do you think?

[elichai]

@dhardy the only thing I would like to see here is __builtin_expect(false) to keep this out of the hot path. but that's micro optimization

[elichai]

Another thing, the read(2) syscall is a bit more defined:

If count is zero, read() may detect the errors described below. In
the absence of any errors, or if read() does not check for errors, a
read() with a count of 0 returns zero and has no other effects.

If that was written also for getrandom(2) I would be more inclined to agree with @josephlr

[dhardy]

@elichai given that we're talking about a system call here, is it even significant whether the branch predictor gets this right? (According to WP, a mis-prediction costs 10-20 clock cycles; according to this post a system call takes 100-200 cycles.)

Lets not get too carried away with optimisations.

[RalfJung]

@elichai

Unless explicitly stated otherwise in the description of a particular function in this subclause, pointer arguments on such a call shall still have valid values.

The question is, what is "valid"? "valid" is a property relative to a size -- a pointer that is valid for accesses of size 4 might not be valid for accesses of size 8. So what is "valid for accesses of size 0"? Everything I know says that this just means "non-null and aligned" (possibly with an exception for "logical pointers" in the sense of this paper that are dangling, but that wouldn't affect integers cast to a pointer).

In particular, that is what LLVM demands for such functions.

in particular llvm defines for memcpy:

If “len” is 0, the pointers may be NULL or dangling. However, they must still be appropriately aligned.

Correct. And a slice is always appropriately aligned, so we are good.

i.e. memcpy(1,1,0) isn't valid AFAICT

Do you have any reference or indication for this? I think this is valid, and in my reading neither the LLVM docs nor the C standard say otherwise.

Rust doesn't care much about C, but for LLVM we do rely on that being valid.

---

On some targets (Windows, WASM, Solaris, SGX, etc...), we already don't perform syscalls if the input slice is zero-length.

I am curious if this is based on misunderstandings of Rust behavior (of which there clearly are a lot; I hope fixing the Nomicon will help here in the long run), bugs in other parts of the toolchain, or actual mismatches on what is considered "valid".

Lets not get too carried away with optimisations.

I am less worried about performance and more worried about a cargo cult of "Rust's empty slices need special treatment when interfacing with other stuff", in the absence of data/specs showing that this really is the case.

[dhardy]

I am less worried about performance and more worried about a cargo cult of ...

Suit yourself; personally I'm more worried about wasting a lot of time on easily-solvable matters.

I am curious if this is based on misunderstandings of Rust behavior ...

This may just be a side-effect of chunking dest (some systems have a max. length), then looping over the list of chunks (which may be an empty list).

[gnzlbg]

@elichai

Personally I'm still not convinced, it's pretty upsetting that I getrandom(2) doesn't have a well defined contract of how it should behave in case of NULL/dangling/length=

If you are not sure, then inform yourself: open a Linux kernel documentation bug, send an email to the Linux kernel mailing list asking the question, etc. Speculating about this is a waste of everybody's time - we see the same docs you do, but talking about them here doesn't fix anything.

[dhardy]

Actually, we should mention that there is no system call when dest.is_empty().

[elichai]

@RalfJung when talking about FFI llvm's reference doesn't matter much because dynlibs might be compiled with gcc/other compilers.
I will send later an email to the linux mailing lit to figure out the contract of getrandom(2).
About memcpy(1,1,0) everyone I asked that is very familiar with the C standard told me that it's implementation defined, I'm unsure if there's an authority on this, if there are I personally would like to hear :)

I'm not saying i'm 100% right, and my life would be easier if it turns out i'm wrong :)
and I agree with @dhardy that this is an easily solvable and will remove the syscall calling in that case.

[gnzlbg]

About memcpy(1,1,0) everyone I asked that is very familiar with the C standard told me that it's implementation defined, I'm unsure if there's an authority on this, if there are I personally would like to hear :)

Which part of the C standard does it make you think that it doesn't work?

[elichai]

I know getrandom is a syscall and not part of the C standard. but sadly there's no equivalent for unix.
if we look at the C standard:

Each of the following statements applies unless explicitly stated otherwise in the detailed descriptions that follow: If an argument to a function has an invalid value (such as a value outside the domain of the function, or a pointer outside the address space of the program,or a null pointer, or a pointer to non-modifiable storage when the corresponding parameter is not const-qualified) or a type (after promotion) not expected by a function with variable number of arguments, the behavior is undefined.

7.1.4.p1.
http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1570.pdf

Specifically both the "outside address space" and a pointer to non-modifiable storage when the corresponding parameter is not const-qualified)

[RalfJung]

when talking about FFI llvm's reference doesn't matter much because dynlibs might be compiled with gcc/other compilers.

True, for getrandom this is not about LLVM (nor about the C standard).

memset is not FFI though, LLVM treats it as a primitive.

specifically both the "outside address space" and *a pointer to non-modifiable storage when the corresponding parameter is not const-qualified) *

I'd argue a ZST pointer "points to" no memory, so in particular it doesn't point to non-modifiable storage... but well, I think we can agree that the standard is less clear than it could be. That's true for many parts of the standard though. ;)

Lucky enough for us, what C has to say about memset is not relevant here.

[gnzlbg]

1 is inside the address space of the program for Rust programs at least, but it is not modifiable memory, so memcpy(1, 1, 0) would indeed be UB in C.

[elichai]

@RalfJung I'm away from my laptop but I'm curious now why this commit rust-bitcoin/rust-secp256k1@e8a1513 fails with cargo web test --verbose --target=asmjs-unknown-emscripten by saying the pointer to an empty slice is null.

(in particular this is the test that crashes: https://github.com/rust-bitcoin/rust-secp256k1/blob/master/src/key.rs#L478)

[newpavlov]

@dhardy
I don't think we should add such checks without a clear indication that they are really needed. As I've shown earlier, Rust std does pass pointers to libc functions without the check proposed in this PR. Also see the Linux code linked earlier, it processes null pointer with a zero length absolutely without issues. I suspect we will find a similar code for read as well.

UPD: Although, it may be worth from consistency PoV, we could document that passing zero-length slices to getrandom will immediately return Ok(()) without talking to the host OS. Right now on some targets we will talk to the OS and will not on others.

@elichai
Can you update docs accordingly? It may be also worth to add #[inline] attribute.

[josephlr]

UPD: Although, it may be worth from consistency PoV, we could document that passing zero-length slices to getrandom will immediately return Ok(()) without talking to the host OS. Right now on some targets we will talk to the OS and will not on others.

Agreed, but only for consistency reasons (and to just get this resolved).

Can you update docs accordingly? It may be also worth to add #[inline] attribute.

Good idea. Docs update should probably be in second paragraph of the getrandom::getrandom function documentation.

I agree that we should look into #[inline] attributes, but that's probably best reserved for a future PR, so we could look at the effect in isolation.

[dhardy]

I agree that we should look into #[inline] attributes

#[inline] has two effects: enable inlining across crate boundaries, and tweak the heuristics used for inlining of the function. In this case inlining should save a double function call (none of the getrandom_inner impls have #[inline]), however it might be preferable to not inline this function but instead the getrandom_inner function via #[inline(always)]. So yes, a new PR may be better.

[gnzlbg]

If you end up adding a check for slices of zero length, inlining this function inlines the check, which might allow the check and the getrandom_inner call to be removed at compile-time.

[dhardy]

Yes, alternatively inlining getrandom_inner will on some platforms allow the check to be optimised out: several platforms chunk dest then loop over the chunks. Overall I guess this may be more useful.

[josephlr]

@dhardy @newpavlov I added the necessary docs. Can we merge this?

[elichai]

For future reference and for the sake of honesty etc.
I want to say that after working more closely with the kernel i'm now convinced that @gnzlbg was/is right.
The kernel syscalls api is fairly robust and shouldn't introduce UB on it's side even with contract violations.

For actual code references even if this check wasn't there: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/char/random.c#n1642 the kernel will always use the copy_to_user function, which checks validity of pointer, address space etc.

I'm not saying that this PR is necessarily bad, just wanted to admit when I'm mistaken. and I now think that I was mistaken.