search

Tarsnap / kivaloo

Kivaloo is a collection of utilities which together form a data store associating keys of up to 255 bytes with values of up to 255 bytes.
http://www.tarsnap.com/kivaloo.html
Other
201 stars 17 forks source link

kvlds: avoid double-free of findleaf() cookie #205

Closed gperciva closed 3 years ago

gperciva commented 3 years ago

Two alternative fixes for the same bug. #205 is a smaller fix, while #206 is bigger but makes the code easier to reason about. (That said, I might be missing something about the code organization, so I wouldn't be shocked if you prefer the smaller fix.)

cperciva commented 3 years ago

Neither fix is correct. findleaf can pass the cookie to btree_node_fetch and see it come back later; that alone breaks the patch in #206, while #205 fails to free the cookie if findleaf fails when it is invoked from btree_node_fetch (aka after needing to page in a node).

The correct fix is to say that findleaf take ownership of the cookie and is responsible for ensuring that it is freed even if findleaf returns non-zero.

gperciva commented 3 years ago

Updated

gperciva commented 3 years ago

Another attempt, again with low confidence in the comments.

I'm particularly un-enthused about

if we found the leaf and (C->e != NULL), then C->callback_range is responsible for freeing C->e.

I mean, I get that you don't want to use kvldskey_dup(C->e) to pass a new value to the callback, to avoid an unnecessary memory allocation. But that makes the explanation of what's happening a bit more wordy.

cperciva commented 3 years ago

How about simply "findleaf is responsible for ensuring that C and C->e are freed"? Along with a comment you already have in the code about how C->e was freed by callback_range.

gperciva commented 3 years ago

Updated.