Don't cast thread name to an integer for prctl

[saethlin]

libc::prctl and the prctl definitions in glibc, musl, and the kernel headers are C variadic functions. Therefore, all the arguments (except for the first) are untyped. It is only the Linux man page which says that prctl takes 4 unsigned long arguments. I have no idea why it says this.

In any case, the upshot is that we don't need to cast the pointer to an integer and confuse Miri.

But in light of this... what are we doing with those three 0s? We're passing 3 i32s to prctl, which doesn't fill me with confidence. The man page says unsigned long and all the constants in the linux kernel are macros for expressions of the form 1UL << N. I'm mostly commenting on this because looks a whole lot like some UB that was found in SQLite a few years ago: https://youtu.be/LbzbHWdLAI0?t=1925 that was related to accidentally passing a 32-bit value from a literal 0 instead of a pointer-sized value. This happens to work on x86 due to the size of pointers and happens to work on x86_64 due to the calling convention. But also, there is no good reason for an implementation to be looking at those arguments. Some other calls to prctl require that other arguments be zeroed, but not PR_SET_NAME... so why are we even passing them?

I would prefer to end such questions by either passing 3 libc::c_ulong, or not passing those at all, but I'm not sure which is better.

[rust-highfive]

r? @dtolnay

(rust-highfive has picked a reviewer for you, use r? to override)

[RalfJung]

My understanding is that variadic functions are special and all scalar types are passed in a consistent way. But I don't actually properly understand what is going on. Some links from the last time this came up:

• Make weak syscalls in std work. miri#1629 (comment)
• linux: try to use libc getrandom to allow interposition #78785 (comment)
• unix/weak: pass arguments to syscall at the given type #79196
• check that all syscall arguments are scalars miri#1570

We ended up ensuring that all variadic arguments of syscall are scalars, but I don't think we have a check against a 'too small' scalar being passed. I don't think we do the same check for prctl, because I didn't know that it actually is variadic.

[workingjubilee]

I would like to note that in #95026 which we will eventually land (plausible delays notwithstanding) that we will be capable of simply eliminating this particular call from the codebase by using the pthread abstraction instead.

[RalfJung]

Fair. But it might still be worth figuring out what we have to do with variadic functions...

[saethlin]

I don't have all the context here, but I did just do my best to read over the comments in all the PRs you linked.

It sounds to me like the previous discussion has interleaved a lot of what i686/x86_64-linux-gnu do and what C guarantees. I can't tell if this is with good reason or not, but it makes me uncomfortable. So I'm going to lay out how I understand the situation, and maybe this will help clarify things, or other people can straighten me out if I misunderstand.

On i686/x86_64-linux-gnu, you get particular behavior due to the calling convention. The first 6 integer or pointer arguments are passed in registers, which means there might as well be an as usize or as isize on all of the arguments. (I have no idea how u128 works)

In C however, the callee varargs API is that you declare a va_list, you call va_start passing it the name of the last argument before the variadic arguments, then you call va_arg passing it the va_list and the type (yay macros) of the next argument to extract from the va_list. That's how you get the arguments out. You call va_end when you're done. C also specifies that the argument promotions apply when calling a variadic argument, which basically convert everything smaller than an int to an int or unsigned int.

The reason I'm bringing this up is that due to the calling convention above, on x86_64 you can pass a bunch of nonnegative i32s in Rust to a C variadic function, and even if the callee attempts to parse usize, it will succeed due to the widening that the calling convention applies. But you are not guaranteed this behavior across all platforms. Therefore, in general varargs are super dangerous because if the calling convention specifies that arguments are passed on the stack instead, relying on widening becomes trouble. If you pass two i32 to a variadic function that expects 2 usize, it will probably grab all the data in the first usize and the second pop will read who-knows-what as the second argument. So in this prctl call, if those last 3 0s were important, they'd be parsed by some va_arg calls (and docs indicate that they would be parsed as unsigned long/usize), and we would have a big problem on a platform where those arguments were passed on the stack. But on i686/x86_64-linux-gnu things would work fine.

I saw that the other PRs were about syscalls and (old?) Miri. I've read over Miri's shims, but I don't understand how a system calling convention could be relevant there. It probably doesn't matter a whole lot because Miri is solidly an interpreter, it's not intercepting system calls after a call instruction. This whole situation just makes me uncomfortable, because it's a bunch of unexplained zeroes in unsafe and platform-dependent code.

[RalfJung]

I saw that the other PRs were about syscalls and (old?) Miri. I've read over Miri's shims, but I don't understand how a system calling convention could be relevant there.

The goal was for Miri to check that the caller passes all arguments with the right type, or at least the right size. Basically, if it is accepted by Miri, it should also work in the real program.

[RalfJung]

it's a bunch of unexplained zeroes in unsafe and platform-dependent code.

FWIW, even when ignoring platform-specific properties, it looks like extra trailing arguments are fine -- the number of va_arg calls in the callee does not have to exactly match the number of arguments that the caller passed, right?

But of course we don't know how often prctl calls va_arg, and with which types...

[saethlin]

Yes, trailing arguments ought to be ignored. Which is why I'm uncomfortable with the fact that they are in here, because for some other prctl calls (PR_GET_SPECULATION_CTRL, PR_SET_SPECULATION_CTRL), the man page stipulates that the unused "arguments must be specified as 0; otherwise the call fails with the error EINVAL." https://man7.org/linux/man-pages/man2/prctl.2.html so I'm just left wondering if the original writer of this code originally had an extern declaration that took 6 arguments, they were extrapolating from the man page, they knew something I don't, or this is just some other kind of cruft that I'm now making too big a deal out of :p

[eddyb]

We should add a property computed in e.g. FnAbi of whether C variadics are compatible with non-variadics (even when FnAbi doesn't already encode the possible distinction at the argument level - which it won't in many cases, unless we start computing details LLVM handles today).

AFAIK SysV x86_64 does have this compatibility, at the cost of their va_list having to track two kinds of registers (gp: general-purpose r*, fp: SSE/AVX's xmm*/ymm*/zmm*) and the stack:

rust/library/core/src/ffi/mod.rs

Lines 295 to 301 in ac4b345

	pub struct VaListImpl<'f> {
	gp_offset: i32,
	fp_offset: i32,
	overflow_arg_area: *mut c_void,
	reg_save_area: *mut c_void,
	_marker: PhantomData<&'f mut &'f c_void>,
	}

Some other ABIs that offer C variadic compatibility are similar in complexity (e.g. PowerPC and AArch64's AAPCS, at least AFAICT from core::ffi), or get away with a simpler va_list just because their argument passing is always stack-only anyway (e.g. most x86_32 ABIs).

But it is important to keep track of when this isn't the case. I believe I saw some 32-bit ARM stuff that was incompatible only for floats (integers in variadic arguments would still use registers), and more recently Apple's non-standard (i.e. not AAPCS) AArch64 ABI has always-on-stack variadics, with their documentation explicitly calling it out as a compatibility hazard.

IMO C should've mandated one of:

• full compatibility between variadic and non-variadic
  • this would penalizes implementations/ABIs with "interesting" calling conventions
• va_list is always a simple pointer to memory containing the arguments
  • more specifically, that memory would have the same layout as if the variadic arguments were placed in sequence in a struct variable, and a pointer to that struct got passed instead
  • this would make it more like sugar for a struct (or even an array when uniform), instead of interacting with the calling convention at the argument level

[RalfJung]

IMO C should've mandated one of:

Yeah, it should, but it didn't... so what do we do on the Rust side?^^ We have to match whether the function is variadic or not in the platform (which Miri currently doesn't check, i.e., it basically assumes "full compatibility between variadic and non-variadic"), and we have to match the types use by the va_arg in the implementation. And that seems poorly documented?

[saethlin]

I don't know what you mean by compatibility between variadic and non-variadic. If a function accepts two u64 and you try to pass is two i32, that's wrong whether or not it's variadic.

In terms of matching types, yes this is just a disaster in C. In many places, the only documentation that exists simply says to pass the result of some macro expansion. But due to ABI requirements, what type that macro expands to should be stable for a target triple. And it is probably reasonably stable for just architecture and operating system. It's just quite unfortunate that the only "documentation" is the headers.

[eternaleye]

@saethlin

I don't know what you mean by compatibility between variadic and non-variadic.

Compatible: the following code "works", as do its moral equivalents like "casting the variadic foo's function pointer to int(int, int)":

• foo.c

int foo(int a, ...) {
    va_list args;
    va_start(args, a);
    int b = va_arg(args, int);
    va_end(args);
    return a + b;
}

• foo.h

int foo(int, int);

• main.c

#include <stdio.h>
#include "foo.h"

int main() {
    printf("%d", foo(40, 2));
}

In other words, when they're "compatible", the caller doesn't need to know that the callee is variadic to call it - it just calls it as if it was a function with exactly the callsite's signature.

When they're "incompatible", then the caller must know that the callee is variadic, and handle it appropriately.

[Stargateur]

@eternaleye I'm not sure it's allowed to do that in C.

@eddyb C purpose have always to be as portable as possible (in 1980 it was :p), thus blaming C serve no purpose

---

libc::prctl and the prctl definitions in glibc, musl, and the kernel headers are C variadic functions. Therefore, all the arguments (except for the first) are untyped. It is only the Linux man page which says that prctl takes 4 unsigned long arguments. I have no idea why it says this.

The libc crates clearly use variadic https://docs.rs/libc/0.2.121/libc/fn.prctl.html

I would prefer to end such questions by either passing 3 libc::c_ulong, or not passing those at all, but I'm not sure which is better.

Normally we can remove them but I would let them cause the code suggest they check all arg every-time. It's very unclear what they are doing https://github.com/torvalds/linux/blob/7403e6d8263937dea206dd201fed1ceed190ca18/kernel/sys.c#L2342

For the type according to doc:

PR_SET_NAME (since Linux 2.6.9) Set the name of the calling thread, using the value in the location pointed to by (char *) arg2. The name can be up to 16 bytes long, including the terminating null byte. (If the length of the string, including the terminating null byte, exceeds 16 bytes, the string is silently truncated.)

So we need a char * not an unsigned long. example that use char * and not with 3 following 0 https://github.com/torvalds/linux/blob/e3a8b6a1e70c37702054ae3c7c07ed828435d8ee/tools/perf/builtin-bench.c#L202. So it's could suggest it's would be ok.

And that seems poorly documented?

@RalfJung I just don't understand why they did this, I suggest we contact them. It's not the first time people are confuse https://stackoverflow.com/questions/36551394/correct-way-to-use-prctl. Who like mailing list ?

[eternaleye]

@eternaleye I'm not sure it's allowed to do that in C.

There's a lot of C in the world that's non-conformant but "works in practice". That's why Apple had to be careful to call out the fact that their ABI makes them incompatible - whether it's "allowed" or not, people do it.

It also complicates the lives of any third-party compilers that need to link to C - the situation Rust is in - because it means that tricks based on writing the more-friendly signature (e.g. using *const or *mut rather than usize) in the FFI binding don't necessarily work.

[saethlin]

@RalfJung

We have to match whether the function is variadic or not in the platform (which Miri currently doesn't check

I think we should eventually expand the degree to which Miri understand the vagaries of platform behavior. Doing so will probably be gross to implement, but I think that's just the price you pay for validating that you're using each target triple correctly. Or we could punt on this whole problem, like Miri does with threads. We don't catch all UB, but we catch what we can and narrow in over time. Personally, I'd very strongly prefer that we have less false positives. Miri has earned a reputation for having false positives for many reasons including past issues with open. And we currently may or may not have a false positive in the crate caps because it calls libc::prctl with 4 arguments:

   --> /root/build/src/bounding.rs:27:24
    |
27  |     let ret = unsafe { libc::prctl(nr::PR_CAPBSET_READ, libc::c_uint::from(cap.index()), 0, 0) };
    |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ incorrect number of arguments: got 4, expected 5
    |

As far as I can tell from the man page, this call is fine.

and we have to match the types use by the va_arg in the implementation. And that seems poorly documented?

I would be surprised if this is documented at all. So yes, this is not easy to validate, and if we do not have access to the source for the call we can't validate it at all so we'd need some other kind of approach for such calls. For example, we could accept integer or pointer arguments. It might be kind of wobbly, but if that's the interface... 🤷

---

Replace C by Rust in your sentence, does it feel fair ?

Feels fair to me. We should be building interfaces where it is possible for people to be confident they are using them correctly, and we shouldn't make excuses. A formal memory model would be a good step in that direction ;)

[RalfJung]

And we currently may or may not have a false positive in the crate caps because it calls libc::prctl with 4 arguments:

That call is weird though, why would it add 2 zeroes but then not also add the third one? If the goal is not to conform to the type from the man page, then why add any zeroes at all?

[RalfJung]

FWIW, discussing whom to "blame" is off-topic here. This is for technical discussion, we need to figure out what ABIs/APIs platforms actually provide and what we have to do to be sure that we are calling these operations correctly.

Given that the type signatures in the manpage and libc differ, we could decide to rely on the fact that PR_SET_NAME doesn't say anything about the other arguments. We could also check the implementation code, I guess. Or we could keep passing at least 5 arguments (@saethlin mentioned 6 arguments above, not sure what that is about), but then we should cast them to the type given in the manpage, rather than passing i32 like we do now.

[RalfJung]

Oh also Cc @rust-lang/libs who might have other (better) ideas for what to do here. :)

