samba-bugs at samba.org
2011-Dec-16 08:43 UTC
[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.
samba-bugs at samba.org
2011-Dec-16 17:15 UTC
[Bug 8665] Crash in free_xattr(), from recv_generator()
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.
Reasonably Related Threads
- [Bug 8475] New: memory leak around free_xattr() and rsync_xal_free(), with -aX, 200 bytes per file and per directory
- DO NOT REPLY [Bug 7489] New: rsyncd segfaults using daemon exclude filter
- [PATCH] Unsnarl missing_below/dry_run logic.
- Rsync 2.6.9pre2 tries to read ACLs of nonexistent files
- [PATCH v2 1/2] xattrs: Skip security.evm extended attribute