[Bug 8665] New: Crash in free_xattr(), from recv_generator()

[samba-bugs at samba.org]

https://bugzilla.samba.org/show_bug.cgi?id=8665

           Summary: Crash in free_xattr(), from recv_generator()
           Product: rsync
           Version: 3.1.0
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: normal
          Priority: P5
         Component: core
        AssignedTo: wayned at samba.org
        ReportedBy: chris at onthe.net.au
         QAContact: rsync-qa at samba.org


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

-- 
Configure bugmail: https://bugzilla.samba.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.