[libvirt] [PATCH] fix crash when starting network
commit 27908453 introduces a regression, and it will cause libvirt crashed when starting network. The reason is that tapfd may be NULL, but we dereference it without checking whether it is NULL. --- src/util/bridge.c | 18 ++++++++++-------- 1 files changed, 10 insertions(+), 8 deletions(-) diff --git a/src/util/bridge.c b/src/util/bridge.c index 952f0f3..6774f99 100644 --- a/src/util/bridge.c +++ b/src/util/bridge.c @@ -484,7 +484,8 @@ brAddTap(brControl *ctl, errno = brCreateTap(ctl, ifname, vnet_hdr, tapfd); - if (*tapfd < 0 || errno) + /* fd has been closed in brCreateTap() when it failed. */ + if (errno) goto error; /* We need to set the interface MAC before adding it @@ -494,22 +495,23 @@ brAddTap(brControl *ctl, * device before we set our static MAC. */ if ((errno = brSetInterfaceMac(ctl, *ifname, macaddr))) - goto error; + goto close_fd; /* We need to set the interface MTU before adding it * to the bridge, because the bridge will have its * MTU adjusted automatically when we add the new interface. */ if ((errno = brSetInterfaceMtu(ctl, bridge, *ifname))) - goto error; + goto close_fd; if ((errno = brAddInterface(ctl, bridge, *ifname))) - goto error; + goto close_fd; if (up && ((errno = brSetInterfaceUp(ctl, *ifname, 1)))) - goto error; + goto close_fd; return 0; - error: - VIR_FORCE_CLOSE(*tapfd); - +close_fd: + if (tapfd) + VIR_FORCE_CLOSE(*tapfd); +error: return errno; } -- 1.7.1
On 02.11.2011 06:17, Wen Congyang wrote:
commit 27908453 introduces a regression, and it will cause libvirt crashed when starting network.
The reason is that tapfd may be NULL, but we dereference it without checking whether it is NULL.
--- src/util/bridge.c | 18 ++++++++++-------- 1 files changed, 10 insertions(+), 8 deletions(-)
ACK Michal
On 11/02/2011 01:17 AM, Wen Congyang wrote:
commit 27908453 introduces a regression, and it will cause libvirt crashed when starting network.
The reason is that tapfd may be NULL, but we dereference it without checking whether it is NULL.
--- src/util/bridge.c | 18 ++++++++++-------- 1 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/src/util/bridge.c b/src/util/bridge.c index 952f0f3..6774f99 100644 --- a/src/util/bridge.c +++ b/src/util/bridge.c @@ -484,7 +484,8 @@ brAddTap(brControl *ctl,
errno = brCreateTap(ctl, ifname, vnet_hdr, tapfd);
- if (*tapfd< 0 || errno) + /* fd has been closed in brCreateTap() when it failed. */ + if (errno) goto error;
/* We need to set the interface MAC before adding it @@ -494,22 +495,23 @@ brAddTap(brControl *ctl, * device before we set our static MAC. */ if ((errno = brSetInterfaceMac(ctl, *ifname, macaddr))) - goto error; + goto close_fd; /* We need to set the interface MTU before adding it * to the bridge, because the bridge will have its * MTU adjusted automatically when we add the new interface. */ if ((errno = brSetInterfaceMtu(ctl, bridge, *ifname))) - goto error; + goto close_fd; if ((errno = brAddInterface(ctl, bridge, *ifname))) - goto error; + goto close_fd; if (up&& ((errno = brSetInterfaceUp(ctl, *ifname, 1)))) - goto error; + goto close_fd; return 0;
- error: - VIR_FORCE_CLOSE(*tapfd); - +close_fd: + if (tapfd) + VIR_FORCE_CLOSE(*tapfd); It's not necessary to test for tapfd. This is similar to VIR_FREE(). Besides that you should test for if(tapfd>=0) if testing at all.
Stefan
+error: return errno; }
On 11/02/2011 07:16 PM, Stefan Berger wrote:
On 11/02/2011 01:17 AM, Wen Congyang wrote:
commit 27908453 introduces a regression, and it will cause libvirt crashed when starting network.
The reason is that tapfd may be NULL, but we dereference it without checking whether it is NULL.
--- src/util/bridge.c | 18 ++++++++++-------- 1 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/src/util/bridge.c b/src/util/bridge.c index 952f0f3..6774f99 100644 --- a/src/util/bridge.c +++ b/src/util/bridge.c @@ -484,7 +484,8 @@ brAddTap(brControl *ctl,
errno = brCreateTap(ctl, ifname, vnet_hdr, tapfd);
- if (*tapfd< 0 || errno) + /* fd has been closed in brCreateTap() when it failed. */ + if (errno) goto error;
/* We need to set the interface MAC before adding it @@ -494,22 +495,23 @@ brAddTap(brControl *ctl, * device before we set our static MAC. */ if ((errno = brSetInterfaceMac(ctl, *ifname, macaddr))) - goto error; + goto close_fd; /* We need to set the interface MTU before adding it * to the bridge, because the bridge will have its * MTU adjusted automatically when we add the new interface. */ if ((errno = brSetInterfaceMtu(ctl, bridge, *ifname))) - goto error; + goto close_fd; if ((errno = brAddInterface(ctl, bridge, *ifname))) - goto error; + goto close_fd; if (up&& ((errno = brSetInterfaceUp(ctl, *ifname, 1)))) - goto error; + goto close_fd; return 0;
- error: - VIR_FORCE_CLOSE(*tapfd); - +close_fd: + if (tapfd) + VIR_FORCE_CLOSE(*tapfd); It's not necessary to test for tapfd. This is similar to VIR_FREE(). Besides that you should test for if(tapfd>=0) if testing at all.
Without this check, libvirt may crash. tapfd is pointer, and it is not necessary to check if *tapfd >= 0, but it is necessary to test for tapfd. So I pushed this patch without any change. Thanks Wen Congyang
Stefan
+error: return errno; }
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 11/02/2011 10:25 AM, Wen Congyang wrote:
On 11/02/2011 07:16 PM, Stefan Berger wrote:
On 11/02/2011 01:17 AM, Wen Congyang wrote:
commit 27908453 introduces a regression, and it will cause libvirt crashed when starting network.
The reason is that tapfd may be NULL, but we dereference it without checking whether it is NULL.
--- src/util/bridge.c | 18 ++++++++++-------- 1 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/src/util/bridge.c b/src/util/bridge.c index 952f0f3..6774f99 100644 --- a/src/util/bridge.c +++ b/src/util/bridge.c @@ -484,7 +484,8 @@ brAddTap(brControl *ctl,
errno = brCreateTap(ctl, ifname, vnet_hdr, tapfd);
- if (*tapfd< 0 || errno) + /* fd has been closed in brCreateTap() when it failed. */ + if (errno) goto error;
/* We need to set the interface MAC before adding it @@ -494,22 +495,23 @@ brAddTap(brControl *ctl, * device before we set our static MAC. */ if ((errno = brSetInterfaceMac(ctl, *ifname, macaddr))) - goto error; + goto close_fd; /* We need to set the interface MTU before adding it * to the bridge, because the bridge will have its * MTU adjusted automatically when we add the new interface. */ if ((errno = brSetInterfaceMtu(ctl, bridge, *ifname))) - goto error; + goto close_fd; if ((errno = brAddInterface(ctl, bridge, *ifname))) - goto error; + goto close_fd; if (up&& ((errno = brSetInterfaceUp(ctl, *ifname, 1)))) - goto error; + goto close_fd; return 0;
- error: - VIR_FORCE_CLOSE(*tapfd); - +close_fd: + if (tapfd) + VIR_FORCE_CLOSE(*tapfd); It's not necessary to test for tapfd. This is similar to VIR_FREE(). Besides that you should test for if(tapfd>=0) if testing at all.
Without this check, libvirt may crash. tapfd is pointer, and it is not necessary to check if *tapfd >= 0, but it is necessary to test for tapfd.
So I pushed this patch without any change. You're right..
Stefan
On 11/01/2011 11:17 PM, Wen Congyang wrote:
commit 27908453 introduces a regression, and it will cause libvirt crashed when starting network.
The reason is that tapfd may be NULL, but we dereference it without checking whether it is NULL.
if ((errno = brSetInterfaceMac(ctl, *ifname, macaddr))) - goto error; + goto close_fd;
I don't think we needed the new label...
/* We need to set the interface MTU before adding it * to the bridge, because the bridge will have its * MTU adjusted automatically when we add the new interface. */ if ((errno = brSetInterfaceMtu(ctl, bridge, *ifname))) - goto error; + goto close_fd; if ((errno = brAddInterface(ctl, bridge, *ifname))) - goto error; + goto close_fd; if (up&& ((errno = brSetInterfaceUp(ctl, *ifname, 1)))) - goto error; + goto close_fd; return 0;
- error: - VIR_FORCE_CLOSE(*tapfd); - +close_fd: + if (tapfd) + VIR_FORCE_CLOSE(*tapfd);
That is, just the added if(tapfd) was sufficient to fix the bug, and the old code was still correct in trying to close the fd on all error paths with one less label. But since you've already pushed, we can keep it as is; it's not worth the churn to cut out the new label. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (5)
-
Eric Blake -
Michal Privoznik -
Stefan Berger -
Wen Congyang -
Wen Congyang