From a296227618c04ac1012ac2ed64267afb97ca346a Mon Sep 17 00:00:00 2001 From: ValisSowilo Date: Wed, 23 Sep 2026 00:11:23 -0400 Subject: [PATCH] Clamp threads on extraction, and stop four ways to lose data quietly Found by a read-through of the streaming paths. Compression and extraction already stream a segment at a time; what was missing was the guard rails around that. Extraction had no thread clamp. threads_for() sized -t to fit in RAM on the compress side only; run_members took NTHREAD as given. Decoding holds more per worker than encoding, so `x -t0` on a -9 archive asked for a full model per core and died in the allocator with no hint the thread count was why. It now applies the same clamp, sizing the model for a strided geometry (the largest, 30 contexts at -9) since the real one is not known until segments are parsed. On a 16 GB box -t64 at -9 now caps at -t13. `r bad.gl bad.gl` destroyed the archive it was asked to repair. do_repair opens its output "wb" before reading a segment, so the input was truncated and every segment then reported unrecoverable. Refused now, by file identity rather than spelling, so ./bad.gl and hard links count. Non-regular files were archived as empty. fsize64 seeks to the end, which on a FIFO, device or /proc entry gives 0, so `c a.gl /dev/stdin` or a process substitution stored a zero-byte member with a valid SHA-256 and exit 0. The walk now skips anything that is not a regular file, says so, and exits 1. The output archive was archived into itself. `c backup.gl .` run a second time finds last run's backup.gl in the walk, truncates it, and stores whatever partial copy of the new archive exists when it gets there. Inputs that are the output file are dropped with a note. Segment rawlen was bounded at 1 TB instead of by the header. The writer never emits a segment over its -s (floored at MINCHUNK), so anything above that is a corrupt index. Before, a crafted one got a terabyte malloc per thread and exited 1 "out of memory"; now it is "index is corrupt", exit 2. sfuzz.py gains rawlen_over_segmax. rawlen_over_wn_packed claims 256 MB, which the new bound would now stop at the index, so it declares a 512 MB segmax to keep reaching the decoder check it was written for. Archives byte-identical to the previous build at -1 -5 -9 -f1, and old and new binaries read each other's output. sfuzz (93 runs), gfuzz (40) and tfuzz (60) pass on Windows; the symlink cases were skipped on this host. --- gleipnir.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++-- scripts/sfuzz.py | 10 ++++-- 2 files changed, 98 insertions(+), 6 deletions(-) diff --git a/gleipnir.c b/gleipnir.c index f566194..8e43f8a 100644 --- a/gleipnir.c +++ b/gleipnir.c @@ -3189,9 +3189,50 @@ static int is_dir(const char *p) { return (st.st_mode & S_IFMT) == S_IFDIR; } +/* True if a and b name the same file on disk, by identity rather than by + * spelling: "a.gl", "./a.gl" and a hard link to it all count. False when + * either does not exist or cannot be asked about. */ +static int same_file(const char *a, const char *b) { +#ifdef _WIN32 + BY_HANDLE_FILE_INFORMATION ia, ib; + DWORD share = FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE; + HANDLE ha = CreateFileA(a, 0, share, NULL, OPEN_EXISTING, + FILE_FLAG_BACKUP_SEMANTICS, NULL); + if (ha == INVALID_HANDLE_VALUE) return 0; + HANDLE hb = CreateFileA(b, 0, share, NULL, OPEN_EXISTING, + FILE_FLAG_BACKUP_SEMANTICS, NULL); + if (hb == INVALID_HANDLE_VALUE) { CloseHandle(ha); return 0; } + int same = GetFileInformationByHandle(ha, &ia) && + GetFileInformationByHandle(hb, &ib) && + ia.dwVolumeSerialNumber == ib.dwVolumeSerialNumber && + ia.nFileIndexHigh == ib.nFileIndexHigh && + ia.nFileIndexLow == ib.nFileIndexLow; + CloseHandle(ha); CloseHandle(hb); + return same; +#else + STAT_T sa, sb; + return STAT_F(a, &sa) == 0 && STAT_F(b, &sb) == 0 && + sa.st_dev == sb.st_dev && sa.st_ino == sb.st_ino; +#endif +} + static void walk(const char *path, Names *out) { if (excluded(path)) return; - if (!is_dir(path)) { nm_add(out, path); return; } + if (!is_dir(path)) { + /* Only regular files have a size that means anything. A FIFO, a + * device or a /proc entry seeks to 0, so it used to be stored as an + * empty member with a valid SHA-256 and exit 0 -- `gleipnir c a.gl + * /dev/stdin` archived nothing and reported success. A path that + * cannot be stat'd at all is kept, so fopen reports the real error. */ + STAT_T st; + if (STAT_F(path, &st) == 0 && (st.st_mode & S_IFMT) != S_IFREG) { + fprintf(stderr, "gleipnir: %s: not a regular file, skipped\n", path); + EXITCODE = 1; + return; + } + nm_add(out, path); + return; + } #ifdef _WIN32 char pat[NAME_MAX_B]; snprintf(pat, sizeof pat, "%s\\*", path); @@ -3413,6 +3454,24 @@ static int do_compress(char **paths, int npath, const char *outp, int lvl) { } files.n = w; } + + /* The output is truncated before any input is read, so an input that is + * the output -- `gleipnir c backup.gl .` run a second time, where the walk + * finds last run's backup.gl -- would be archived as a partial copy of + * the archive being written. Drop it, and say so. */ + { + size_t w = 0; + for (size_t i = 0; i < files.n; i++) { + if (same_file(files.v[i], outp)) { + if (VERBOSE) + fprintf(stderr, "gleipnir: %s: is the output archive, skipped\n", + files.v[i]); + free(files.v[i]); free(snames[i]); continue; + } + files.v[w] = files.v[i]; snames[w] = snames[i]; w++; + } + files.n = w; + } if (!files.n) { fprintf(stderr, "gleipnir: nothing to compress\n"); return 1; } /* Two inputs can resolve to the same stored name -- `gen c a.gen /x/data @@ -3771,6 +3830,11 @@ static int arc_open(const char *path, Archive *a) { } uint64_t ns = rd64(&r); if (r.bad || ns > (1u << 28)) { free(ib); goto bad; } + /* The writer never emits a segment longer than the -s it records in the + * header (floored at MINCHUNK). Allowing anything up to 1 TB let a crafted + * index ask extraction for a terabyte per thread, which surfaced as "out + * of memory", exit 1, rather than as the corrupt archive it is. */ + uint64_t seglim = a->h.segmax > MINCHUNK ? a->h.segmax : MINCHUNK; for (uint64_t i = 0; i < ns; i++) { Seg s; s.off = rd64(&r); s.clen = rd64(&r); @@ -3779,7 +3843,7 @@ static int arc_open(const char *path, Archive *a) { /* A segment must lie entirely inside the payload region. Without this * a corrupt offset would seek anywhere in the file and decode noise. */ if (s.off < HDR_BYTES || s.clen > ixoff || s.off > ixoff - s.clen || - s.rawlen > (1ull << 40)) { r.bad = 1; break; } + s.rawlen > seglim) { r.bad = 1; break; } idx_adds(&a->x, s); } /* Recovery records are optional and were added after the segment table, so @@ -3923,7 +3987,23 @@ static int run_members(Archive *a, const char *destdir, int test_only, for (uint64_t i = 0; i < a->x.nm; i++) grand += a->x.m[i].size; - int nthr = NTHREAD > 1 ? NTHREAD : 1; + /* The same clamp compression applies. Without it `x -t0` on a -9 archive + * asked for a full model per core -- more than compression ever did, since + * decoding holds more per worker -- and died in the allocator with no hint + * that the thread count was the cause. Geometry is not known until the + * segments are parsed, so size the model for a strided one, which is the + * largest (30 contexts at -9 against 27), and put the globals back. */ + int nthr = 1; + if (NTHREAD > 1) { + uint64_t segbytes = 0; + for (uint64_t i = 0; i < a->x.ns; i++) + if (a->x.s[i].rawlen > segbytes) segbytes = a->x.s[i].rawlen; + int ds = DET_STRIDE, dw = DET_WIDTH; + DET_STRIDE = 1; DET_WIDTH = 1; + set_level(a->h.lvl); + nthr = threads_for(segbytes); + DET_STRIDE = ds; DET_WIDTH = dw; + } SegR *sr = calloc((size_t)nthr, sizeof(SegR)); Job *jb = calloc((size_t)nthr, sizeof(Job)); Job *rj = calloc((size_t)nthr, sizeof(Job)); @@ -4531,6 +4611,14 @@ int main(int argc, char **argv) { fprintf(stderr, "gleipnir: r needs an output archive\n"); arc_close(&ar); return 1; } + /* do_repair opens its output "wb" before reading a single segment, so + * `gleipnir r bad.gl bad.gl` truncated the damaged archive to nothing + * and then tried to rebuild it from the empty file. */ + if (same_file(arc, argv[a])) { + fprintf(stderr, "gleipnir: r would overwrite the archive it is " + "repairing; write to a different file\n"); + arc_close(&ar); return 1; + } rc = do_repair(&ar, argv[a]); } else if (mode == 't') rc = deep ? run_members(&ar, NULL, 1, NULL, 0) diff --git a/scripts/sfuzz.py b/scripts/sfuzz.py index 50e2d08..3738f50 100644 --- a/scripts/sfuzz.py +++ b/scripts/sfuzz.py @@ -135,7 +135,7 @@ def seg_stored(payload): def build(path, name, blob, rawlen, seghash=0, sha=b"\0" * 32, lvl=5, - memshift=0, segoff=None, seg0=0, nseg=1): + memshift=0, segoff=None, seg0=0, nseg=1, segmax=64 << 20): """A complete archive: one member, one segment, with a correct header hash, index hash and trailer. Everything arc_open verifies is right, so the decoder is always the thing under test.""" @@ -157,7 +157,7 @@ def build(path, name, blob, rawlen, seghash=0, sha=b"\0" * 32, lvl=5, struct.pack_into("