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 <leobras.c@xxxxxxxxx> > > --- > > 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); >
