stdlib makes assumptions about errors returned by POSIX functions

[tavianator]

Split off from rust-lang/miri#1981 (comment)

For example, we assume that the return value of pthread_rwlock_rdlock() is either 0, EAGAIN, or EDEADLK:

rust/library/std/src/sys/unix/rwlock.rs

Lines 51 to 54 in ac470e9

	// According to POSIX, for a properly initialized rwlock this can only
	// return EAGAIN or EDEADLK or 0. We rely on that.
	debug_assert_eq!(r, 0);
	self.num_readers.fetch_add(1, Ordering::Relaxed);

And that's kinda what POSIX documents: https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_rwlock_rdlock.html

RETURN VALUE

If successful, the pthread_rwlock_rdlock() function shall return zero; otherwise, an error number shall be returned to indicate the error. ...

ERRORS

... The pthread_rwlock_rdlock() and pthread_rwlock_tryrdlock() functions may fail if:

[EAGAIN]
The read lock could not be acquired because the maximum number of read locks for rwlock has been exceeded.

The pthread_rwlock_rdlock() function may fail if:

[EDEADLK]
A deadlock condition was detected or the current thread already owns the read-write lock for writing.

These functions shall not return an error code of [EINTR].

But the thing is, POSIX error sections are not exhaustive: https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_03

Implementations may generate error numbers listed here under circumstances other than those described, if and only if all those error conditions can always be treated identically to the error conditions as described in this volume of POSIX.1-2017. Implementations shall not generate a different error number from one required by this volume of POSIX.1-2017 for an error condition described in this volume of POSIX.1-2017, but may generate additional errors unless explicitly disallowed for a particular function.

And even the docs for that function mention "shall not return an error code of [EINTR]" rather than "shall not return any other error code". So unless there's limits documented elsewhere, I don't think it's sound to assume that there can't be another error code returned from these functions.

CC: @RalfJung

[RalfJung]

Cc @rust-lang/libs

[cuviper]

In general, we should be bubbling up errors that we can't otherwise handle. In this specific instance where we aren't returning Result, we should probably promote that debug assert to a real assert, with a message like "unexpected error: {:?}". If there are others like this, let's treat them on a case-by-case basis.

[kckeiks]

@rustbot claim

[kckeiks]

@RalfJung I found these that might be of interest

• https://github.com/rust-lang/rust/blob/master/library/std/src/sys/unix/thread_local_key.rs#L16
• https://github.com/rust-lang/rust/blob/master/library/std/src/sys/unix/thread_local_key.rs#L27
• https://github.com/rust-lang/rust/blob/master/library/std/src/sys/unix/condvar.rs#L69
• https://github.com/rust-lang/rust/blob/master/library/std/src/sys/unix/condvar.rs#L76
• https://github.com/rust-lang/rust/blob/master/library/std/src/sys/unix/condvar.rs#L82
• https://github.com/rust-lang/rust/blob/master/library/std/src/sys/unix/condvar.rs#L183
• https://github.com/rust-lang/rust/blob/master/library/std/src/sys/unix/fs.rs#L558
• https://github.com/rust-lang/rust/blob/master/library/std/src/sys/unix/mutex.rs#L63
• https://github.com/rust-lang/rust/blob/master/library/std/src/sys/unix/mutex.rs#L68
• https://github.com/rust-lang/rust/blob/master/library/std/src/sys/unix/mutex.rs#L78
• https://github.com/rust-lang/rust/blob/master/library/std/src/sys/unix/mutex.rs#L115
• https://github.com/rust-lang/rust/blob/master/library/std/src/sys/unix/mutex.rs#L125
• https://github.com/rust-lang/rust/blob/master/library/std/src/sys/unix/mutex.rs#L130
• https://github.com/rust-lang/rust/blob/master/library/std/src/sys/unix/mutex.rs#L140
• https://github.com/rust-lang/rust/blob/master/library/std/src/sys/unix/rwlock.rs#L75
• https://github.com/rust-lang/rust/blob/master/library/std/src/sys/unix/rwlock.rs#L114
• https://github.com/rust-lang/rust/blob/master/library/std/src/sys/unix/rwlock.rs#L132
• https://github.com/rust-lang/rust/blob/master/library/std/src/sys/unix/thread.rs#L115
• https://github.com/rust-lang/rust/blob/master/library/std/src/sys/unix/thread.rs#L263
• https://github.com/rust-lang/rust/blob/master/library/std/src/sys/unix/thread_local_key.rs#L16
• https://github.com/rust-lang/rust/blob/master/library/std/src/sys/unix/thread_local_key.rs#L27

[kckeiks]

If someone can confirm if we should update all of these functions (promoting assert_debug to assert) then I'll go ahead and open a PR.

[tavianator]

Yeah I figured this might be a more general problem than just that one example. I think most of them should become unconditional asserts. It would be helpful if the assertion message included errno (or the returned error code for the pthread functions).

It might be tempting to ignore failures of _destroy() functions in Drop impls, but that can theoretically lead to UB, since the allocation might get re-used and "attempting to initialize an already initialized {mutex,condition variable,...} results in undefined behavior."

These are the special cases I noticed:

rust/library/std/src/sys/unix/fs.rs

Lines 558 to 559 in b97dc20

	let r = unsafe { libc::closedir(self.0) };
	debug_assert_eq!(r, 0);

closedir() can return EINTR which should be treated like success. EIO is probably possible too from the underlying close() call and should probably be ignored. I think it's worth checking for EBADF (which would indicate an earlier violation of I/O safety). But it might also be fine to ignore all errors here, like File::drop does.

rust/library/std/src/sys/unix/thread.rs

Lines 115 to 116 in b97dc20

	let ret = unsafe { libc::sched_yield() };
	debug_assert_eq!(ret, 0);

It probably doesn't matter if sched_yield() fails, since it has no observable effects (and no one should call it anyway).

[RalfJung]

These are the special cases I noticed:

No strong opinion on what to do with them, but if we opt to not change the code for the reasons you mention, we should still add a comment explaining exactly that.

[vincenzopalazzo]

This should be fixed in #94712

[RalfJung]

That fixes one instance of this problem; #94705 (comment) lists a couple more (and I am not sure if that list is exhaustive).

[vincenzopalazzo]

@rustbot claim

Ok! @RalfJung, I can take a look into it

[RalfJung]

#94705 (comment) lists a lot of these assertions, only 2 or 3 have been changed by the PRs so far I think? Not sure the issue is closed.

[RalfJung]

Actually a lot of these links are dead. So the next step probably would be to figure out if there are cases of this left in the stdlib where we assume error lists are exhaustive. A quick search did uncover some:

• rust/library/std/src/sys/unix/thread_parker.rs

  Line 96 in 90c59e7

  debug_assert!(r == libc::ETIMEDOUT || r == 0);

• rust/library/std/src/sys/unix/locks/pthread_rwlock.rs

  Line 115 in e642c59

  debug_assert_eq!(r, 0);

And there are a whole bunch of debug-only assertions without further comment, listing just a few:

• rust/library/std/src/sys/unix/locks/pthread_mutex.rs

  Line 90 in d722944

  debug_assert_eq!(r, 0);

• rust/library/std/src/sys/unix/locks/pthread_mutex.rs

  Line 95 in d722944

  debug_assert_eq!(r, 0);

• rust/library/std/src/sys/unix/locks/pthread_mutex.rs

  Line 105 in d722944

  debug_assert_eq!(r, 0);

This list is by no means exhaustive.
@rust-lang/libs what is your take on this? So far we only heard from @cuviper that these should become proper assertions, but then when PRs are submitted there often is pushback against that (at least that's what PR authors report). Surprisingly though nobody made any comment in this issue. Cc @Amanieu @thomcc

[vincenzopalazzo]

@RalfJung Yes this was done in my previous PR #99624 but this not pass the code review! I will open a thread on zulip

Theread link