rsync - Dec 2011 - [Bug 8665] New: Crash in free_xattr(), from recv_generator()

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.

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

Wayne Davison <wayned at samba.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED

--- Comment #1 from Wayne Davison <wayned at samba.org> 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.

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