Skip to content

🐛 fix(read-write): keep SQLite off shared names, bound schema - #834

Merged
gaborbernat merged 20 commits into
mainfrom
fix/rw-sqlite-files
Oct 17, 2026
Merged

gaborbernat merged 20 commits into
mainfrom
fix/rw-sqlite-files

Conversation

@gaborbernat

@gaborbernat gaborbernat commented Oct 16, 2026 •

Copy link
Copy Markdown
Member

ReadWriteLock keeps its lock in a SQLite database and relied on names it did not control. It handed SQLite /proc/self/fd/<n>, a path SQLite turns back into a plain filename. It let SQLite use -journal, -wal and -shm files found beside that name. It removed its .filelock-* link directories by name with shutil.rmtree. It also ran statements that load the database's schema, and nothing bounded that work.

🔒 In a lock directory that processes of other accounts can write, a file placed there could redirect a write, delete a file a journal names, admit a reader beside a writer, fail every acquire, or make a release delete a directory renamed onto the link directory's name. A crafted database at the lock path kept an acquire running past its timeout. On POSIX this PR connects SQLite only through names no other process can change, removes link directories through a checked descriptor, and refuses a database whose schema is larger than a lock needs. This resolves GHSA-v4x8-3wr5-3wq8, GHSA-jqc4-5cvw-7w85 and GHSA-cwqr-hh33-gh44.

Schema loading ignored the acquire timeout

Every acquire runs PRAGMA journal_mode=MEMORY; a read lock also runs a SELECT on sqlite_master. Both make SQLite load the schema (the table definitions) of the file at the lock path. The caller's timeout reaches SQLite only as busy_timeout, and bounds only the wait for another connection's lock. Two file layouts defeat this. In the first, every sqlite_master b-tree page points at one child page, so each level multiplies the rows to scan by 661. In the second, a schema row continues on an overflow page that names itself as the next page, so one page of disk reads as a gigabyte of text. Both are legal under the file format, and SQLite rejects them, if at all, after walking the tree. acquire_read and acquire_write stayed inside that one statement for as long as the file's author chose, or forever. blocking=False and timeout=0 made no difference. This has been reachable since ReadWriteLock arrived in #399.

The acquire now caps the string length SQLite materializes at 4 KiB and installs a progress handler while its statements run. The handler aborts the statement after 10,000 virtual machine steps. Waiting for a lock runs no such steps, so the budget bounds schema loading and never shortens a wait. The length limit covers the overflow layout, which the progress handler cannot interrupt during a copy. A database that hits either cap raises ValueError, worded like the WAL refusal from #805: the file is not a lock database of its own. A schema of thousands of tables still loads within the budget. ✨

flowchart LR
    A[acquire] --> B[PRAGMA journal_mode=MEMORY<br/>BEGIN / SELECT]
    B --> C{schema loads<br/>within budget?}
    C -- "empty or small schema" --> D[lock taken<br/>or Timeout]
    C -- "string over 4 KiB" --> E[ValueError]
    C -- "step budget spent" --> E
    classDef ok fill:#d8f3dc,stroke:#2d6a4f,color:#1b4332;
    classDef bad fill:#ffd6d6,stroke:#c1121f,color:#5c0000;
    classDef info fill:#cfe4ff,stroke:#1f6feb,color:#0b1f3a;
    class A,B info;
    class D ok;
    class E bad;
Loading

SQLite opened names beside the database that any process could create

