Skip to content

馃悰 fix(soft): keep live holders when owner identity differs - #836

Merged
gaborbernat merged 13 commits into
mainfrom
fix/soft-owner-identity
Oct 17, 2026
Merged

gaborbernat merged 13 commits into
mainfrom
fix/soft-owner-identity

Conversation

@gaborbernat

@gaborbernat gaborbernat commented Oct 16, 2026 •

Copy link
Copy Markdown
Member

SoftFileLock, MarkerSoftFileLock and SoftFileLease decide whether a lock is free by reading the marker (the file whose presence says a process holds the lock). A contender removes the marker when its record proves the owner gone. Four separate bugs made that proof wrong for a live owner, so a second process acquired beside it and neither got an error. 馃悰 The rules are described under stale lock detection. The fix protects a marker from being rewritten by a process of a different account. It cannot protect a marker whose name that account can replace, which a directory without the sticky bit (a directory flag that lets only a file's owner remove it) allows. This resolves GHSA-rfpc-qcw5-3pc4.

A process name forged an NSpid line

When a marker names a dead PID under a bare hostname, the contender looks for the owner in another PID namespace. Since #815 it reads the NSpid line of /proc/<pid>/status for every task with the recorded start token. It read the file in text mode and split it with str.splitlines(). A task chooses its own name, and proc_pid_status(5) prints it raw except for \n and \\. A name such as "\rNSpid:\tx" therefore produced a line of its own, which matched before the real one. int("x") then raised ValueError out of SoftFileLease.acquire(), StrictSoftFileLock.acquire() and lock_path_occupant; a numeric forgery kept a dead owner's marker in place. _innermost_pid now reads bytes and splits only at \n, the one line end the kernel writes.

flowchart LR
    A["Name:\t\rNSpid:\tx"] --> B{split at}
    B -- "str.splitlines()" --> C["forged NSpid line wins"]
    B -- "b'\n' only" --> D["real NSpid line"]
    classDef bad fill:#ffd6d6,stroke:#c1121f,color:#5c0000;
    classDef ok fill:#d8f3dc,stroke:#2d6a4f,color:#1b4332;
    classDef info fill:#cfe4ff,stroke:#1f6feb,color:#0b1f3a;
    class A,B info;
    class C bad;
    class D ok;
Loading

One side could not read the boot id

Since #661 the Linux start token (the value that tells one use of a PID from a later one) folds the boot id into its high 64 bits. It uses 0 when /proc/sys/kernel/random/boot_id cannot be read. systemd's ProcSubset=pid hides /proc/sys from one unit while /proc/<pid>/stat stays readable. That unit and a job beside it then computed different tokens for the same live PID, and owner_is_stale read the mismatch as a recycled PID. A token without a boot id now matches on starttime alone, through _same_start, both for a live PID and in the other-namespace scan. A different starttime, or two different non-zero boot ids, still reads as a recycled PID. A PID reused after a reboot at the same tick now reads as held, which fails closed.

sequenceDiagram
    participant H as holder (boot id hidden)
    participant C as contender (boot id read)
    H->>H: token = 0 << 64 | starttime
    C->>C: token = boot << 64 | starttime
    rect rgb(255, 214, 214)
    Note over C: before: tokens differ, live holder judged recycled
    end
    rect rgb(216, 243, 220)
    Note over C: now: one side lacks the boot id, compare starttime only
    end
Loading

Time namespaces shifted starttime

Since Linux 5.6 the kernel adds the reader's boottime offset to field 22 of /proc/<pid>/stat. time_namespaces(7) describes the offset as the shift a time namespace applies to the boot clock. A holder and a contender in one PID namespace under one hostname, but in time namespaces with different offsets, read two values for one live task. The token comparison from #660 then broke the live holder's marker. process_start_token now returns None when _starttime_unshifted finds a boottime offset in /proc/self/timens_offsets. Such a process records and reads no token and falls back to PID liveness, which fails closed. The offset is read rather than the namespace identity because container runtimes may give every container its own time namespace without an offset; those keep their tokens. The check runs on each read because setns can move a process. A missing /proc/self/ns/time means the kernel has no time namespaces, so tokens are read. Any other failure to read it, such as a denied stat under a restricted /proc, leaves the namespace unknown, so no token is recorded or compared.

flowchart TD
    A[read /proc/self/ns/time] -->|initial namespace or no file| T[token from starttime]
    A -->|unreadable| N
    A -->|other namespace| B{boottime offset 0 and namespace is this process's own?}
    B -->|yes| T
    B -->|no| N[no token: PID liveness only]
    classDef ok fill:#d8f3dc,stroke:#2d6a4f,color:#1b4332;
    classDef info fill:#cfe4ff,stroke:#1f6feb,color:#0b1f3a;
    class A,B info;
    class T,N ok;
Loading

Another account could write the marker

A contender decides from the record inside a soft marker whether its owner is gone. The marker could be created group- or world-writable: an explicit mode such as 0o666 is applied with fchmod since #807, and the default mode follows the umask. A process of a different account that could write the marker but not remove it, for example in a sticky directory, could change the record. A contender of the holder's account then removed the live marker. Contenders only read and rename a marker, so the soft locks now clear the group and other write bits from both the create mode and the explicit mode, as File permissions and mode now states.

sequenceDiagram
    participant H as holder
    participant O as process of another account
    participant C as contender of the holder's account
    H->>H: create marker 0o666
    O->>H: rewrite record to a dead PID
    rect rgb(255, 214, 214)
    C->>C: owner gone, remove marker, acquire beside H
    end
    rect rgb(216, 243, 220)
    Note over H,O: now the marker is 0o644, the rewrite is refused
    end
Loading

A denied lock open on a loaded machine

WindowsFileLock opens its lock file through NtCreateFile, and NTFS answers STATUS_ACCESS_DENIED for a moment while another thread's unlink tears the name down. Since #604 the open retried that status for 0.5 s of wall-clock time and then raised PermissionError. On a loaded machine a process stalled for most of those 0.5 s made only a few probes, so twenty threads contending for one lock raised the error although nothing denied them. The open now makes 250 probes 2 ms apart, so a stall stretches the wait instead of cutting the probes short. A denial that outlasts the probes still raises before the caller's timeout.

sequenceDiagram
    participant A as releasing thread
    participant B as acquiring thread
    A->>A: close, unlink the lock file
    B->>B: NtCreateFile: STATUS_ACCESS_DENIED
    rect rgb(255, 214, 214)
    Note over B: before: 0.5 s pass with 3 probes under load, PermissionError
    end
    rect rgb(216, 243, 220)
    Note over B: now: 250 probes, the name settles, open succeeds
    end
Loading

Behavior changes

SoftFileLock(mode=0o666) now yields a -rw-r--r-- marker. A process in a time namespace with a boottime offset no longer tells a recycled PID from the owner, so a dead owner whose PID was reused keeps its marker until timeout, lifetime or break_lock().

@gaborbernat
gaborbernat force-pushed the fix/soft-owner-identity branch from 8fd45de to ba67ac8 Compare October 16, 2026 01:20
@gaborbernat gaborbernat changed the title fix(soft): keep live holders when owner identity differs by reader 馃悰 fix(soft): keep live holders when owner identity differs Oct 16, 2026
@gaborbernat
gaborbernat force-pushed the fix/soft-owner-identity branch 2 times, most recently from 8130fc6 to d5e4069 Compare October 16, 2026 02:36
@gaborbernat
gaborbernat force-pushed the fix/soft-owner-identity branch from d5e4069 to faf4a11 Compare October 16, 2026 04:57
gaborbernat added a commit that referenced this pull request Oct 16, 2026
On a filesystem without flock, UnixFileLock falls back to a soft lock and
waits on the marker the holder left. Soft markers are losing their group and
other write bits (#836), so a marker left by a process of a different account
is readable but not writable, and the O_RDWR open that precedes the flock
probe raised PermissionError where 4.2.0 waited for the lock.

Retry such an open read-only: reading the marker is all that waiting on it
takes. Where flock works the lock writes its file, so the denial still
stands there.
gaborbernat added a commit that referenced this pull request Oct 16, 2026
On a filesystem without flock, UnixFileLock falls back to a soft lock and
waits on the marker the holder left. Soft markers are losing their group and
other write bits (#836), so a marker left by a process of a different account
is readable but not writable, and the O_RDWR open that precedes the flock
probe raised PermissionError where 4.2.0 waited for the lock.

Retry such an open read-only: reading the marker is all that waiting on it
takes. Where flock works the lock writes its file, so the denial still
stands there.
@gaborbernat gaborbernat added the security Fixes a published or draft security advisory label Oct 16, 2026
A task chooses its own name, and the kernel prints it in the Name line of
/proc/<pid>/status with carriage returns, vertical tabs and other characters
left raw. Reading the file in text mode and splitting with str.splitlines()
broke the Name line at those characters, so a process whose name looked like
an NSpid line was parsed as one. The stale check then raised ValueError out of
SoftFileLease.acquire(), StrictSoftFileLock.acquire() and lock_path_occupant,
or kept a dead owner's marker in place.

Read the file as bytes and split only at b"\n", the only line end the kernel
writes, as process_start_token already does for /proc/<pid>/stat.
The Linux start token folds the boot id into its high bits and uses 0 when the
process cannot read /proc/sys/kernel/random/boot_id. That read can fail for
one process and succeed for another on the same host: systemd's
ProcSubset=pid hides /proc/sys from one unit while /proc/<pid>/stat stays
readable. The two processes then computed different tokens for the same live
PID, the stale check took the mismatch for a recycled PID, and a contender
removed a live holder's marker and acquired beside it.

A token without a boot id now matches on starttime alone, both in the
liveness check and in the scan for an owner in another PID namespace. A
recycled PID with a different starttime, or a token from another boot when
both sides have one, still reads as stale.
Since Linux 5.6 the kernel shifts the starttime field of /proc/<pid>/stat by
the boottime offset of the reading process's time namespace. A holder and a
contender in time namespaces with different offsets, but in one PID namespace
under one hostname, read different start tokens for the same live process, and
the contender broke the holder's marker as if its PID had been recycled.

A process whose time namespace has a boottime offset now records no start
token and reads none, falling back to PID liveness, which fails closed. The
offset is read from /proc/self/timens_offsets, because container runtimes may
put every container in its own time namespace without an offset, and those
keep their tokens. The cost is that a process with an offset no longer tells a
recycled PID from the owner.
A contender decides from the record inside a soft marker whether its owner is
gone, but the marker could be created group- or world-writable: an explicit
mode such as 0o666 is applied with fchmod, and the default mode follows the
umask. A process that could write the marker but not remove it could then
change the record, and a contender would remove the live holder's marker and
acquire beside it.

Contenders only read and rename a marker, so the soft locks now clear the
group and other write bits from both the create mode and the explicit mode.
The time-namespace pin, the boot id fixture and the other-namespace fixture
only run on Linux, so macOS and Windows reported their bodies as uncovered.
Three tests reclaimed a dead owner's or a malformed marker under a one
second lock timeout, which stood in for the test's budget. The reclaim takes
two polls and a third to acquire, and a loaded free-threaded Windows runner
spent most of that second on two polls, so the lock gave up before the
reclaim although nothing was wrong with it.

The tests now poll non-blocking, once per call, and require the lock within
three polls: a step count instead of a wall-clock budget, which a stalled
runner cannot miss.
The test asserted that a dropped lease's marker is gone right after
gc.collect(). PyPy only queues the finalizer the collection found and runs it
at a later bytecode boundary, in whichever thread reaches one first, so on a
loaded runner the assertion ran before the release. Waiting for the marker to
go, within the bound the lease tests already use, keeps the assertion on the
effect without depending on when the collector runs __del__.
Shorter sentences, one idea each, and the boottime offset defined where it
first appears, so the changelog, the concepts page and the code comments read
without prior knowledge of Linux namespaces.
@gaborbernat
gaborbernat force-pushed the fix/soft-owner-identity branch from d19532f to 97e5859 Compare October 16, 2026 06:29
A lock open that NtCreateFile answered with STATUS_ACCESS_DENIED retried for
0.5 s of wall-clock time before raising PermissionError. NTFS gives that
status for a moment while a peer's unlink tears the name down, and on a
loaded machine a process stalled for most of those 0.5 s made only a few
probes, so twenty threads contending for one WindowsFileLock raised the
error although nothing denied them.

The open now makes 250 probes 2 ms apart, so a stall stretches the wait
instead of cutting the probes short. A denial that outlasts the probes still
raises before the caller's timeout.
The write-exclusion test released the holder one second after starting a
contender with a 0.5 s timeout and then terminated the contender. On a
loaded free-threaded Windows runner the contender's interpreter took longer
than that second to start, so it acquired the lock after the release and was
terminated inside the acquired event's lock. The event's semaphore stayed
locked, and the test hung in clear() until pytest-timeout killed it.

The test now joins the contender, which exits on its own timeout, before it
releases the holder, and asserts the event was never set. The successor then
reuses an event nothing has touched.
Only a missing /proc/self/ns/time proves the kernel has no time
namespaces. Any other failure, such as a denied stat under a
restricted /proc, leaves the namespace unknown, so start times are no
longer compared and the marker is kept.
@gaborbernat
gaborbernat merged commit eeceaa8 into main Oct 17, 2026
47 checks passed
@gaborbernat
gaborbernat deleted the fix/soft-owner-identity branch October 17, 2026 09:21
gaborbernat added a commit that referenced this pull request Oct 17, 2026
Replaying #843 over main dropped path handling from its merge with #836.
Restore the relative-path and marker cleanup behavior, and cover a failed
released-record write when another marker takes the path.
gaborbernat added a commit that referenced this pull request Oct 17, 2026
On a filesystem without flock, UnixFileLock falls back to a soft lock and
waits on the marker the holder left. Soft markers are losing their group and
other write bits (#836), so a marker left by a process of a different account
is readable but not writable, and the O_RDWR open that precedes the flock
probe raised PermissionError where 4.2.0 waited for the lock.

Retry such an open read-only: reading the marker is all that waiting on it
takes. Where flock works the lock writes its file, so the denial still
stands there.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug security Fixes a published or draft security advisory

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant