[libvirt] PATCH: better error checking for LOCAL_PEERCRED

[Daniel P. Berrange]

On Sun, Oct 13, 2013 at 08:04:25AM +0100, Brian Candler wrote:
> I was debugging libvirt with OSX today, and got as far as finding
> the problem with LOCAL_PEERCRED, then googled this only to find that
> Ryota Ozaki had fixed the problems a few days ago!
> 
> However you still may find the following patch useful. It tightens
> up the checking in the LOCAL_PEERCRED block, and in particular fixes
> the unlocking of the socket in the error return path for invalid
> groups, by using the same logic from SO_PEERCRED - have a 'goto
> cleanup' in all return paths.
> 
> (Detail: I found that when getsockopt was being called with
> SOL_SOCKET, cr_ngroups was typically <0, probably because it was
> uninitialised. However once the check for this was tightened, it
> hung because the socket wasn't being unlocked on return. So better
> to (a) initialise it to a negative value anyway, and (b) fix the
> return path)
> 
> However I have not checked that NGROUPS is defined on other BSD-like
> systems. You could just have "if (cr.cr_ngroups <= 0)" instead.
> 
> Regards,
> 
> Brian Candler.
> 
> --- src/rpc/virnetsocket.c.orig    2013-10-10 22:37:49.000000000 +0100
> +++ src/rpc/virnetsocket.c    2013-10-12 22:51:57.000000000 +0100
> @@ -1157,8 +1157,10 @@
>  {
>      struct xucred cr;
>      socklen_t cr_len = sizeof(cr);
> +    int ret = -1;
>      virObjectLock(sock);
> 
> +    cr.cr_ngroups = -1;
>  # if defined(__APPLE__)
>      if (getsockopt(sock->fd, SOL_LOCAL, LOCAL_PEERCRED, &cr,
> &cr_len) < 0) {
>  # else
> @@ -1166,20 +1168,19 @@
>  # endif
>          virReportSystemError(errno, "%s",
>                               _("Failed to get client socket identity"));
> -        virObjectUnlock(sock);
> -        return -1;
> +        goto cleanup;
>      }
> 
>      if (cr.cr_version != XUCRED_VERSION) {
>          virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
>                         _("Failed to get valid client socket identity"));
> -        return -1;
> +        goto cleanup;
>      }
> 
> -    if (cr.cr_ngroups == 0) {
> +    if (cr.cr_ngroups <= 0 || cr.cr_ngroups > NGROUPS) {
>          virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
>                         _("Failed to get valid client socket
> identity groups"));
> -        return -1;
> +        goto cleanup;
>      }
> 
>      /* PID and process creation time are not supported on BSDs */
> @@ -1188,8 +1189,11 @@
>      *uid = cr.cr_uid;
>      *gid = cr.cr_gid;
> 
> +    ret = 0;
> +
> +cleanup:
>      virObjectUnlock(sock);
> -    return 0;
> +    return ret;
>  }
>  #else
>  int virNetSocketGetUNIXIdentity(virNetSocketPtr sock ATTRIBUTE_UNUSED,

Unfortunately your patch does not apply since your mail client has
messed up line wrapping. Also there have been conflicting changes
to the code since your patch. I would fix it myself, but I don't
have ability to compile test code on BSD platforms.

Can you update your patch & re-send.

Regards,
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 :|