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

On Tue, 2023-06-20 at 15:40 -0700, Paul E. McKenney wrote:
> 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.

That's correct.
I have proposed a hack, which works even though the code is still 'incorrect'.

The thing is that the current code calls tree_remove_all() for &trp->max which
was malloc'ed as a struct treeroot, unlike the other treenodes, and yet will try
to free trp->max.

> 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!)

Sure, it's possible to do that.
I will create a helper _tree_remove_all() that does everything except remove the
treenode, and call that on trp->max @ tree_free(), as the trp itself is freed
soon after.

Then I will call _tree_remove_all() + free_treenode_cache() at 
tree_remove_all(). This should make sure everything is right.

It's a longer change, and I was unsure of how much would be ok to change in that
code, but I am happy to provide v2 like this.

I will send the v2 soon.

Thanks,
Leo


> 
> 							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);
> > 
> 
>