[libvirt] [PATCH] virNetDevMacVLanVPortProfileRegisterCallback: Fix segfault

[Daniel P. Berrange]

On Tue, Apr 24, 2012 at 05:52:19PM +0200, Michal Privoznik wrote:
> Currently, we are calling memcpy(virtPortProfile, ...) unconditionally.
> Which means if virtPortProfile is NULL we SIGSEGV. Therefore, add check
> to call memcpy() conditionally.
> (gdb) bt
>  #0  virNetDevMacVLanVPortProfileRegisterCallback (ifname=0x7ffff3f1ee60 "macvtap0", macaddress=0x7fffe8096e34 "RT", linkdev=0x7fffe8098b90 "eth0",
>      vmuuid=0x7fffe8099558 "l\220*\237u`\326\213\325\227F\f-B\bD0g\t\350\377\177", virtPortProfile=0x0, vmOp=VIR_NETDEV_VPORT_PROFILE_OP_CREATE) at /usr/include/bits/string3.h:52
>  #1  0x00007ffff7782446 in virNetDevMacVLanCreateWithVPortProfile (tgifname=<optimized out>, macaddress=0x7fffe8096e34 "RT", linkdev=0x7fffe8098b90 "eth0", mode=<optimized out>, withTap=true, vnet_hdr=0,
>      vmuuid=0x7fffe8099558 "l\220*\237u`\326\213\325\227F\f-B\bD0g\t\350\377\177", virtPortProfile=0x0, res_ifname=0x7ffff3f1ef00, vmOp=VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
>      stateDir=0x7fffe800d900 "/var/run/libvirt/qemu", bandwidth=0x0) at util/virnetdevmacvlan.c:954
>  #2  0x00000000004738d4 in qemuPhysIfaceConnect ()
>  #3  0x000000000047e5f3 in qemuBuildCommandLine ()
>  #4  0x000000000049a404 in qemuProcessStart ()
>  #5  0x0000000000468ac9 in qemuDomainObjStart ()
>  #6  0x0000000000469112 in qemuDomainStartWithFlags ()
>  #7  0x00007ffff77ef0c6 in virDomainCreate (domain=0x768cb0) at libvirt.c:8162
>  #8  0x000000000043e158 in remoteDispatchDomainCreateHelper ()
>  #9  0x00007ffff783d835 in virNetServerProgramDispatchCall (msg=0x7fffe80cbe50, client=0x7699d0, server=0x7658a0, prog=0x770ab0) at rpc/virnetserverprogram.c:416
>  #10 virNetServerProgramDispatch (prog=0x770ab0, server=0x7658a0, client=0x7699d0, msg=0x7fffe80cbe50) at rpc/virnetserverprogram.c:289
>  #11 0x00007ffff7839961 in virNetServerHandleJob (jobOpaque=<optimized out>, opaque=0x7658a0) at rpc/virnetserver.c:161
>  #12 0x00007ffff776c1ce in virThreadPoolWorker (opaque=<optimized out>) at util/threadpool.c:144
>  #13 0x00007ffff776b876 in virThreadHelper (data=<optimized out>) at util/threads-pthread.c:161
>  #14 0x00007ffff74f0e2c in start_thread () from /lib64/libpthread.so.0
>  #15 0x00007ffff723766d in clone () from /lib64/libc.so.6
> ---
>  src/util/virnetdevmacvlan.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
> index 879d846..c2fd6d0 100644
> --- a/src/util/virnetdevmacvlan.c
> +++ b/src/util/virnetdevmacvlan.c
> @@ -769,9 +769,11 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname,
>              goto memory_error;
>          if ((calld->cr_ifname = strdup(ifname)) == NULL)
>              goto memory_error;
> -        if (VIR_ALLOC(calld->virtPortProfile) < 0)
> -            goto memory_error;
> -        memcpy(calld->virtPortProfile, virtPortProfile, sizeof(*virtPortProfile));
> +        if (virtPortProfile) {
> +            if (VIR_ALLOC(calld->virtPortProfile) < 0)
> +                goto memory_error;
> +            memcpy(calld->virtPortProfile, virtPortProfile, sizeof(*virtPortProfile));
> +        }
>          if (VIR_ALLOC_N(calld->macaddress, VIR_MAC_BUFLEN) < 0)
>              goto memory_error;
>          memcpy(calld->macaddress, macaddress, VIR_MAC_BUFLEN);

ACK to your change, but while looking at this diff I see two other
design mistakes - using an allocated array for 'macaddress' and
'uuid' is wrong. We should fix that struct so that it uses statically
declared elements of VIR_MAC_BUFLEN and VIR_UUID_BUFLEN instead of
adding pointless allocation


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|