On Wed, Jun 21, 2023 at 02:18:31AM -0300, Leonardo Brás wrote:
> While building the CodeSamples/datastruct/Issaquah/ directory, I can see
> a couple instances of this warning:
>
> In function ‘free_treenode_cache’,
> inlined from ‘tree_remove_all’ at tree.c:102:2,
> inlined from ‘tree_free’ at tree.c:128:2:
> tree.c:251:9: warning: ‘free’ called on pointer ‘trp’ with nonzero offset 96 [-Wfree-nonheap-object]
> 251 | free(tnp);
> | ^~~~~~~~~
>
> I took a look and tried to understand what was happening:
> - tree_remove_all() calls free_treenode_cache() on it's input, which ends
> up free()'ing it (!BAD_MALLOC)
> - It makes sense in most treenodes, since they are allocated with
> alloc_treenode_cache() and the malloc() output is the same as the free()
> input.
> - tree_free() calls tree_remove_all() on &trp->max, which ends up trying
> to free() this same address.
> - trp is a struct treeroot, which is composed of 2 treenodes: min & max
> - The output of malloc() for trp ends up being different from the address
> used for free(), since &trp->max is used instead, and there is an offset
> since max is the second element of struct treeroot.
>
> To solve this while keeping the tree_remove_all() generic, add a boolean
> free_node parameter to tree_remove_all() so the caller can decide if the
> node should be freed. The new boolean is true for normal treenodes, and
> false if the pointed treenode is contained in the struct treeroot.
>
> Signed-off-by: Leonardo Bras <leobras.c@xxxxxxxxx>
Applied, thank you!!!
I really should get this code described in the book at some point...
Thanx, Paul
> ---
> CodeSamples/datastruct/Issaquah/tree.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/CodeSamples/datastruct/Issaquah/tree.c b/CodeSamples/datastruct/Issaquah/tree.c
> index 052fa870..26600118 100644
> --- a/CodeSamples/datastruct/Issaquah/tree.c
> +++ b/CodeSamples/datastruct/Issaquah/tree.c
> @@ -89,17 +89,19 @@ struct treeroot *tree_alloc(void)
> * readers accessing the tree.
> */
> static void tree_remove_all(struct treenode *cur,
> - void (*freefunc)(void *p))
> + void (*freefunc)(void *p),
> + bool free_node)
> {
> if (cur == NULL)
> return;
> - tree_remove_all(cur->lc, freefunc);
> - tree_remove_all(cur->rc, freefunc);
> + tree_remove_all(cur->lc, freefunc, true);
> + tree_remove_all(cur->rc, freefunc, true);
> if (cur->perm)
> return;
> if (cur->data && freefunc)
> freefunc(cur->data);
> - free_treenode_cache(cur);
> + if (free_node)
> + free_treenode_cache(cur);
> }
>
> /*
> @@ -125,7 +127,7 @@ static void tree_check_insertion(struct treeroot *trp, struct treenode *new)
> */
> void tree_free(struct treeroot *trp, void (*freefunc)(void *p))
> {
> - tree_remove_all(&trp->max, freefunc);
> + tree_remove_all(&trp->max, freefunc, false);
> free(trp);
> }
>
> --
> 2.41.0
>
