Hi,

In current git, I'm getting a crash in the free_xattr() just before the end of recv_generator():

(gdb) bt
#0  0x0000000000445d98 in rsync_xal_free (xalp=0x1986e60) at xattrs.c:101
#1  0x0000000000445df9 in free_xattr (sxp=0x7ffff250d2d0) at xattrs.c:111
#2  0x0000000000415ab7 in recv_generator (fname=0x7ffff250f540 "xxxxxx", file=0x7f457b201db8, ndx=8, 
    itemizing=1, code=FLOG, f_out=1) at generator.c:1892
#3  0x0000000000416e96 in generate_files (f_out=1, local_name=0x0) at generator.c:2229
#4  0x000000000042521c in do_recv (f_in=3, f_out=1, local_name=0x0) at main.c:945
#5  0x000000000042561b in do_server_recv (f_in=0, f_out=1, argc=1, argv=0x195a248) at main.c:1057
#6  0x000000000042575d in start_server (f_in=0, f_out=1, argc=2, argv=0x195a240) at main.c:1091
#7  0x0000000000426a28 in main (argc=2, argv=0x195a240) at main.c:1630
(gdb) p *xalp
$1 = {items = 0x0, count = 1, malloced = 5}

Note that *xalp has count==1 but items==NULL, which is at odds with what rsync_xal_free() is expecting to see.

At the moment I'm only able to trigger this when running with --link-by-hash, either my previous hacked together version (#8654, #8655) or today's newly committed version (many thanks for that!). However, tracing the code through, I think that's just the trigger and the underlying problem is in the unpatched master code.

I can reproduce the problem at will with:

/tmp/rsync-testing --rsync-path=/tmp/rsync-testing -aHXX --link-by-hash=../link-by-hash --link-dest=../A --link-dest=../B C/ server:${PWD}/C/

...where ../link-by-hash was previously populated with a full sync of 'A'.

Unfortunately it's on a very large data set (although it crashes on the 2nd file into the set) and I haven't yet been able to reduce it to a simple test case.

But looking through the code there's an obvious issue. In generator.c recv_generator(), with just a few lines removed for clarity, we have:

recv_generator() {
        ...
1373:   itemize(fnamecmp, file, ndx, statret, &sx,
            statret ? ITEM_LOCAL_CHANGE : 0, 0, NULL);
        ...
1657:   real_sx = sx;
        ...
1837:   free_xattr(&real_sx);
        ...
1892:   free_xattr(&sx);
        ...
}

With xattrs in play, the itemize() ends up mallocing bits of memory and attaching it to sx.xattr.  The "real_sx = sx" does a copy of the top level structure, including the pointer to sx.xattr.  The free_xattr(&real_sx) descends into real_sx.attr (which is pointing to the same location as sx.xattr) and frees up the malloced memory, and then the free_xattr(&sx) tries to do the same thing and we go "pop!".

Note that sx.acc_acl and sx.def_acl have the same issue (I'm not using acls so I'm not seeing the problem there).

I had a look at doing a deep copy of sx into real_sx as my first thought, however that quickly turned into a maze of twisty little mallocs, all alike, and I'm not sure the complexity is necessary.

Another solution might be to ensure only one of sx or real_sx has the pointers to sx.xattr, sx.acc_acl and sx.def_acl, e.g. by simply NULLing them in the other one.

Unfortunately I'm not understanding how sx and real_sx are being used to know if NULLing those pointers in either of sa or real_sa is the right thing to do.

Suggestions welcome!

A fully fledged fix would be even more welcome! :-)

Cheers,

Chris