Description of problem: cpio in Rawhide has a memory leak: $ zcat initrd | valgrind --leak-check=full cpio --quiet -id "bin/ls" "bin/rm" "bin/modprobe" "sbin/modprobe" "bin/sh" "bin/bash" "bin/dash" "bin/nash" ==20620== Memcheck, a memory error detector ==20620== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al. ==20620== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info ==20620== Command: cpio --quiet -id bin/ls bin/rm bin/modprobe sbin/modprobe bin/sh bin/bash bin/dash bin/nash ==20620== ==20620== ==20620== HEAP SUMMARY: ==20620== in use at exit: 1,547 bytes in 3 blocks ==20620== total heap usage: 43 allocs, 40 frees, 7,983 bytes allocated ==20620== ==20620== 11 bytes in 1 blocks are definitely lost in loss record 1 of 3 ==20620== at 0x4A06409: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==20620== by 0x416498: xmalloc (xmalloc.c:47) ==20620== by 0x403938: read_in_new_ascii (copyin.c:1155) ==20620== by 0x403D26: read_in_header (copyin.c:1023) ==20620== by 0x4041BF: process_copy_in (copyin.c:1350) ==20620== by 0x4029B0: main (main.c:738) ==20620== ==20620== LEAK SUMMARY: ==20620== definitely lost: 11 bytes in 1 blocks ==20620== indirectly lost: 0 bytes in 0 blocks ==20620== possibly lost: 0 bytes in 0 blocks ==20620== still reachable: 1,536 bytes in 2 blocks ==20620== suppressed: 0 bytes in 0 blocks ==20620== Reachable blocks (those to which a pointer was found) are not shown. ==20620== To see them, rerun with: --leak-check=full --show-reachable=yes ==20620== ==20620== For counts of detected and suppressed errors, rerun with: -v ==20620== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 2 from 2) Version-Release number of selected component (if applicable): cpio-2.11-16.fc19.x86_64 How reproducible: 100% Steps to Reproduce: 1. See valgrind command above.
Created attachment 707074 [details] cpio-2.11-fix-memory-leak-in-copyin.patch Suggested patch to fix the memory leak.
Hello Richard! > Created attachment 707074 [details] > cpio-2.11-fix-memory-leak-in-copyin.patch > > Suggested patch to fix the memory leak. Thanks for looking at cpio code. Your patch seems to be innocent to me (I did not look too much on the code whether there is the NULL check/double free check needed before free) though I'm not sure if it is necessary — this leek is quite not critical because (1) it is in place at most once per cpio run and (1) it is immediately before program exit, iirc. I'm just not sure if it is worth to hit upstream with this issue and applying it downstream seems to be not beneficial to me.. what do you think? I have no problem to apply it in Rawhide (including more security observation around) if you think it is worth to pay attention. Pavel
(In reply to comment #2) > Hello Richard! > > > Created attachment 707074 [details] > > cpio-2.11-fix-memory-leak-in-copyin.patch > > > > Suggested patch to fix the memory leak. > > Thanks for looking at cpio code. Your patch seems to be innocent to me (I > did > not look too much on the code whether there is the NULL check/double free > check > needed before free) though I'm not sure if it is necessary There isn't a check, because it's not needed. free (NULL) is legal: http://stackoverflow.com/questions/1938735/does-freeptr-where-ptr-is-null-corrupt-memory/1938758#1938758 > — this leek is > quite > not critical because (1) it is in place at most once per cpio run and (1) it > is > immediately before program exit, iirc. Although it's not that important, it breaks any program that runs cpio as a subprocess and uses valgrind (which is how I found it -- libguestfs is such a program). You can't turn off valgrind in subprocesses, so it causes the cpio subprocess to fail. > I'm just not sure if it is worth to hit upstream with this issue and applying > it downstream seems to be not beneficial to me.. what do you think? I have > no > problem to apply it in Rawhide (including more security observation around) > if > you think it is worth to pay attention. It'd be great if you could send it upstream and encourage them to run valgrind on their releases in future.
Would there be possible to use something like following command: valgrind --trace-children=yes --trace-children-skip=/usr/bin/cpio Are you forced to inspect children also by valgrind? Pavel
It's a fair question. I'm not using any --trace-children flag at all, and according to the valgrind man page the default is 'no'. However even if we assume the man page is wrong, using --trace-children-skip doesn't seem to help. Below is another example (a memory leak in libvirtd) where you can see I've used --trace-children-skip and it doesn't seem to have had any effect: /home/rjones/d/libguestfs/run --test ../run valgrind --vgdb=no --log-file=/home/rjones/d/libguestfs/valgrind.log --leak-check=full --error-exitcode=119 --suppressions=/home/rjones/d/libguestfs/valgrind-suppressions --trace-children-skip=/usr/sbin/libvirtd ./t/guestfs_400_progress.bc libvir: error : internal error Child process (LC_ALL=C LD_PRELOAD=/usr/lib64/valgrind/vgpreload_core-amd64-linux.so:/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so LD_LIBRARY_PATH=/home/rjones/d/libguestfs/ruby/ext/guestfs:/home/rjones/d/libguestfs/src/.libs:/home/rjones/d/libguestfs/gobject/.libs:/home/rjones/d/libguestfs/ruby/ext/guestfs:/home/rjones/d/libguestfs/src/.libs:/home/rjones/d/libguestfs/gobject/.libs PATH=/home/rjones/d/libguestfs/erlang:/home/rjones/d/libguestfs/erlang:/usr/local/bin:/usr/bin:/usr/local/sbin:/usr/sbin:/home/rjones/.local/bin:/home/rjones/bin HOME=/home/rjones USER=rjones LOGNAME=rjones XDG_RUNTIME_DIR=/run/user/1000 /usr/sbin/libvirtd --timeout=30) unexpected exit status 119 Fatal error: exception Guestfs.Error("could not connect to libvirt (URI = NULL): internal error Child process (LC_ALL=C LD_PRELOAD=/usr/lib64/valgrind/vgpreload_core-amd64-linux.so:/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so LD_LIBRARY_PATH=/home/rjones/d/libguestfs/r /home/rjones/d/libguestfs/run: command failed with exit code 119 FAIL: t/guestfs_400_progress.bc [Rawhide, x86-64]. In any case, I think cpio upstream should fix this memory leak, since the patch is trivial and with the patch cpio is leak free.
> In any case, I think cpio upstream should fix this memory leak, since the > patch is trivial and with the patch cpio is leak free. I'm still thinking about it — and I guess that the best solution for us would be to apply the patch downstream if it causes problems to you. That is easy, no big maintenance issue. Upstream will almost for sure say they are not going to fix it as it is just code change without benefits (bigger code, complication, requires code pushing, ..). The call causing the leak is not a library call & it takes only couple of bytes usually (length of single filename). The main problem with upstream will probably be that the GNU coding standards discourage from doing such changes (and GNU cpio is one of the basic tools maintained by GNU people): ... Memory analysis tools such as valgrind can be useful, but don’t complicate a program merely to avoid their false alarms. For example, if memory is used until just before a process exits, don’t free it simply to silence such a tool. (GNU coding standards - 4.11 Memory Usage) Would you appreciated if I added this code change to Rawhide git? I would be glad if I could help you. Just don't want to disturb upstream with such issue and suggest them to use valgrind :) as they are IMO using it. Otherwise, I would prefer to close this as WONTFIX. > FAIL: t/guestfs_400_progress.bc Could this be relevant to some other problem than with valgrind? Pavel
(In reply to comment #6) At some point we may have to fix it in RHEL, eg. if Coverity finds the same error. So yes, please fix this in Rawhide. > > FAIL: t/guestfs_400_progress.bc > > Could this be relevant to some other problem than with valgrind? This was just an illustrative example of the way that valgrind doesn't let you silence errors in subprocesses. It's not the same problem, because I have fixed my own copy of cpio so I cannot easily reproduce the cpio problem locally.
(In reply to comment #7) > At some point we may have to fix it in RHEL, eg. if Coverity finds > the same error. AFAIK, Coverity does not report memory leaks on program exit by default. It reports a memory leak in: int foo() { void *p = malloc(1U); } ... but not in: int main() { void *p = malloc(1U); }
OK, still then - give it a try - upstream ticket raised (CC-ing reporter (and author of this patch)). Fixed & built in Rawhide ~> CLOSED Pavel
Just an info, I reverted this change due to the bug #921725. The cause of double freeing (or freeing uninitialized memory or static pointer) is not known - for that reason, it is quite hard to make fix for this fix. Pavel