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; }

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.