2:13 p.m.
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.
6:30 a.m.
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
1:34 p.m.
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(a)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
2:16 a.m.
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
5:30 p.m.
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
6:54 a.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
5533
days inactive
5547
days old
5 comments
4 participants
participants (4)
-
Chris Lalancette -
Daniel Veillard -
Jim Fehlig -
Mark Hamzy