Bug 8475 - memory leak around free_xattr() and rsync_xal_free(), with -aX, 200 bytes per file and per directory
memory leak around free_xattr() and rsync_xal_free(), with -aX, 200 bytes per...
Status: RESOLVED FIXED
Product: rsync
Classification: Unclassified
Component: core
3.0.9
All All
: P5 normal
: ---
Assigned To: Wayne Davison
Rsync QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-22 01:59 UTC by Tim Taiwanese Liim
Modified: 2011-09-23 15:07 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 Tim Taiwanese Liim 2011-09-22 01:59:14 UTC
Description of problem:
    With flag "-aX", rsync creates xattr data structure, but does not free
    it when done.  This leads to 200 bytes of memory leak for each file
    and each dir, or a few giga for my big backup job.

    In free_xattr(), we have
        rsync_xal_free(sxp->xattr);
        free(sxp->xattr);
        sxp->xattr = NULL;
    In rsync_xal_free() we should also free the pointer to allocated memory:
        rsync_xa *rxas = xalp->items;  // "xalp" is "sxp->xattr"
        free(rxas);            <== need to add this line
        xalp->items = NULL;    <== for good practice.
        xalp->count = 0;
    because xalp->items comes from malloc() earlier in expand_item_list():
        new_ptr = _realloc_array(...); // wrapper of malloc() and realloc()
        lp->items = new_ptr;

Version-Release number of selected component (if applicable):
    rsync-3.0.9pre2.tar.gz
    rsync-3.0.8-1.fc14.x86_64

How reproducible:
    always, with "-aX" flag.

Steps to Reproduce:
1. create a few source files
    d1=/tmp/t1
    d2=/tmp/t2
    mkdir $d1
    cd    $d1
    for i in {1..40}; do mkdir -p $i; echo 123 > $i/a$i; cp $i/a$i $i/b$i; done

2. use valgrind to check for memory leak
    rm -rf $d2; 
    valgrind --leak-check=full rsync -aX $d1/ $d2/

Actual results:
    valgrind indicates memory leak.

Expected results:
    no memory leak, at least no big ones.

Additional info:
  - valgrind reports 200 bytes leak for each file processed:
    16,000 bytes in 80 blocks are definitely lost in loss record 27 of 30
       at 0x4A05E46: malloc (vg_replace_malloc.c:195)
       by 0x41DFD7: _realloc_array (util.c:1438)
       by 0x41EC36: expand_item_list (util.c:1690)
       by 0x43C3DE: rsync_xal_get (xattrs.c:268)
       by 0x43C602: get_xattr (xattrs.c:309)
       by 0x4076EF: send_file_name (flist.c:1433)
       by 0x40817D: send_directory (flist.c:1673)
       by 0x408991: send1extra (flist.c:1827)
       by 0x408D23: send_extra_file_list (flist.c:1899)
       by 0x417345: send_files (sender.c:184)
       by 0x42110A: client_run (main.c:1039)
       by 0x421B9B: start_client (main.c:1287)

  - There is another unrelated leak at much lower rate; 
    109 bytes in 39 blocks are definitely lost in loss record 17 of 30
       at 0x4A05E46: malloc (vg_replace_malloc.c:195)
       by 0x41DF8F: _new_array (util.c:1430)
       by 0x406D00: make_file (flist.c:1215)
       by 0x40725B: send_file_name (flist.c:1333)
       by 0x40817D: send_directory (flist.c:1673)
       by 0x408991: send1extra (flist.c:1827)
       by 0x408D23: send_extra_file_list (flist.c:1899)
       by 0x417345: send_files (sender.c:184)
       by 0x42110A: client_run (main.c:1039)
       by 0x421B9B: start_client (main.c:1287)
       by 0x4221E0: main (main.c:1514)
   Just FYI; not sure if you want to fix this one.
Comment 1 Tim Taiwanese Liim 2011-09-22 02:00:20 UTC
This issue was oroginally reported at
    https://bugzilla.redhat.com/show_bug.cgi?id=739794
    memory leak around free_xattr() and rsync_xal_free(), 200 bytes
    per file and per directory
Comment 2 Wayne Davison 2011-09-22 16:35:42 UTC
Looks like rsync_xal_free() was originally written to allow re-use of the allocated memory (it leaves the malloced value unzeroed along with the "items" memory), but only one of the 2 callers was freeing the item memory, and neither was expecting it to stick around.

I have committed a fix that frees the memory and also avoids calling free on the "items" list if no allocations were done.  I decided to get rid of the memory clearing, since no callers expect to re-use the allocated area -- one discards the memory, and the other pre-clears it (using EMPTY_ITEM_LIST) before each new/reused item is used.
Comment 3 Tim Taiwanese Liim 2011-09-23 15:07:10 UTC
Wayne,
Thanks for fast action!  In which release will we see this change?