Bug 8665 - Crash in free_xattr(), from recv_generator()
Crash in free_xattr(), from recv_generator()
Status: RESOLVED FIXED
Product: rsync
Classification: Unclassified
Component: core
3.1.0
All All
: P5 normal
: ---
Assigned To: Wayne Davison
Rsync QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-16 08:43 UTC by Chris Dunlop
Modified: 2011-12-16 17:15 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dunlop 2011-12-16 08:43:18 UTC
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
Comment 1 Wayne Davison 2011-12-16 17:15:19 UTC
Thanks for all that very informative detail!

I believe that the fix is to change the assignment to:

init_stat_x(&real_sx);
real_sx.st = sx.st;

And thus, avoid the duplicating of allocated data.  I made that change in both places where the real_sx = sx assignment was done.  In the first case, it shouldn't change any thing because there shouldn't be any already allocated xattr/acl data on the directory-updating code path (so it was just done for consistency).  In the second case, the real_sx copy is only used for one itemize call, so it should be fine that the call allocates its own data (and seems to be what is expected, since it immediately frees it).

Let me know if you notice any more issues.