Conversation
jserv
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
thread_exec_de_thread gave up on its siblings after 1000 ms with no departure. Departures are not a liveness signal: once one sibling is left there are none to observe, so the interval measures that thread's own latency, which is the fixed bound it replaced. Sizing it honestly means covering the slowest re-check quantum a parked sibling owes (200 ms, in the io wait) times how far behind schedule the host runs, and that multiplier is not a property this code can know. An ASAN make check on a machine running the sanitizer lanes of several workflow runs at once reported one sibling still inside mmap after 1005 ms, last departure at 4 ms, and sys_execve took its post-PNR fatal exit. The join below it, 100 iterations of usleep(5000) and nominally 500 ms, took 9 s in the same run. The ceiling is the only bound, which keeps the budget in the loop that can spend it: this one re-issues the whole wake set every iteration and the join only polls.
The warning set in mk/config.mk is strong and every diagnostic only printed. The gates around it, syscall coverage and the EINTR contract and the proof targets, all fail the build instead, so a compiler warning was the one signal a reader had to notice unaided. WERROR=0 turns it off, which is what a newer compiler with a new warning wants: the flag must not be the reason a fresh clone stops building. tests/test-casefold-walk-host.c indexed a string literal by a size_t walk offset twice, which -Wstring-plus-int reads as an attempt to concatenate. The arithmetic is right and the spelling is not, so the literal takes a name and the offsets index an array.
This process parses input the guest fully controls: its ELF, every syscall argument, FUSE frames, netlink messages, sockaddr and cmsg blobs. The bounds math for those is proved in src/proved/, and the proofs cover the arithmetic rather than the call sites consuming it. PIE is already the Darwin default; -fstack-protector-strong and _FORTIFY_SOURCE are not. _FORTIFY_SOURCE is skipped under AddressSanitizer alone, which predefines it to 0 on purpose since its interceptors do the same job, so redefining it is a -Wmacro-redefined error under the -Werror above. UBSAN and TSAN predefine nothing and keep it, which is what makes those lanes exercise the same libc entry points the shipped binary calls. Each -fsanitize= argument is split on commas and matched by name. A substring test for -fsanitize=address is order-dependent while reading as if it were not: it answers correctly for -fsanitize=address,undefined and wrongly for -fsanitize=undefined,address, the same request spelled the other way round.
The block at the top of src/syscall/internal.h documented an 11-entry acquisition order and presented it as the ordering. Two of those entries name a per-instance lock by role, so it covered 9 of the tree's 31 file-scope locks, and seven of the twenty-two it left out are held across another acquisition: autoreap_lock over pid_lock and pidfd_lock, elf_path_lock over cwd_lock and sysroot_lock, oom_write_lock over fd_lock, fuse_lock over the per-session lock, proc_tmpdir_lock over mmap_lock, pty_keepalive_lock over fd_lock, and cwd_lock over fuse_lock. Each is correct as written, and a reader adding the next lock had no rule to consult. The FUSE per-session lock reaches fd_lock and sig_lock through asyncio_fire, so the pairing with fuse_lock is not the whole of its ordering. log_mutex sits below the entire order rather than beside the other leaves, since any lock may log while held. sig_lock is not a leaf: signal_queue_thread_common takes thread_lock under it. A leaf list closes the set, so the two lists together are exhaustive: a new lock belongs in one of them, and a leaf that grows a call to another locker moves up. The numbered "Lock order: N" comments at the definitions are not indices into this list; a number there records only which locks that one was known to precede when it was written.
The lock-order block in src/syscall/internal.h claims to name every lock, and prose cannot hold that claim on its own. An inversion is not the kind of defect the suite finds: it needs the two threads to interleave the one way that deadlocks, on a machine loaded enough to make that likely, which describes CI and not a developer's laptop. scripts/check-lock-order.py is a set comparison in both directions: every file-scope pthread_mutex_t and pthread_rwlock_t under src/ is named by the block, and every lock the block names still exists. Headers are scanned too, because a static definition in one compiles into every translation unit that includes it. The declaration is captured whole and taken apart afterwards, because no single pattern reads initializers correctly: a clause that stops at the first comma drops the declarators behind it, one that runs to the semicolon swallows them, and one that refuses braces skips a brace initializer outright. The list splits at commas outside brackets and each declarator drops its own initializer, so an array bound, a brace initializer, a parameter list, and a declarator standing behind an initializer all parse. Every one of those failures is a silent miss, which is the one shape worse than no gate. Which of the two lists a lock belongs in stays a human judgement. That question needs the call graph, and a branch-insensitive answer is wrong in both directions: a linear scan of fuse.c drops fuse_lock at an early-return unlock and files a documented non-leaf as a leaf, while the same scan reads two comments mentioning sys_close as an sfd_lock/fd_lock cycle. A gate that cries wolf earns an exemption list and then gets ignored.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by cubic
Removes the 1s “no-departure” stall from threaded exec de-threading and bounds the wait by a single 10s wall-clock ceiling to prevent false fatal exits under load; the loop re-pokes siblings each iteration. Completes and enforces the lock-order document and hardens the default build so CI fails on undocumented locks and compiler warnings.
scripts/check-lock-order.pytomake checkwith self-tests; fails if any file-scopepthread_mutex_t/pthread_rwlock_tundersrc/is missing from the lock-order block, if the document names a non-existent lock, or if the same lock name is defined in more than one file. Updatesdocs/testing.mdandmk/tests.mk.src/syscall/internal.h: names all file-scope locks, adds an explicit leaf list, placeslog_mutexbelow the entire order, usesexec_handoff_lock, and fixes several positions.mk/config.mk(WERROR=1), enables-fstack-protector-strong, and sets_FORTIFY_SOURCE=2except under AddressSanitizer (sanitizers parsed by exact name). Fixestests/test-casefold-walk-host.cto avoid-Wstring-plus-int.Rollout/Migration
pthread_mutex_t/pthread_rwlock_t(including in headers), add it to the ordered or leaf list insrc/syscall/internal.h; otherwisemake checkfails.WERROR=0; CI keepsWERROR=1.Written for commit ee41153. Summary will update on new commits.