Hi Frederik,
On Fri, Oct 07, 2016 at 10:08:37AM -0700, Frederik Deweerdt wrote:
> Hello,
>
> shsess_store() has this odd looking code sequence (including line numbers):
>
> 333 /* it returns the already existing node
> 334 or current node if none, never returns null */
> 335 oldshsess = shsess_tree_insert(shsess);
> 336 if (oldshsess != shsess) {
> 337 /* free all blocks used by old node */
> 338 shsess_free(oldshsess);
> 339 shsess = oldshsess;
> 340 }
>
> Is the `shsess = oldshsess` assignment intentional? It looks like the code
> might be leaking the blocks from shsess?
I agree it looks strange. Let's try to see what happens :
1) the original data to insert is in shsess.
2) line 335: we get a pointer to a node containing these data in the
tree, whether it is a new insert or reuses an existing one
3) line 336: the only way the two pointers are different is if the
key already existed, in which case the old one is returned and
the new one (shsess) is not inserted.
4) line 338: the key currently in the tree is removed from the tree
5) shsess is lost and now points to the just freed entry.
I don't understand the intent here. I suspect that the purpose was to
free shsess instead so that shsess now points to the key in the tree,
which contains the same key and can be updated.
Or maybe there's a trick later consisting in systematically inserting
the new node into the tree, so once the old one was released to the
free list and the new pointer points to it, it can safely be reinserted.
I must confess that's obscure to me. I'm CCing Emeric who certainly knows
better.
Cheers,
Willy