[saethlin]

we need to figure out what ABIs/APIs platforms actually provide and what we have to do to be sure that we are calling these operations correctly.

I do not think this will always be possible. These are platform APIs, so in some cases I think "whatever the platform accepts" will end up being the API as best it can be defined. If your code only runs on x86_64-linux-gnu and you're writing an ABI, why would you bother specifying whether this accepts a raw pointer, a pointer-sized integer, or any integer smaller than that? I hope that libs has a better answer.

(@saethlin mentioned 6 arguments above, not sure what that is about)

An honest off-by-one error.

[cuviper]

We could also check the implementation code, I guess.

I checked glibc, musl, and uClibc, and they all blindly read all 4 long args after the int option to forward to the syscall. They do not try to understand the option or how many arguments it is "supposed" to have.

Glibc is rather blunt about that:
https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/prctl.c;hb=HEAD

/* Unconditionally read all potential arguments.  This may pass
   garbage values to the kernel, but avoids the need for teaching
   glibc the argument counts of individual options (including ones
   that are added to the kernel in the future).  */

Musl does the same without comment:
https://git.musl-libc.org/cgit/musl/tree/src/linux/prctl.c

uClibc varies a little by arch -- avr32 and c6x both use a variadic definition that reads all 4 long arguments, while common defines it as a function of an int and 4 longs -- I suppose all other supported arches are "compatible" in that sense. The common header still declares it as variadic.
https://git.uclibc.org/uClibc/tree/libc/sysdeps/linux/avr32/prctl.c
https://git.uclibc.org/uClibc/tree/libc/sysdeps/linux/c6x/prctl.c
https://git.uclibc.org/uClibc/tree/libc/sysdeps/linux/common/prctl.c
https://git.uclibc.org/uClibc/tree/libc/sysdeps/linux/common/sys/prctl.h

[RalfJung]

@cuviper thanks for the investigation!

I would then argue that

• calling libc::prctl with 3 variadic arguments (4 arguments total) is actually UB (since some implementations always call va_arg 4 times), and not a false positive in Miri
• we should change the 0 to 0 as libc::c_ulong, and Miri should check for all arguments having that size

[RalfJung]

Hm, OTOH syscall also always reads 6 arguments in glibc and we don't require that in Miri (and it doesn't even match the manpage). So maybe we shouldn't read too much into that and the glibc people are just fine with va_arg going OOB...

So maybe fewer arguments are defensible, but the arguments we pass should at least all have the right size?

[cuviper]

I would like to note that in #95026 which we will eventually land (plausible delays notwithstanding) that we will be capable of simply eliminating this particular call from the codebase by using the pthread abstraction instead.

One caution here is that PR_SET_NAME will silently truncate names longer than 16 bytes (TASK_COMM_LEN), while pthread_setname_np checks the length and returns ERANGE. We should probably keep that truncation ourselves.

[Stargateur]

It's UB on paper and I would argue it's they fault, but I agree I don't see any problem doing that like glibc say it's just garbage, I expect they don't use the arg if not needed, on system where it would cause problem (like system that implement variadic using allocation memory... I don't know one but it's allowed in C standard) I expect code implementation to respect variadic convention.

It's funny, they invented the "variadic but in fact no" function call convention. I think people making man page have done they best to find a (strange) middle ground.

[cuviper]

r? @cuviper

@bors r+ rollup

[bors]

📌 Commit e8a6f53 has been approved by cuviper

[RalfJung]

It's UB on paper and I would argue it's they fault, but I agree I don't see any problem doing that like glibc say it's just garbage

We just have to hope nobody ever inlines prctl so that the compiler sees that the number of va_arg is wrong and then optimizes away this code since it has UB...

[cuviper]

One more for good measure -- Android's bionic uses a generated syscall stub from a non-variadic decl:
https://android.googlesource.com/platform/bionic/+/refs/heads/master/libc/SYSCALLS.TXT#84

int     prctl(int, unsigned long, unsigned long, unsigned long, unsigned long) all