Skip to content

Fix use-after-free between ZipperHead readers and writers - #150

Open
imlvts wants to merge 3 commits into
Adam-Vandervorst:masterfrom
imlvts:fix/zipper-head-reader-writer-uaf
Open

imlvts wants to merge 3 commits into
Adam-Vandervorst:masterfrom
imlvts:fix/zipper-head-reader-writer-uaf

Conversation

@imlvts

@imlvts imlvts commented Sep 29, 2026

Copy link
Copy Markdown
Collaborator

A head reader owned whatever node the walk down its path reached, which for a path that stops inside a node -- or is not in the trie at all -- is a node the head's writers descend through. The next exclusive writer copied it, and the writers already made kept pointing into the copy that was then dropped; the reproducer segfaults. A value at the reader's root was borrowed from the node above it, which writers copy the same way.

Keep owning the node when the path lands on one and it carries no value of its own: no writer may be at or below the reader's path, so none is inside it, and that is the common case. Otherwise copy just this entry -- its value, its subtrie, or its dangling path -- into a private node, and root the reader one byte above it.

A head reader owned whatever node the walk down its path reached, which
for a path that stops inside a node -- or is not in the trie at all -- is
a node the head's writers descend through. The next exclusive writer
copied it, and the writers already made kept pointing into the copy that
was then dropped; the reproducer segfaults. A value at the reader's root
was borrowed from the node above it, which writers copy the same way.

Keep owning the node when the path lands on one and it carries no value
of its own: no writer may be at or below the reader's path, so none is
inside it, and that is the common case. Otherwise copy just this entry --
its value, its subtrie, or its dangling path -- into a private node, and
root the reader one byte above it.
@luketpeterson

Copy link
Copy Markdown
Collaborator

This is a really serious bug and fixing it is non-negotiable. But the perf cost of the fix seems to be quite nasty. I don't have any more time to spend on this immediately. But I'll try to come up with something that is a little easier on the benchmarks.

@imlvts

imlvts commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator Author

I'm looking at the benchmarks, and there doesn't seem to be a big impact?
What's the impact of targeted benchmark?

@luketpeterson

luketpeterson commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator

I'm looking at the benchmarks, and there doesn't seem to be a big impact? What's the impact of targeted benchmark?

I had to add new benchmarks to make sure the affected cases in particular were hit. But when I added (had Claude add) the new benchmarks, (and cherry-picked them back to master locally), here are the results I saw:

 Benchmark                                             Base → PR             Change
 shared_parent_reader_val (1,000 calls)                1.013 → 5.486 µs         5.4× slower
 shared_parent_reader_is_val (1,000 calls)             0.772 → 5.520 µs        7.1× slower
 shared_parent_reader_is_shared (1,000 calls)          1.764 → 5.806 µs        3.3× slower
 shared_parent_reader_get_focus (1,000 calls)          4.682 → 12.108 µs      2.6× slower
 borrowed_head_read_value_in_shared_parent (100 reads) 8.542 → 12.236 µs         43% slower
 borrowed_head_read_missing_path (100 reads)           9.959 → 12.460 µs    25% slower
 borrowed_head_read_creation (100 reads)               8.805 → 8.665 µs    No clear change

You don't need to spend any more time on this because I haven't yet validated there isn't something bogus going on. And I think I have some ideas about how to approach speeding up a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants