Created attachment 47704 [details] simple fix, not necessarily right for upstream This code directly passes a userspace timespec struct to the SYS_futex syscall, which does not work if the userspace type is 64-bit but the syscall expects legacy 32-bit timespec. I'm attaching the patch we're using in musl-cross-make to fix this. It does not attempt to use the SYS_futex_time64 syscall, since that would require fallback logic with cost tradeoffs for which to try first, and since the timeout is relative and therefore doesn't even need to be 64-bit. Instead it just uses the existence of SYS_futex_time64 to infer that the plain SYS_futex uses a pair of longs, and converts the relative timestamp into that. This assumes that any system where the libc timespec type has been changed for time64 will also have had its headers updated to define SYS_futex_time64. Error handling for extreme out-of-bound values should probably be added.
(In reply to Rich Felker from comment #0) > Created attachment 47704 [details] > simple fix, not necessarily right for upstream I like this solution for upstream too. It's simple and it works. > This assumes that any system where the libc timespec type has been > changed for time64 will also have had its headers updated to define > SYS_futex_time64. The futex.cc file is only used for linux targets, so the range of possible implementations we need to handle is limited and the assumption seems safe. > Error handling for extreme out-of-bound values should probably be added. I've created PR 93456 for that.
Rather than #if defined(SYS_futex_time64), I think it should be made: #if defined(SYS_futex_time64) && SYS_futex_time64 != SYS_futex This is in consideration of support for riscv32 and future archs without legacy syscalls. It's my intent in musl to accept the riscv32 port with SYS_futex defined to be equal to SYS_futex_time64; otherwise all software making use of SYS_futex gratuitously breaks.
Ack. I assume you saw PR 97182 and that's what prompted your comment. Otherwise it's quite a coincidence :)
Actually I didn't see it, I just saw Florian added to CC and it reminded me of the issue, which reminded me I needed to check this for riscv32 issues with the riscv32 port pending merge. :-)
Created attachment 49267 [details] Proposed fix Rich, does this work for musl? As glibc hasn't updated its userspace timespec yet I have no way to test whether this fixes the problem, because I don't have the problem. Is inspecting __USE_TIME_BITS64 allowed with musl? The current proposal at https://sourceware.org/glibc/wiki/Y2038ProofnessDesign#API_design says users can check if that's defined, although usually the __USE_xxx macros are not meant for public consumption.
On second thoughts, we probably don't need to worry about SYS_clock_gettime_time64. We only use SYS_clock_gettime syscalls on old glibc systems where clock_gettime is in librt not libc, and those systems aren't going to get updated with the time64 version.
Indeed, the direct clock_gettime syscall stuff is just unnecessary on any modern system, certainly any time64 one. I read the patch briefly and I don't see anywhere it would break anything, but it also wouldn't produce a useful Y2038-ready configuration, so I don't think it makes sense. Configure or source-level assertions should just ensure that, if time_t is larger than long and there's a distinct time64 syscall, the direct syscall is never used.
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>: https://gcc.gnu.org/g:4d039cb9a1d0ffc6910fe09b726c3561b64527dc commit r11-5022-g4d039cb9a1d0ffc6910fe09b726c3561b64527dc Author: Jonathan Wakely <jwakely@redhat.com> Date: Thu Sep 24 17:35:52 2020 +0100 libstdc++: Use custom timespec in system calls [PR 93421] On 32-bit targets where userspace has switched to 64-bit time_t, we cannot pass struct timespec to SYS_futex or SYS_clock_gettime, because the userspace definition of struct timespec will not match what the kernel expects. We use the existence of the SYS_futex_time64 or SYS_clock_gettime_time64 macros to imply that userspace *might* have switched to the new timespec definition. This is a conservative assumption. It's possible that the new syscall numbers are defined in the libc headers but that timespec hasn't been updated yet (as is the case for glibc currently). But using the alternative struct with two longs is still OK, it's just redundant if userspace timespec still uses a 32-bit time_t. We also check that SYS_futex_time64 != SYS_futex so that we don't try to use a 32-bit tv_sec on modern targets that only support the 64-bit system calls and define the old macro to the same value as the new one. We could possibly check #ifdef __USE_TIME_BITS64 to see whether userspace has actually been updated, but it's not clear if user code is meant to inspect that or if it's only for libc internal use. libstdc++-v3/ChangeLog: PR libstdc++/93421 * src/c++11/chrono.cc [_GLIBCXX_USE_CLOCK_GETTIME_SYSCALL] (syscall_timespec): Define a type suitable for SYS_clock_gettime calls. (system_clock::now(), steady_clock::now()): Use syscall_timespec instead of timespec. * src/c++11/futex.cc (syscall_timespec): Define a type suitable for SYS_futex and SYS_clock_gettime calls. (relative_timespec): Use syscall_timespec instead of timespec. (__atomic_futex_unsigned_base::_M_futex_wait_until): Likewise. (__atomic_futex_unsigned_base::_M_futex_wait_until_steady): Likewise.
(In reply to Rich Felker from comment #7) > Indeed, the direct clock_gettime syscall stuff is just unnecessary on any > modern system, certainly any time64 one. I read the patch briefly and I > don't see anywhere it would break anything, but it also wouldn't produce a > useful Y2038-ready configuration, so I don't think it makes sense. Configure > or source-level assertions should just ensure that, if time_t is larger than > long and there's a distinct time64 syscall, the direct syscall is never used. I didn't get around to making use of the new syscalls for GCC 11, so I've committed that patch. I'll keep this bug open to revisit it and do it properly later.
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>: https://gcc.gnu.org/g:1e3e6c700f04fe6992b9077541e434172c1cbdae commit r11-5114-g1e3e6c700f04fe6992b9077541e434172c1cbdae Author: Jonathan Wakely <jwakely@redhat.com> Date: Tue Nov 17 16:13:02 2020 +0000 libstdc++: Revert changes for SYS_clock_gettime64 [PR 93421] As discussed in the PR, it's incredibly unlikely that a system that needs to use the SYS_clock_gettime syscall (e.g. glibc 2.16 or older) is going to define the SYS_clock_gettime64 macro. Ancient systems that need to use the syscall aren't going to have time64 support. This reverts the recent changes to try and make clock_gettime syscalls be compatible with systems that have been updated for time64 (those changes were wrong anyway as they misspelled the SYS_clock_gettime64 macro). The changes for futex syscalls are retained, because we still use them on modern systems that might be using time64. To ensure that the clock_gettime syscalls are safe, configure will fail if SYS_clock_gettime is needed, and SYS_clock_gettime64 is also defined (but to a distinct value from SYS_clock_gettime), and the tv_sec member of timespec is larger than long. This means we will be unable to build on a hypothetical system where we need the time32 version of SYS_clock_gettime but where userspace is using a time64 struct timespec. In the unlikely event that this failure is triggered on any real systems, we can fix it later. But we probably won't need to. libstdc++-v3/ChangeLog: PR libstdc++/93421 * acinclude.m4 (GLIBCXX_ENABLE_LIBSTDCXX_TIME): Fail if struct timespec isn't compatible with SYS_clock_gettime. * configure: Regenerate. * src/c++11/chrono.cc: Revert changes for time64 compatibility. Add static_assert instead. * src/c++11/futex.cc (_M_futex_wait_until_steady): Assume SYS_clock_gettime can use struct timespec.
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>: https://gcc.gnu.org/g:b108faa9400e13a3d00dd7f71cff0ac45e29c5c9 commit r11-5167-gb108faa9400e13a3d00dd7f71cff0ac45e29c5c9 Author: Jonathan Wakely <jwakely@redhat.com> Date: Thu Nov 19 13:33:11 2020 +0000 libstdc++: Fix overflow checks to use the correct "time_t" [PR 93456] I recently added overflow checks to src/c++11/futex.cc for PR 93456, but then changed the type of the timespec for PR 93421. This meant the overflow checks were no longer using the right range, because the variable being written to might be smaller than time_t. This introduces new typedef that corresponds to the tv_sec member of the struct being passed to the syscall, and uses that typedef in the range checks. libstdc++-v3/ChangeLog: PR libstdc++/93421 PR libstdc++/93456 * src/c++11/futex.cc (syscall_time_t): New typedef for the type of the syscall_timespec::tv_sec member. (relative_timespec, _M_futex_wait_until) (_M_futex_wait_until_steady): Use syscall_time_t in overflow checks, not time_t.