Since the symlink fix in 4.0.9 (4d45e83), the lock checked its database through a file descriptor (an open handle on the file) and connected SQLite to /proc/self/fd/<n>. The intent was that the kernel reopens the checked inode (its identity for a file, independent of the file's name). SQLite does not open that name. It reads the link, which proc(5) resolves to the path used to open the descriptor, and opens that path by name, following symlinks. After an unlink of the name, the path reads <name> (deleted). A process that can create entries in the directory can make that name a symlink to a file of its choice. Linking the same inode back under the original name kept the replacement check satisfied.

SQLite also derives the -journal, -wal and -shm names from the real path. A rollback journal (SQLite's record of a transaction to undo) beside a non-empty database counts as unfinished, so the pragma replayed it and then unlinked the super-journal file its tail names. A -wal (write-ahead log) recreated between the pragma and the BEGIN put the holder in WAL mode, which admits a reader beside a writer; How To Corrupt An SQLite Database File warns about side files SQLite cannot trust. In a sticky directory (one where only a file's owner may remove it), a -wal beside an empty database that the lock's process cannot unlink failed every acquire with sqlite3.OperationalError: disk I/O error. The journal cases predate 4.0.9 and go back to #399.

The connect now decides by the database directory's mode. When the directory belongs to the locking account or root and has no group or other write bit, no other process can create or rename names there, so SQLite gets the database path itself. Otherwise it connects through a hard link (a second name for the same file) inside a subdirectory with a random name under a private .filelock-* directory, the route #820 introduced for macOS. The lock creates the subdirectory through a descriptor of the .filelock-* directory, after an fstat confirms it is the directory just made and readable by nobody else. A directory another process swaps in under the .filelock-* name cannot contain that subdirectory, so SQLite fails to open rather than follow the swap, and every side file SQLite looks for lives where only the locking account can list. The descriptor-path fallback is gone: a process that can create names in the directory can also make the link fail.

SQLite opens the file the name holds at connect time, and its first statement replays a journal beside that file, so the comparison of the name with the checked file now runs before any statement as well as after the BEGIN. The acquire refuses a replacement swapped in before the connect with OSError; its journal and WAL stay, the file the journal names stays, and SQLite's open adds at most an empty lock.db where the name was missing.

Use shared read / exclusive write locks documents the trade-off: a lock in a directory writable beyond its owner needs that directory writable by the locking account and a filesystem with hard links. Both routes hold only while no other account can replace the database directory or a directory above it, because a process that can rename an entry there owns every name below it. A swap between that comparison and SQLite's first statement still makes SQLite look up -journal and -wal under the replacement. It deletes both, and beside a non-empty lock database it first replays the journal into that database and unlinks the file the journal names. The how-to states this residual.

sequenceDiagram
    participant L as lock process
    participant K as kernel
    participant S as SQLite
    participant P as another process
    rect rgb(255, 214, 214)
        Note over L,P: before: the name decides what SQLite opens
        L->>K: open("lock.db", O_NOFOLLOW) → fd 3, checked regular
        L->>S: connect("/proc/self/fd/3")
        P->>K: unlink("lock.db")#59; link(kept, "lock.db")
        S->>K: readlink("/proc/self/fd/3") → "lock.db (deleted)"
        S->>K: open("lock.db (deleted)") follows the symlink P made
        S->>K: open("<target>-journal"), replay into target
    end
    rect rgb(216, 243, 220)
        Note over L,S: after: a name only this account can reach
        L->>K: open("lock.db", O_NOFOLLOW) → fd 3, checked regular
        L->>K: mkdtemp(".filelock-…") 0700, fstat, mkdir(<random>) via dir_fd, link(lock.db)
        L->>S: connect(".filelock-…/<random>/lock.db")
        S->>K: side files looked up under .filelock-…/<random>/
    end
Loading

Releasing or sweeping deleted the directory under a link directory's name

On the hard-link route (every connection on macOS since #820, and POSIX without /proc) the lock remembered its mkdtemp directory as a path. It ran shutil.rmtree on that path when the connection closed, when a connect failed, and in the dead-holder sweep from #815 for any .filelock-* name whose embedded PID is gone. Nothing tied the path to the directory the lock made. Renaming an entry needs write permission on the parent only, per rename(2). In a parent without the sticky bit, a process of a different account could rename the link directory aside during a hold and rename a sibling directory onto its name. The release then deleted that tree with the holder's permissions and reported success. The sweep needed no hold: a name built from an unused PID, an empty start field and the host digest marked any directory for removal at the next lock construction there.

Removal now goes through a descriptor. Removal opens the directory with O_DIRECTORY | O_NOFOLLOW, checks that its fstat identity matches the directory mkdtemp returned, unlinks the link names relative to that descriptor, and ends with rmdir, which refuses a non-empty directory. The sweep removes only hard links to the database the lock opens, compared by st_dev and st_ino, and leaves a directory whose name resembles a dead holder's. It runs once per database inode instead of once per parent, so a database recreated under the same name still sweeps its own leftovers. The sweep no longer collects a dead holder's link to a database that no longer exists; that costs one unused directory instead of a deleted tree. The sweep opens each .filelock-* entry through a descriptor first and reads the name only when the directory belongs to the locking account, so a name chosen by another process is never parsed and no process id is probed for it. It also removes a directory a holder left empty when it died between creating the directory and linking the database. The module no longer imports shutil.

sequenceDiagram
    participant H as lock holder
    participant F as filesystem
    participant P as another process
    H->>F: mkdtemp(".filelock-A") , link lock.db inside
    H->>F: write lock held
    P->>F: rename(".filelock-A", "aside")
    P->>F: rename("project", ".filelock-A")
    rect rgb(255, 214, 214)
        Note over H,F: before
        H->>F: release → shutil.rmtree(".filelock-A") deletes project/
    end
    rect rgb(216, 243, 220)
        Note over H,F: after
        H->>F: release → open(".filelock-A", O_DIRECTORY|O_NOFOLLOW), fstat
        F-->>H: st_ino differs from the directory made
        H->>F: leave it#59; rmdir only an empty directory of its own
    end
Loading

Behavior changes

A lock whose database directory is writable beyond its owner now needs that directory writable by the locking account and a filesystem with hard links. Where the link cannot be made, the connect raises OSError, as it already did on macOS, instead of falling back to a descriptor path. In an owner-only directory, Linux and macOS connect by the database name and create no .filelock-* directory. A database whose schema is larger than a lock needs raises ValueError at its first acquire. The protection against processes of other accounts ends once one of them can replace the database directory or a directory above it; the acquire then raises OSError before running any statement rather than report the lock held, with the one residual the how-to states.

Windows is unchanged: the lock connects by the database name, and the how-to now says which directories to use.

gaborbernat added a commit that referenced this pull request Oct 16, 2026
@gaborbernat gaborbernat changed the title fix(read-write): keep SQLite off shared names and bound schema loading 🐛 fix(read-write): keep SQLite off shared names, bound schema Oct 16, 2026
gaborbernat added a commit that referenced this pull request Oct 16, 2026
gaborbernat added a commit that referenced this pull request Oct 16, 2026
gaborbernat added a commit that referenced this pull request Oct 16, 2026
gaborbernat added a commit that referenced this pull request Oct 16, 2026
gaborbernat added a commit that referenced this pull request Oct 16, 2026
@gaborbernat gaborbernat added the security Fixes a published or draft security advisory label Oct 16, 2026
PRAGMA journal_mode=MEMORY, and the read lock's SELECT on sqlite_master,
make SQLite load the schema of whatever file sits at the lock path. The
acquire timeout reaches SQLite only as busy_timeout, which bounds waits for
another connection and not that load. A database whose schema rows expand
through a looping overflow page, or whose pages point at the same child
many times over, kept an acquire running far past its timeout or forever.

Cap string length and virtual machine steps while the acquire's statements
run, and raise ValueError for a database that hits either cap, as for a
WAL-mode database.
SQLite opens the database by name and looks up -journal, -wal and -shm
files beside that name. Connecting through /proc/self/fd/<n> did not pin
the checked file: SQLite reads the link back into the path the kernel
reports, "<name> (deleted)" once the name is unlinked, and opens that path
following symlinks. In a directory processes of other accounts can write, a journal or WAL
file placed beside the database got rolled into another file, deleted a
file the journal named, put a writer in WAL mode so a reader got in beside
it, or failed every acquire.

Connect by the database name only when no process of another account can change names in
its directory. Elsewhere connect through a hard link in a randomly named
subdirectory of a private .filelock-* directory, so a directory swapped in
under the .filelock-* name cannot hold the link SQLite opens.

The .filelock-* directories were removed with shutil.rmtree by name, on
release and in the sweep of dead holders, so a directory renamed onto that
name was deleted with its contents. Remove entries only through a
descriptor checked to be the directory created, sweep only hard links to
the database being opened, and rmdir what is left empty.
The module skips where POSIX hard links are missing, so Windows and Termux
coverage counted its lines as missed.
os.geteuid does not exist on Windows, so the type check for that platform
rejected the helpers that called it. Read it in the branch of the connect
that only POSIX takes and hand it down.
Checking the database directory says nothing about its ancestors: anyone who
can rename an entry in one of them can swap the checked directory before
SQLite resolves the name, on the direct and the private-link route alike. No
path-based lock can prevent that, so document the precondition and pin what
the acquire does then: it refuses with OSError rather than report the lock
held.
The swap test showed the refusal but not what SQLite did to the replacement
before it: the acquire runs a pragma and a BEGIN on it. Both directories now
keep exactly their entries and bytes, so a journal, link directory or page
write on either would fail the test.
The account distinction only matters where the mechanism needs it; the
rest of the docs and comments talk about processes.
SQLite opens whatever the name holds at connect time, and its first
statement replays a journal found beside that file. The identity check ran
after the pragma and the BEGIN, so a directory swapped in under the lock's
path before the connect had its journal finished, and the file the journal
named unlinked, before the acquire refused. Compare the name with the file
checked before any statement runs; the check after the BEGIN stays for a
swap during the statements. A swap between the early check and SQLite's
first statement still has SQLite look up the side files under the
replacement; the how-to states exactly what happens then.
The ReadWriteLock tests live in test_read_write.py; the two modules added
for the shared-directory and crafted-database cases split them for no
reason, and the shared-directory module needed its own coverage omit.
One idea per sentence, each term defined where it first appears, and
comments that say why rather than what.
The sweep parsed every .filelock-* name and probed its PID before opening
the directory, so a name another account chose cost a /proc scan and a
symlink under a dead holder's name was followed. Open each entry through a
descriptor first, parse the name only when this account owns the directory,
and open nested entries without following symlinks. A directory a holder
left empty is now removed too, since rmdir refuses anything non-empty.
SQLite's open creates the database name where it is missing, so a refused replacement can gain an empty lock.db, also through a symlink.
The changelog, concepts page and docstring stated the protection without a platform. It applies on POSIX; on Windows the database directory must be one other accounts cannot write.
ty on win32 flags st_ctime as deprecated. Copy the stat fields by index instead.
@gaborbernat
gaborbernat merged commit 5e3f2ec into main Oct 17, 2026
44 checks passed
@gaborbernat
gaborbernat deleted the fix/rw-sqlite-files branch October 17, 2026 10:39
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