Skip to content

store: appendVersion OOM during first-touch path dupe poisons the files map — UB on retry, invalid free on deinit #603

@justrach

Description

@justrach

Problem

Store.appendVersion (src/store.zig:86-91) calls files.getOrPut(path) and only afterwards dupes the path:

const entry = try self.files.getOrPut(path);
if (!entry.found_existing) {
    const duped = try self.allocator.dupe(u8, path);   // can fail
    entry.key_ptr.* = duped;
    entry.value_ptr.* = FileVersions.init(self.allocator, duped);
}

getOrPut already inserted the slot: its key is the caller's transient path slice and its value is undefined memory. If the dupe fails, the function errors out and the poisoned entry stays in the map. Every later path is then broken:

  • A retry with the same path takes found_existing = true and runs entry.value_ptr.versions.append(...) on an undefined FileVersions — undefined behavior (garbage len/capacity/pointer).
  • Store.deinit frees entry.key_ptr.*, a slice the store never allocated — invalid free.
  • Once the caller's path buffer is freed, the map holds a dangling key that any probe of that slot can read — UAF.

Same class as #594 (OOM error-path corruption), in the store instead of the explorer.

Failing Test

src/test_core.zig (fails on current release tip: expected 0, found 1 — the poisoned entry persists, plus a leak because deinit cannot run safely):

test "issue-XX: appendVersion failed key dupe leaves a poisoned files entry" {
    var failing = std.testing.FailingAllocator.init(testing.allocator, .{ .fail_index = 1 });
    var store = Store.init(failing.allocator());

    try testing.expectError(error.OutOfMemory, store.recordSnapshot("src/a.zig", 10, 0x1));
    try testing.expectEqual(@as(usize, 0), store.files.count());
    store.deinit();
}

(allocation 0 = getOrPut table growth, allocation 1 = the path dupe.)

Expected

A failed appendVersion leaves the store exactly as it was: no entry for the path, retry works, deinit is safe.

Fix

Scope an errdefer to the first-touch block so the slot is removed if the dupe fails:

const entry = try self.files.getOrPut(path);
if (!entry.found_existing) {
    errdefer self.files.removeByPtr(entry.key_ptr);
    const duped = try self.allocator.dupe(u8, path);
    entry.key_ptr.* = duped;
    entry.value_ptr.* = FileVersions.init(self.allocator, duped);
}

FileVersions.init is infallible, so the errdefer only guards the dupe; later failures (e.g. versions.append) leave a valid, initialized entry behind, which is fine.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingpriority:p0Highest priority

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions