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

On Mon, Jun 19, 2023 at 11:13:32PM -0300, Leonardo Bras wrote:
> On Fri, 2023-06-16 at 03:39 -0300, Leonardo Bras 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, move
> > struct traceroot->max to be the first element, and guarantee the
> > address
> > used for free() is the same returned by malloc().
> 
> Extra info:
> 
> Bug reproduction:
> https://gitlab.com/linux-kernel/perfbook/-/jobs/4501216686#L212
> 
> With bugfix provided in this patch:
> https://gitlab.com/linux-kernel/perfbook/-/jobs/4485986705

Hello, Leo, and apologies for being slow.

My feeling is that there is a deeper bug involving use of the wrong
pointer, as in freeing a pointer to a field of the enclosing structure.
What are your thoughts on adjusting things so that the correct pointer
is freed?  (And no, I have not yet looked at this closely, so there
might well be a very good reason why my suggestion is bogus.  But I have
to ask!)

							Thanx, Paul

> Thanks,
> Leo
> 
> > 
> > Signed-off-by: Leonardo Bras <[email protected]>
> > ---
> >  CodeSamples/datastruct/Issaquah/tree.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/CodeSamples/datastruct/Issaquah/tree.h
> > b/CodeSamples/datastruct/Issaquah/tree.h
> > index f007558a..bbe5e7c1 100644
> > --- a/CodeSamples/datastruct/Issaquah/tree.h
> > +++ b/CodeSamples/datastruct/Issaquah/tree.h
> > @@ -48,8 +48,8 @@ struct treenode {
> >   * Root of a tree.
> >   */
> >  struct treeroot {
> > -       struct treenode min;
> >         struct treenode max;
> > +       struct treenode min;
> >  } __attribute__((__aligned__(CACHE_LINE_SIZE)));
> >  
> >  void treenode_wire_call_rcu(void);
>