Skip to content

Commit 8999e8d

Browse files
Peter MarreckPeter Marreck
authored andcommitted
Fix double-free in parseDirectoryEntries on malformed dir entries
Directory entries registered both an outer errdefer and an inner defer against the same full_name allocation. When the recursive parse hit an error (e.g. Truncated), both fired and freed the same pointer twice, causing SIGABRT/SIGSEGV on malformed CPT archives. Found by validate's --test-coverage stress run (exit 134 on corrupted sample.cpt). Fix: hoist the defer/errdefer into the specific branch that owns the allocation. Dir entries use a single defer (success continue and error unwind both free exactly once); file entries use errdefer (ownership transfers to the accumulator on successful append). Regression test crafts a minimal archive with a non-empty directory name whose recursive child is truncated, which std.testing.allocator detects as a double-free under the unfixed code.
1 parent e4d328e commit 8999e8d

2 files changed

Lines changed: 37 additions & 2 deletions

File tree

src/core.zig

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -215,18 +215,24 @@ fn parseDirectoryEntries(
215215
const is_dir = (name_len_kind & 0x80) != 0;
216216
const name_part = try reader.readSlice(name_len);
217217
const full_name = try joinPath(allocator, prefix, name_part);
218-
errdefer allocator.free(full_name);
219218

220219
if (is_dir) {
220+
// Directory entries: full_name is only consumed as the recursion prefix.
221+
// A single `defer` frees on every exit (success continue and error unwind),
222+
// avoiding the double-free that an outer errdefer + inner defer would cause.
223+
defer allocator.free(full_name);
221224
const descendants = try reader.readU16();
222225
const subtree_count: usize = @as(usize, descendants) + 1;
223226
if (subtree_count > remaining) return Error.InvalidEntryCount;
224-
defer allocator.free(full_name);
225227
try parseDirectoryEntries(allocator, reader, acc, full_name, @intCast(descendants));
226228
remaining -= subtree_count;
227229
continue;
228230
}
229231

232+
// File entries: full_name transfers to acc on successful append. errdefer
233+
// handles the error path; successful iteration leaves full_name owned by acc.
234+
errdefer allocator.free(full_name);
235+
230236
const entry = MetadataEntry{
231237
.name = full_name,
232238
.volume = try reader.readU8(),

tests/unit/zig_unit_tests.zig

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -381,3 +381,32 @@ test "archive header crc changes when entry metadata changes" {
381381
const crc_b = readU32BE(archive_b, @intCast(off_b));
382382
try std.testing.expect(crc_a != crc_b);
383383
}
384+
385+
test "parseMetadata: corrupted directory entry must not double-free (regression)" {
386+
// Reproduces a real crash found by validate --test-coverage stress testing: a
387+
// truncated CPT archive whose outer entry is a directory used to free the
388+
// directory's full_name path twice (outer errdefer + inner defer on the same
389+
// allocation) when the recursive parse hit Truncated. std.testing.allocator's
390+
// GeneralPurposeAllocator surfaces the double-free as a test failure. Uses a
391+
// non-empty directory name so the allocation is actually tracked.
392+
var buf: std.ArrayListUnmanaged(u8) = .{};
393+
defer buf.deinit(allocator());
394+
395+
// Preamble: marker(0x01) + pad(1) + pad(2) + header_offset=u32 BE -> 8
396+
try buf.append(allocator(), 0x01);
397+
try buf.append(allocator(), 0);
398+
try buf.appendSlice(allocator(), &[_]u8{ 0, 0 });
399+
try buf.appendSlice(allocator(), &[_]u8{ 0, 0, 0, 8 });
400+
401+
// Header: crc(u32 BE, ignored when strict=false), entry_count=2 (u16 BE),
402+
// comment_len=0, then one dir entry (name_len_kind=0x83 → is_dir + name_len=3),
403+
// name="dir", descendants=1, then truncated so the recursive call errors out.
404+
try buf.appendSlice(allocator(), &[_]u8{ 0, 0, 0, 0 });
405+
try buf.appendSlice(allocator(), &[_]u8{ 0, 2 });
406+
try buf.append(allocator(), 0);
407+
try buf.append(allocator(), 0x83);
408+
try buf.appendSlice(allocator(), "dir");
409+
try buf.appendSlice(allocator(), &[_]u8{ 0, 1 });
410+
411+
try std.testing.expectError(core.Error.Truncated, core.parseMetadata(allocator(), buf.items, false));
412+
}

0 commit comments

Comments
 (0)