Keep Linux update extraction from writing through symlinks #1327

Merged

bronze-ff merged 2 commits from fix/linux-update-zip-symlinks into main

2026-11-09 23:11:55 +00:00

bronze-ff commented

2026-11-09 21:48:28 +00:00

(Migrated from github.com)

Keep the symlinks themselves. The v2.0.11 sysroot contains a legitimate absolute libmvec.so link, and the bundled Python and compiler use relative links.

Refs #1295 (Linux ZIP extraction).

Validation

On upstream main 7dbdcdc, the new regression checks reproduced writes through a directory link, a file link, and a destination linked by an earlier part. They pass with this patch.
On Ubuntu under WSL, python3 -I -B tests/run_all.py --strict linux_update passed.
Installed all six official v2.0.11 parts from their SHA-256-verified ZIPs into a temporary builder home, then tested reuse of all six parts without downloads. Both installs preserved the sysroot link and the executable launcher. No game files were used.

Staging directory

Thanks to @monsterlag for identifying the dangling staging-link case in #1343. Part placement resolves only the staging folder's parent, then checks the whole destination. A temporary-directory test confirms that a dangling staging link stops before download and that an installation with a symlinked parent still works. The official v2.0.11 part install and reuse checks still pass. The two PRs merge together cleanly. #1343 adds the earlier installer check before cleanup or part placement.

The Linux builder's ZIP extractor checks archive member names but can still write through a symlink placed in the staging tree. Validate the destination and parent directories before extracting, and create regular files exclusively so an earlier entry cannot redirect or be overwritten by a later one. Keep the symlinks themselves. The v2.0.11 sysroot contains a legitimate absolute `libmvec.so` link, and the bundled Python and compiler use relative links. Refs #1295 (Linux ZIP extraction). ### Validation - On upstream main `7dbdcdc`, the new regression checks reproduced writes through a directory link, a file link, and a destination linked by an earlier part. They pass with this patch. - On Ubuntu under WSL, `python3 -I -B tests/run_all.py --strict linux_update` passed. - Installed all six official v2.0.11 parts from their SHA-256-verified ZIPs into a temporary builder home, then tested reuse of all six parts without downloads. Both installs preserved the sysroot link and the executable launcher. No game files were used. ### Staging directory Thanks to @monsterlag for identifying the dangling staging-link case in #1343. Part placement resolves only the staging folder's parent, then checks the whole destination. A temporary-directory test confirms that a dangling staging link stops before download and that an installation with a symlinked parent still works. The official v2.0.11 part install and reuse checks still pass. The two PRs merge together cleanly. #1343 adds the earlier installer check before cleanup or part placement.

hckrman101 (Migrated from github.com) approved these changes

2026-11-09 23:08:00 +00:00

hckrman101 (Migrated from github.com) left a comment

Looks good. I walked through the extraction paths and the protection holds up for what a ZIP can actually do:

Symlink dir entry followed by link/x: the per-entry realpath(parent) != abspath(parent) check catches it, including a link to . or one that points back inside the tree.
Symlink entry followed by a regular file with the same name (link, ./link): open(..., "xb") is O_EXCL, so it fails even on a dangling link, and os.symlink already refuses to replace an existing entry. Duplicate members are covered too.
Absolute and .. member names were already rejected by _safe_name, which is now also applied to part.dest. Absolute symlink targets are still allowed, as they need to be for libmvec.so.
Hardlinks: ZIP has no hardlink entries, so nothing to do on the extraction side.

On #1343: the overlap is small. With this PR the dangling and live .part links are both rejected in _place (the realpath of the parent only, then the full destination check), so #1343 is no longer needed for safety. What it still adds is failing earlier with a clear message, before shutil.rmtree (which otherwise raises its own "Cannot call rmtree on a symbolic link") and before makedirs. They touch different hunks and GitHub reports this one as cleanly mergeable, so I'd be fine with both landing, in either order.

Residual things, none blocking:

The check-then-open is not atomic (TOCTOU), so something else in the same user's builder home that swaps a directory for a link between the check and the write could still redirect it. xb closes the leaf, the parent isn't. That needs a concurrent local writer in the staging tree, so I think it's fine to leave; worth a one-line comment if you want to be explicit.
Nit: _finish_tree (copyfile to AppRun, icon, desktop file) and the LIST_NAME write in install still follow a link at their destination. A part would have to ship e.g. AppRun as a link at the root of the staging folder, and parts are SHA-checked against the manifest, so this is theoretical. Opening those with xb/lstat first would close it, no need to do it here.
Nit: the reuse path (_clone/_clone_tree from an old version) doesn't _safe_name the part.files names. Those come from the locally stored list, so low risk.

Tests are registered via the existing tests/test_linux_update.py entry in run_all.py, the three new tests exercise the real failure modes (and the legit sysroot links), and CI is green.

Looks good. I walked through the extraction paths and the protection holds up for what a ZIP can actually do: - Symlink dir entry followed by `link/x`: the per-entry `realpath(parent) != abspath(parent)` check catches it, including a link to `.` or one that points back inside the tree. - Symlink entry followed by a regular file with the same name (`link`, `./link`): `open(..., "xb")` is O_EXCL, so it fails even on a dangling link, and `os.symlink` already refuses to replace an existing entry. Duplicate members are covered too. - Absolute and `..` member names were already rejected by `_safe_name`, which is now also applied to `part.dest`. Absolute symlink *targets* are still allowed, as they need to be for `libmvec.so`. - Hardlinks: ZIP has no hardlink entries, so nothing to do on the extraction side. On #1343: the overlap is small. With this PR the dangling and live `.part` links are both rejected in `_place` (the realpath of the parent only, then the full destination check), so #1343 is no longer needed for safety. What it still adds is failing earlier with a clear message, before `shutil.rmtree` (which otherwise raises its own "Cannot call rmtree on a symbolic link") and before `makedirs`. They touch different hunks and GitHub reports this one as cleanly mergeable, so I'd be fine with both landing, in either order. Residual things, none blocking: - The check-then-open is not atomic (TOCTOU), so something else in the same user's builder home that swaps a directory for a link between the check and the write could still redirect it. `xb` closes the leaf, the parent isn't. That needs a concurrent local writer in the staging tree, so I think it's fine to leave; worth a one-line comment if you want to be explicit. - Nit: `_finish_tree` (`copyfile` to `AppRun`, icon, desktop file) and the `LIST_NAME` write in `install` still follow a link at their destination. A part would have to ship e.g. `AppRun` as a link at the root of the staging folder, and parts are SHA-checked against the manifest, so this is theoretical. Opening those with `xb`/`lstat` first would close it, no need to do it here. - Nit: the reuse path (`_clone`/`_clone_tree` from an old version) doesn't `_safe_name` the `part.files` names. Those come from the locally stored list, so low risk. Tests are registered via the existing `tests/test_linux_update.py` entry in `run_all.py`, the three new tests exercise the real failure modes (and the legit sysroot links), and CI is green.