[libvirt] PATCH] Stop double free
This prevents a the following trap. Program received signal SIGABRT, Aborted. 0x00000035ad830265 in raise () from /lib64/libc.so.6 (gdb) bt #0 0x00000035ad830265 in raise () from /lib64/libc.so.6 #1 0x00000035ad831d10 in abort () from /lib64/libc.so.6 #2 0x00000035ad86a84b in __libc_message () from /lib64/libc.so.6 #3 0x00000035ad8722ef in _int_free () from /lib64/libc.so.6. #4 0x00000035ad87273b in free () from /lib64/libc.so.6. #5 0x00000000004066f1 in vshDeinit () #6 0x0000000000406925 in vshError ()) #7 0x0000000000406744 in vshDeinit () #8 0x00000000004130be in main () --- src/virsh.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/virsh.c b/src/virsh.c index 4825f1c..5fc6c8f 100644 --- a/src/virsh.c +++ b/src/virsh.c @@ -8201,7 +8201,7 @@ vshError(vshControl *ctl, int doexit, const char *format, ...) fputc('\n', stderr); if (doexit) { - if (ctl) + if (ctl && ctl->conn) vshDeinit(ctl); exit(EXIT_FAILURE); } -- 1.6.4.1 -- Mark You must be the change you wish to see in the world. -- Mahatma Gandhi Worrying is praying for that you do not wish to happen.
Mark Hamzy wrote:
diff --git a/src/virsh.c b/src/virsh.c index 4825f1c..5fc6c8f 100644 --- a/src/virsh.c +++ b/src/virsh.c @@ -8201,7 +8201,7 @@ vshError(vshControl *ctl, int doexit, const char *format, ...) fputc('\n', stderr);
if (doexit) { - if (ctl) + if (ctl && ctl->conn) vshDeinit(ctl); exit(EXIT_FAILURE); }
I don't think this patch is right. vshDeinit() already has a check for ctl->conn, and if you put it higher up in the call chain like this, you'll leak the ctl->name memory and the ctrl->log_fd file descriptors. Do you have a stack trace with line numbers in it (i.e. debugging information)? Also, what version of libvirt are you using? That might shed a bit more light on what the problem is. -- Chris Lalancette
The stack trace is as follows: Program received signal SIGABRT, Aborted. 0x00000035ad830265 in raise () from /lib64/libc.so.6 (gdb) bt #0 0x00000035ad830265 in raise () from /lib64/libc.so.6 #1 0x00000035ad831d10 in abort () from /lib64/libc.so.6 #2 0x00000035ad86a84b in __libc_message () from /lib64/libc.so.6 #3 0x00000035ad8722ef in _int_free () from /lib64/libc.so.6. #4 0x00000035ad87273b in free () from /lib64/libc.so.6. #5 0x0000000000406771 in vshDeinit (ctl=0x7fffd35d35e0) at virsh.c:8244 #6 0x00000000004069a5 in vshError (ctl=0x7fffd35d35e0, doexit=<value optimized out>, format=0x414f66 "%s") at virsh.c:7861 #7 0x00000000004067c4 in vshDeinit (ctl=0x7fffd35d35e0) at virsh.c:8248 #8 0x000000000041335e in main (argc=3, argv=0x7fffd35d3748) at virsh.c:8493 I am trying to run libvirt-0.7.1-0.1.git3ef2e05.fc12.src.rpm on RHEL5.4. vshDeinit gets called twice, so ctl->name is freed twice. How about this patch then? (See attached file: 0001-Fix-possible-double-free.patch) -- Mark You must be the change you wish to see in the world. -- Mahatma Gandhi Worrying is praying for that you do not wish to happen. Chris Lalancette <clalance@redhat. com> To Mark Hamzy/Austin/IBM@IBMUS 09/16/2009 06:30 cc AM libvir-list@redhat.com Subject Re: [libvirt] PATCH] Stop double free Mark Hamzy wrote:
diff --git a/src/virsh.c b/src/virsh.c index 4825f1c..5fc6c8f 100644 --- a/src/virsh.c +++ b/src/virsh.c @@ -8201,7 +8201,7 @@ vshError(vshControl *ctl, int doexit, const char *format, ...) fputc('\n', stderr);
if (doexit) { - if (ctl) + if (ctl && ctl->conn) vshDeinit(ctl); exit(EXIT_FAILURE); }
I don't think this patch is right. vshDeinit() already has a check for ctl->conn, and if you put it higher up in the call chain like this, you'll leak the ctl->name memory and the ctrl->log_fd file descriptors. Do you have a stack trace with line numbers in it (i.e. debugging information)? Also, what version of libvirt are you using? That might shed a bit more light on what the problem is. -- Chris Lalancette
Mark Hamzy wrote:
The stack trace is as follows:
Program received signal SIGABRT, Aborted. 0x00000035ad830265 in raise () from /lib64/libc.so.6 (gdb) bt #0 0x00000035ad830265 in raise () from /lib64/libc.so.6 #1 0x00000035ad831d10 in abort () from /lib64/libc.so.6 #2 0x00000035ad86a84b in __libc_message () from /lib64/libc.so.6 #3 0x00000035ad8722ef in _int_free () from /lib64/libc.so.6 #4 0x00000035ad87273b in free () from /lib64/libc.so.6 #5 0x0000000000406771 in vshDeinit (ctl=0x7fffd35d35e0) at virsh.c:8244 #6 0x00000000004069a5 in vshError (ctl=0x7fffd35d35e0, doexit=<value optimized out>, format=0x414f66 "%s") at virsh.c:7861 #7 0x00000000004067c4 in vshDeinit (ctl=0x7fffd35d35e0) at virsh.c:8248 #8 0x000000000041335e in main (argc=3, argv=0x7fffd35d3748) at virsh.c:8493
I am trying to run libvirt-0.7.1-0.1.git3ef2e05.fc12.src.rpm on RHEL5.4.
vshDeinit gets called twice, so ctl->name is freed twice.
How about this patch then?
Ah, I see now. Your patch is a workaround. The real problem is that vshDeinit is re-entering itself through: vshDeinit()->vshError()->vshDeinit() While your patch would fix the problem, I'm not sure it's a good long-term solution. Other differences might come up in the future, and trying to worry about vshDeinit being re-entrant is probably not worth the effort. (Indeed, it looks like there were earlier attempts to avoid this, but things have changed since then, breaking the workaround). I think we should make it so that vshDeinit() does not try to re-enter itself. At the moment I don't have a patch, but I would look at either splitting vshError() into vshPrintError() and vshError(), or just doing a couple of fprintf()'s directly in vshDeinit() and not calling vshError() at all (with a comment explaining why). -- Chris Lalancette
Chris Lalancette wrote:
Mark Hamzy wrote:
The stack trace is as follows:
Program received signal SIGABRT, Aborted. 0x00000035ad830265 in raise () from /lib64/libc.so.6 (gdb) bt #0 0x00000035ad830265 in raise () from /lib64/libc.so.6 #1 0x00000035ad831d10 in abort () from /lib64/libc.so.6 #2 0x00000035ad86a84b in __libc_message () from /lib64/libc.so.6 #3 0x00000035ad8722ef in _int_free () from /lib64/libc.so.6 #4 0x00000035ad87273b in free () from /lib64/libc.so.6 #5 0x0000000000406771 in vshDeinit (ctl=0x7fffd35d35e0) at virsh.c:8244 #6 0x00000000004069a5 in vshError (ctl=0x7fffd35d35e0, doexit=<value optimized out>, format=0x414f66 "%s") at virsh.c:7861 #7 0x00000000004067c4 in vshDeinit (ctl=0x7fffd35d35e0) at virsh.c:8248 #8 0x000000000041335e in main (argc=3, argv=0x7fffd35d3748) at virsh.c:8493
I am trying to run libvirt-0.7.1-0.1.git3ef2e05.fc12.src.rpm on RHEL5.4.
vshDeinit gets called twice, so ctl->name is freed twice.
How about this patch then?
Ah, I see now. Your patch is a workaround. The real problem is that vshDeinit is re-entering itself through:
vshDeinit()->vshError()->vshDeinit()
While your patch would fix the problem, I'm not sure it's a good long-term solution. Other differences might come up in the future, and trying to worry about vshDeinit being re-entrant is probably not worth the effort. (Indeed, it looks like there were earlier attempts to avoid this, but things have changed since then, breaking the workaround). I think we should make it so that vshDeinit() does not try to re-enter itself. At the moment I don't have a patch, but I would look at either splitting vshError() into vshPrintError() and vshError(), or just doing a couple of fprintf()'s directly in vshDeinit() and not calling vshError() at all (with a comment explaining why).
I'm seeing this problem in 0.7.1 as well when explicitly providing a connection URI, e.g. 'virsh -c ...'. I cooked up a patch based on your latter suggestion but changed it to the attached after chatting with danpb on IRC. This approach removes the doexit parameter and vshDeinit() call from vshError() altogether, requiring callers of vshError() to exit if they so desire. Regards, Jim
On Mon, Sep 28, 2009 at 04:30:06PM -0600, Jim Fehlig wrote:
Chris Lalancette wrote:
While your patch would fix the problem, I'm not sure it's a good long-term solution. Other differences might come up in the future, and trying to worry about vshDeinit being re-entrant is probably not worth the effort. (Indeed, it looks like there were earlier attempts to avoid this, but things have changed since then, breaking the workaround). I think we should make it so that vshDeinit() does not try to re-enter itself. At the moment I don't have a patch, but I would look at either splitting vshError() into vshPrintError() and vshError(), or just doing a couple of fprintf()'s directly in vshDeinit() and not calling vshError() at all (with a comment explaining why).
I'm seeing this problem in 0.7.1 as well when explicitly providing a connection URI, e.g. 'virsh -c ...'. I cooked up a patch based on your latter suggestion but changed it to the attached after chatting with danpb on IRC. This approach removes the doexit parameter and vshDeinit() call from vshError() altogether, requiring callers of vshError() to exit if they so desire.
Looks fine, better simplify this, applied, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (4)
-
Chris Lalancette -
Daniel Veillard -
Jim Fehlig -
Mark Hamzy