Re: [PATCH v2] CodeSamples/tree: Fix compiler warning on free

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 <[email protected]>

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
>