[libvirt] [PATCH] fix crash when starting network

[Stefan Berger]

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