Repository navigation
Conversation
…call refuses
extract(dir, { glob }) went to the next entry when mkdir, open, write or
symlink failed for an entry, and resolved with a smaller count. Now a
refused directory or file stops the extraction, and the promise rejects
with the error of the failed call: code, syscall and path. A refused
symlink does not stop the other entries and rejects after the last one.
A file that cannot be opened for a write is replaced, so a later entry
or a second extract can follow a read-only file. On Windows a file is
no longer created read-only.
files(glob) gave the stored name of an entry to the glob, and
extract(dir, { glob }) the normalized name. match_glob_patterns now
takes a NormalizedName, which only the normalization constructs. A
leading "./" of a pattern is dropped, so both spellings select the same
entries. The keys of the Map stay the entry names.
|
Updated 6:50 PM PT - Oct 7th, 2026
❌ @robobun, your commit 158f2fa has 4 failures in
🧪 To try this PR locally: bunx bun-pr 44731That installs a local version of the PR into your bun-44731 --bun |
|
Status: ready for review. Three questions for a maintainer are at the top of the Notes in the PR body. How I reproduced it. Linux x64, release build of main (
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. WalkthroughArchive glob matching now uses normalized extraction-style paths while ChangesArchive glob and extraction behavior
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The glob matching and extraction-error changes appear mergeable after normal checks; no actionable risk remains from this review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/runtime/api/Archive.rs— Pre-existing: users who extract a truncated or corrupt archive with a glob still get a resolved promise with a smaller count, where extract(dir) rejects. At Archive.rs:1488-1491 a failed archive.read_data only sets write_success = false, so the partial file is unlinked and the loop moves on with no rejection. At Archive.rs:1397 a Fatal read_next_header just ends the while loop and Success(count) is returned. The docs and bun.d.ts added here say the glob form rejects when an entry cannot be written. Fix: map a negative read_data result and a Failed/Fatal read_next_header in the glob loop to ExtractResult::Err(ExtractError::ReadError), the same outcome extract_to_dir gives at src/libarchive/lib.rs:1439-1441 and :1887.Why this was flagged
An archive whose body is cut short is passed to archive.extract(dest, { glob: '**' }). For an entry whose data is missing, archive.read_data at Archive.rs:1487 returns a negative libarchive status; Archive.rs:1488-1491 sets write_success = false and breaks, Archive.rs:1524 unlinks the partial file, rejection is still None so no return happens, and the loop continues to the next header. When the next header is itself unreadable, read_next_header at Archive.rs:1397 returns Fatal, succeeded() is false, the loop ends and Archive.rs:1573 returns ExtractResult::Success(count). The caller gets a resolved promise with a count lower than the archive's entry count and no error. The base branch behaves the same way at these two sites, so this is pre-existing, but the non-glob path extract_to_dir returns Err(crate::Error::Fail) for both a Fatal header at src/libarchive/lib.rs:1439-1441 and a failed data read at src/libarchive/lib.rs:1887, so extract(dest) rejects for the same input. The new rejection variable at Archive.rs:1481 only covers bun_sys::write outcomes, not the libarchive read result.
Verification: Pre-existing: the base already continued on a failed read_data and returned Ok(count). Trigger: archive.extract(dest, { glob }) on a tarball truncated mid-entry. src/runtime/api/Archive.rs:1488-1491 sets write_success = false and no rejection, so the loop continues; line 1397 ends on Fatal and line 1573 returns Success(count). src/libarchive/lib.rs:1439-1441 and 1887 return Err(crate::Error::Fail).
| if let Err(err) = &opened | ||
| && err.get_errno() == bun_sys::E::EACCES | ||
| && bun_sys::unlinkat(dir_fd, pathname_z).is_ok() | ||
| { | ||
| opened = bun_sys::openat(dir_fd, pathname_z, flags, mode); | ||
| } |
There was a problem hiding this comment.
🔴 Users who run extract() with a glob into a directory holding a read-only file lose that file: it is deleted and overwritten, where extract(dir) rejects with EACCES. Archive.rs:1461-1466 answers any EACCES from openat with unlinkat and a second openat, so every pre-existing file without the owner-write bit under the destination is replaced, not only an entry this call wrote. The PR text lists this as a downside; the consequence is silent data loss with no rejection. Fix: make a second extract and a duplicate read-only entry work without deleting foreign files, e.g. create files with the owner-write bit as extract_to_dir does at src/libarchive/lib.rs:1685 (| 0o666) instead of the unlink-and-retry, or limit the unlink to a name this same call already created.
Why this was flagged
A destination directory that is writable contains a file r.txt with mode 0o444 that the user or another user placed there, and the archive holds an entry r.txt; the call comes through archive.extract(dest, { glob }) into extract_to_disk_filtered (src/runtime/api/Archive.rs:1351). openat with O_WRONLY|O_CREAT|O_TRUNC at Archive.rs:1459 fails with EACCES; Archive.rs:1461-1466 then calls bun_sys::unlinkat on the name and opens again, which deletes the existing read-only file and writes the archive content in its place, and the promise resolves with the entry counted. The base branch did Err(_) => continue at this site, so the read-only file kept its content and the entry was skipped; the non-glob path extract_to_dir at src/libarchive/lib.rs:1718-1732 retries once after make_path and then returns the EACCES error, so extract(dir) rejects. The unlink is not limited to files this call created, it applies to any file that exists before the call.
Verification: src/runtime/api/Archive.rs:1459-1466: openat fails with EACCES, and the new arm deletes whatever file is at that name and recreates it with the archive's content; nothing checks that this call created the file. src/libarchive/lib.rs:1718-1733 propagates the EACCES with ?, so extract(dest) rejects. On the base branch the glob path continued, keeping the file.
| #[cfg(unix)] | ||
| if let Some(rejection) = refused_symlink { | ||
| return rejection; |
There was a problem hiding this comment.
🟡 (optional) Users extracting with a glob onto a filesystem that cannot hold symlinks (FAT/exFAT, many CIFS and Docker Desktop bind mounts, drvfs) now get a rejection where main resolved and extract(dir) still resolves. At src/runtime/api/Archive.rs:1540-1547 an EPERM from symlinkat is retried once and then stored in refused_symlink at :1554; after every regular file has been written, :1570 returns it and the count is discarded. Fix: with a glob, treat a symlinkat refusal the way the unfiltered extractor and main do for a symlink entry, or apply one policy to both call forms; at minimum keep the Success count when the only refusals are symlinks.
Why this was flagged
Trigger: an archive with at least one symlink entry extracted via archive.extract(dir, { glob }) onto a mount whose symlinkat returns EPERM or EACCES. At src/runtime/api/Archive.rs:1540 symlinkat fails with EPERM; :1543-1547 runs make_path on the parent and retries, failing again; :1554 stores entry_error in refused_symlink. The loop keeps writing files, and at :1569-1571 the function returns the stored rejection instead of ExtractResult::Success(count). On the base commit the same arm did continue and the promise resolved with the file count; the unfiltered extract(dir) path (libarchive extract_to_dir) is unchanged in this PR and still resolves on a symlink failure. The population (CI containers on Windows hosts, USB/exFAT, SMB shares) sees every glob extract of such an archive flip from resolve to reject, with no way to learn how many entries were written.
Verification: At src/runtime/api/Archive.rs:1540 symlinkat fails; :1549-1561 only EEXIST is benign, every other errno is stored in refused_symlink at :1554; the loop keeps writing files, then :1569-1572 returns it, discarding count. On base (bd599f5a) the same path did continue, so the promise resolved with the symlink skipped. extract(dir) still resolves: src/libarchive/lib.rs:1136-1147 matches Err(_) with a warn and continues.
| return match parent { | ||
| // The open fails because this mkdir failed. | ||
| Some((parent_dir, Err(mkdir_err))) => { | ||
| ExtractResult::entry_error(&mkdir_err, root, parent_dir) | ||
| } |
There was a problem hiding this comment.
🟡 (optional) Users whose glob extract fails at a nested mkdir get an error path naming a directory that was never attempted, not the one the kernel refused. At src/runtime/api/Archive.rs:1473 (and :1436, :1557) entry_error overwrites the path that make_path already put in the error with the whole entry or parent name, but make_path walks up on ENOENT and the refused mkdirat is an ancestor component. Fix: build the rejection from the path the mkdir error carries (mkdir_err.path, joined with root) at the three make_path sites, falling back to the entry name only when that path is empty. Example: dest is 0o555 and the archive holds sub/deep/x.txt; the error says code: EACCES, syscall: mkdir, path: <dest>/sub/deep while the call that failed was mkdir <dest>/sub.
Why this was flagged
A user calls archive.extract(dest, { glob: "**" }) where dest is not writable (mode 0o555) and the archive has the file entry sub/deep/x.txt with no directory entries. mkdir_recursive_at_mode (src/sys/lib.rs:2407-2421) tries mkdirat(dir, "sub/deep"), gets ENOENT, and make_path_with steps to the previous component; mkdirat(dir, "sub") fails with EACCES and check_p! stores path = "sub" in the returned error. Archive.rs:1472-1474 calls ExtractResult::entry_error(&mkdir_err, root, parent_dir) with parent_dir = "sub/deep", and entry_error (Archive.rs:736-741) calls err.with_path(&path), discarding the "sub" the error carried. The promise rejects with path: <dest>/sub/deep, although the refused call was for <dest>/sub. The same override happens at Archive.rs:1436 for a directory entry and at Archive.rs:1557 for a symlink parent. The base branch swallowed these failures entirely, so this is the first time the path is surfaced; the PR's own tests only cover one-level parents, where the last component is also the refused one, so the suite does not catch the ancestor case.
Verification: mkdir_recursive_at_mode (src/sys/lib.rs:2402-2423) walks mkdirat(dir, "sub/deep") -> ENOENT -> mkdirat(dir, "sub") -> EACCES, so mkdir_err.path == "sub". src/runtime/api/Archive.rs:1472-1473 does ExtractResult::entry_error(&mkdir_err, root, parent_dir) with parent_dir = "sub/deep", and entry_error calls err.with_path(&path), which replaces path wholesale.
| let Some(name) = NormalizedName::new(raw_pathname, &mut normalized_buf[..]) else { | ||
| continue; | ||
| } | ||
| let pathname_z: &bun_core::ZStr = bun_paths::resolve_path::normalize_buf_z::< | ||
| bun_paths::platform::Posix, | ||
| >(raw_pathname, &mut normalized_buf[..]); | ||
| }; |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Pre-existing: users extracting with a glob still get a resolved promise, with a smaller count, when an entry name is too long for the path buffer, where the PR's purpose is to reject at every entry that was not written. At Archive.rs:1410-1412 NormalizedName::new returns None for a stored name of MAX_PATH_BYTES or more and the loop does continue, so the entry is silently dropped with no rejection and no count. Fix: treat a name that does not fit as a refused entry and return an ENAMETOOLONG rejection (code, syscall, path) like the other arms, so no entry is skipped without an error; the files(glob) path keeps spilling to a Vec since it writes nothing.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
An archive with a pax-extended name of 4096 or more bytes (Linux MAX_PATH_BYTES; the test at test/js/bun/archive.test.ts 'files(glob) lists an entry with a name longer than a path' builds such a 10,000-byte name) is passed to archive.extract(dest, { glob: '**' }), reaching extract_to_disk_filtered. At Archive.rs:1410 NormalizedName::new(raw_pathname, &mut normalized_buf[..]) returns None because stored.len() >= buf.len() (Archive.rs:1294-1296), and Archive.rs:1411 does continue. The entry is never written, never counted, and no error is produced; the promise resolves with the count of the other entries. The base branch did the same continue at the old raw_pathname.len() >= normalized_buf.len() check, so this is pre-existing, but it is the same class of swallowed failure the PR rewrites this function to remove, and a 300-byte name now rejects with ENAMETOOLONG while a 4096-byte name still vanishes silently. No safeguard reports it: the Directory, File and SymLink arms are never reached for that entry.
Verification: Pre-existing. Triggered when an archive entry's stored name is >= MAX_PATH_BYTES and the user calls extract(dest, { glob }). At src/runtime/api/Archive.rs:1410-1412 NormalizedName::new returns None and the loop does continue, so the entry is neither written, counted, nor turned into a rejection, and the promise resolves with a smaller count. The base branch had the identical skip (base Archive.rs:1347).
| if let Err(err) = dir_fd.make_path(pathname) { | ||
| return ExtractResult::entry_error(&err, root, pathname); | ||
| } | ||
| count += 1; |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Pre-existing: users extracting with a glob still get a resolved promise, with the entry counted, when a directory entry's name is taken by a regular file in the destination. At Archive.rs:1435 dir_fd.make_path maps mkdir's EEXIST to Ok without checking that the existing node is a directory (src/sys/lib.rs:2418), so the arm counts it at :1438 and nothing is created. Fix: after make_path on a directory entry, verify the node at pathname is a directory (fstatat) and otherwise reject through entry_error with the mkdir EEXIST or ENOTDIR, while still tolerating an existing directory, which the 'directory entry whose directory exists' test relies on.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
The destination holds a regular file named d (left by a previous extract or by the user) and the archive holds a directory entry d/ with no file entries below it, passed to archive.extract(dest, { glob: '**' }) which runs extract_to_disk_filtered. At src/runtime/api/Archive.rs:1435 dir_fd.make_path("d") calls mkdir_recursive_at_mode (src/sys/lib.rs:2402); mkdirat returns EEXIST for the file and src/sys/lib.rs:2418 maps it to MakePathStep::Exists, so make_path_with (src/paths/component_iterator.rs:212-216) returns Ok(()). The arm then does count += 1 at Archive.rs:1438 and the promise resolves with a count that includes a directory that does not exist. The base branch behaves the same (its EEXIST arm at the old :1375 was dead because make_path never surfaces EEXIST), so this is pre-existing; but the PR's stated purpose is that a refused mkdir rejects, and the PR notes list this row as not fixed. A file entry below d would reject with ENOTDIR at open, but a directory-only entry is silent.
Verification: pre-existing. Trigger: the destination holds a regular file d and the archive holds a directory entry d/, extracted with a glob. mkdir_recursive_at_mode maps EEXIST to Ok at src/sys/lib.rs:2418 without checking the node type, so at src/runtime/api/Archive.rs:1435-1438 count += 1 runs and the promise resolves while d remains a regular file. On the base the same route applied.
| match created { | ||
| Ok(()) => count += 1, | ||
| // The name is taken. It keeps what it has, and the entry is not counted. | ||
| Err(err) if err.get_errno() == bun_sys::E::EEXIST => {} |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Users re-extracting with a glob after an archive changed a name from a regular file to a symlink keep the stale file and get a resolved promise, while a file entry whose name is taken is replaced. At src/runtime/api/Archive.rs:1552 an EEXIST from symlinkat is swallowed and the entry is neither counted nor reported, so the symlink's target is never applied. Fix: handle a taken symlink name the way the file arm does at :1455-1462 (unlink and recreate, or at least surface the EEXIST through refused_symlink) so every taken-name case on the glob path has one behaviour.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
Trigger: extract(dir, { glob }) where dir already holds a regular file, directory, or older symlink at a symlink entry's name; entry point ExtractContext::do_run -> extract_to_disk_filtered. At src/runtime/api/Archive.rs:1540 symlinkat returns EEXIST; :1543 does not retry (EEXIST is not EPERM/ENOENT); :1552 matches EEXIST and does nothing, so count is not incremented and refused_symlink is untouched; :1573 returns Success. The file arm in the same PR replaces a taken name: O_TRUNC at :1453 and the EACCES unlink-and-reopen at :1455-1462. The base commit also continued on EEXIST, but the PR's stated contract is that a refused syscall rejects, and it rewrote this arm while keeping the one silent exception. A second extract over an existing tree (the exact scenario the PR added the EACCES replacement for) therefore leaves a stale regular file where the archive now has a symlink, with no signal. Remedy: unlink and recreate on EEXIST, or report it through refused_symlink.
Verification: Pre-existing, acknowledged in diff. Trigger: extract(dir, { glob }) into a destination that already has a regular file at a symlink entry's name. In the src/runtime/api/Archive.rs symlink arm EEXIST neither counts nor sets refused_symlink, so the stale file is left in place and the symlink's target never applied. The base's filtered path did exactly the same.
Related to #43132
Problem
archive.extract(dir, { glob })resolves although entries were not written.extract_to_disk_filtered(src/runtime/api/Archive.rs:1377to:1479) didcontinueafter a failed mkdir, open, write or symlink.extract(dir)rejects.archive.files(glob)matched the stored name,extract(dir, { glob })the normalized one. On atar -C dir .tarball,files("src/*")returns nothing.Fix
code,syscall,path). A refused symlink rejects at the end.EACCESatopenreplaces the file, so a second extract over a read-only file works.match_glob_patternstakes aNormalizedName. A leading./of a pattern is dropped.test/js/bun/archive.test.ts(20 new tests, 12 fail on main). Self-reviewed: 15 concerns raised, 9 addressed, 1 in part, 5 held.Background
extract_to_disk_filtered. Other calls runArchiver::extract_to_dir, whichbun installshares.extract()writes, without./and//.Downsides
extract(dir, { glob })now rejects where it resolved with a smaller count.extract(dir)still resolves at a directory or symlink that it cannot create (Notes).files(glob)+327 instructions per entry (+6.6%), anextract()job 152 to 160 bytes, release.text+2,816 bytes.Notes
Source of the report. An automated check found both defects. No user reported them. #43132 lists the glob skip in one row of its table. This PR changes that row: with a glob, the read-only
a.txtnow gets the new content.Three questions for a maintainer.
extract(dir)without a glob has the same defect for a directory entry below the top level and for every symlink entry: it resolves and counts the entry (src/libarchive/lib.rs:1628,:1635,:1136). The fix is a policy inExtractOptionsthat onlyBun.Archivesets, becausebun install,bun createand the--compiledownload share that extractor. It exists as commit6aba634052on branchrobobun/a178f38c/archive-entry-errors. It is not in this PR because it changesextract(dir)for callers that did not ask, and because it uses the per-opener policy that Bun.Archive: reject when a header read fails, with one in-memory tar reader and a named policy #43242 still asks about. Should it follow as its own PR?extract(dir)resolves,extract(dir, { glob })rejects. If that is not acceptable, I move the directory and symlink arms of the glob form into the held part. Then the forms differ on 2 of 30 probed rows (by a code walk, not a build).Archiver::extract_to_dir, andextract_to_disk_filtereddeleted? The design review set it aside for now: it changes 21 of 29 probed glob results (count, file mode, symlink rules), and its write path (read_data_into_fd) resolves after a refused write. This question does not gate this PR.Settlement per failure. Release builds of main (
bd599f5af9) and of this branch, Linux x64. 14 rows run as uid 65534 (chmodcases), 16 rows as uid 0 (cases that do not depend on the user).files(glob)againstextract({ glob }))extract(dir)rejects and the glob form resolvesextract(dir)resolves and the glob form rejectsThe 8 rows where the glob form still resolves: 2 controls, 1 read-only file that it now replaces, 3 symlinks whose name is taken (
EEXIST, the name keeps what it has), and 2 directory entries whose name is taken by a regular file (see "Not fixed here").What the glob form rejects with, as uid 65534:
EACCES,open,<dest>/a.txtEACCES,mkdir,<dest>/subEACCES,mkdir,<dest>/sub/emptyENOTDIR,open,<dest>/blocker/x.txtEISDIR,open,<dest>/takenENAMETOOLONG,open,<dest>/<name>EFBIG,write,<dest>/big.txt(the partial file is removed)EACCES,symlinkat,<dest>/sub/l(the other entries are written)pathis the destination as the caller passed it, joined with the name that the failed call was given. When the directory of an entry cannot be made, the rejection is thatmkdirerror, not theENOENTof theopenafter it.extract(dir)still rejects withError("ReadError")and nocode(#44509).Measurements. Release builds of main (
bd599f5af9) and of this branch (158f2fafcc), Linux x64.perf,valgrindandstraceare not installed here, so the counts come from ptrace tools (a syscall counter and anint3plus single-step instruction counter),objdump,nmandsize.extract():mov $0x98,%edibeforecall mi_mallocon main,$0xa0here (152 to 160 bytes, both in the 160-byte class).ExtractResult8 to 16 bytes. A const assert keeps it at 16.extract(dir, { glob: "**" }), output syscalls per 1,000 entries (N = 1,000 against 2,000): top-level filesopenat1,000,write1,000,close1,000. Nested files addmkdirat1,000. Directoriesmkdirat1,000. Symlinkssymlinkat1,000. Identical on both builds. A fixed archive of 6 entries: 95 file syscalls on both.files("**"): 4,986 to 5,313 instructions per regular entry (+327, +6.6%) for 10-byte names. That is the one normalize pass.files()without a glob: 4,574 to 4,555 (no change within the spread).bun install: no line ofbun_libarchiveorbun_installchanges.extract_to_dir::<()>6,832 bytes,create_deferred_symlinks2,411,ExtractTarball::run10,487,drain_callback12,075 on both builds..text: 65,453,397 to 65,456,213 bytes (+2,816). The file grows by 4,096 bytes.ExtractContextjob function 4,359 to 5,498 bytes,FilesContextjob function 2,380 to 2,982.Behavior changes to know.
extract(dir, { glob })stops at a refused directory or file, asextract(dir)does. The entries after it are not written. Main wrote them.openrefuses withEACCESis unlinked and created again. Main skipped the entry and kept the old content. This covers an archive that holds a read-only file twice (tar -rf), and a second extract over read-only files of the first.extract(dir)creates files writable, so it did not have the case.files("./src/*")andextract(dir, { glob: "./src/*" })now meansrc/*. On main the first matched only stored./names, the second matched nothing.files(glob)still lists an entry thatextract()leaves out for its name (second byte:). A test pins it.tar -C dir .archive, and passedsrc/x\.env, whichextract()writes as the hidden filesrc/x/.env.Not fixed here.
extract(dir): the directory and symlink arms (question 1), and a refused write inside a.tar.gzentry, whereread_data_into_fdfalls back towrite()at offset 0 (tar extraction: no disk allocation from the entry header, file length from libarchive #36542)...component, a name whose second byte is:, hard links, FIFOs and devices.bun pm diffmatches its patterns against stored names.files(). Another change tracks it.Other open PRs on this function. All are mine. None converts these arms.
archive.test.ts(belowFile: 0, twotoBe(1)), and paths: stop the recursive mkdir walk when a parent exists but its child is still not found #40600 at one (count: 1for an entry below a dangling symlink). Whichever lands second changes those expectations to a rejection.extract(dir)) addsExtractResult::SysErrunboxed. The const assert of this PR then needs its bound changed or the box kept.EACCESretry of this PR.Self-review. A review of the first shape (three commits on top of #43242) asked for this shape: a PR against main without the
extract(dir)half. It raised 15 concerns.files(glob)keeps an entry with a long name. Two tests for the error of a parent directory. Per-platform errno values in place of a wildcard, and a reason at each skipped test. The wording of the keys offiles(). The comment and the test name ofNormalizedName, and a test for the:name. The docs recipe. One-line comments.Tests run. Debug build (ASAN):
test/js/bun/archive.test.ts127 pass, 2 skip as root, 128 pass, 1 skip as uid 65534. Release build of main: 12 of the 20 new tests fail as root, 15 as uid 65534. The 5 that pass on both pin what must not change (an existing directory, a symlink whose name is taken, an entry that the glob leaves out, the:name, a long name).bun run rust:check-all: 12 targets ok. The Windows lines are type-checked only. I cannot run Windows here, so the Windows values of two tests (EISDIR,ENOENT) are a first pin for CI.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/archive.test.ts