[libvirt] [PATCH v2 1/2] Introduce Libvirt Wireshark dissector

[Daniel P. Berrange]

On Mon, Sep 30, 2013 at 09:12:35PM +0900, Yuto KAWAMURA wrote:
> 2013/9/20 Daniel P. Berrange <berrange at redhat.com>:
> > On Thu, Sep 19, 2013 at 11:26:08PM +0900, Yuto KAWAMURA(kawamuray) wrote:
> >> diff --git a/tools/wireshark/src/moduleinfo.h b/tools/wireshark/src/moduleinfo.h
> >> new file mode 100644
> >> index 0000000..9ab642c
> >> --- /dev/null
> >> +++ b/tools/wireshark/src/moduleinfo.h
> >> @@ -0,0 +1,37 @@
> >> +/* moduleinfo.h --- Define constants about wireshark plugin module
> > ...
> >> +
> >> +/* Included *after* config.h, in order to re-define these macros */
> >> +
> >> +#ifdef PACKAGE
> >> +# undef PACKAGE
> >> +#endif
> >> +
> >> +/* Name of package */
> >> +#define PACKAGE "libvirt"
> >
> > Huh ?  "PACKAGE" will already be defined to 'libvirt' so why are
> > you redefining it.
> >
> >> +
> >> +
> >> +#ifdef VERSION
> >> +# undef VERSION
> >> +#endif
> >> +
> >> +/* Version number of package */
> >> +#define VERSION "0.0.1"
> >
> > This means the wireshark plugin will have a fixed version, even
> > when libvirt protocol changes in new releases. This seems bogus.
> > Again I think we should just use the existing defined "VERSION".
> >
> > I think this whole file can just go away completely
> >
> 
> Right. I'll remove whole moduleinfo.h.
> 
> >> diff --git a/tools/wireshark/src/packet-libvirt.c b/tools/wireshark/src/packet-libvirt.c
> >> new file mode 100644
> >> index 0000000..cd3e6ce
> >> --- /dev/null
> >> +++ b/tools/wireshark/src/packet-libvirt.c
> >> +static gboolean
> >> +dissect_xdr_bytes(tvbuff_t *tvb, proto_tree *tree, XDR *xdrs, int hf,
> >> +                  guint32 maxlen)
> >> +{
> >> +    goffset start;
> >> +    guint8 *val = NULL;
> >> +    guint32 length;
> >> +
> >> +    start = xdr_getpos(xdrs);
> >> +    if (xdr_bytes(xdrs, (char **)&val, &length, maxlen)) {
> >> +        proto_tree_add_bytes_format_value(tree, hf, tvb, start, xdr_getpos(xdrs) - start,
> >> +                                          NULL, "%s", format_xdr_bytes(val, length));
> >> +        /* Seems I can't call xdr_free() for this case.
> >> +           It will raises SEGV by referencing out of bounds argument stack */
> >> +        xdrs->x_op = XDR_FREE;
> >> +        xdr_bytes(xdrs, (char **)&val, &length, maxlen);
> >> +        xdrs->x_op = XDR_DECODE;
> >
> > Is accessing the internals of the 'XDR' struct really portable ? I think
> > it would be desirable to solve the xdr_free problem rather than accessing
> > struct internals
> >
> 
> I'll change this to use free(), but let me explain this problem detail.
> 
> xdr_bytes may raises SEGV when it called from xdr_free.
> This is caused by xdr_free is accessing it's third argument 'sizep' even if
> it was called from xdr_free(in other word, when xdrs->x_op is XDR_FREE).
> This problem can't be reproduced in 64bit architecture due to 64bit
> system's register usage (I'll explain about this later).
> 
> Following is a small enough code to reproduce this issue:
> 
> #include <stdio.h>
> #include <stdlib.h>
> #include <rpc/xdr.h>
> 
> /* Contents of this buffer is not important to reproduce the issue */
> static char xdr_buffer[] = {
>     0x00, 0x00, 0x00, 0x02, /* length is 2byte */
>     'A', '\0', 0, 0         /* 2 byte data and 2 byte padding bytes */
> };
> 
> /* Same as the prototype of xdr_bytes() */
> bool_t my_xdr_bytes(XDR *xdrs, char **cpp, u_int *sizep)
> {
>     return TRUE;
> }
> 
> /* Same as the prototype of xdr_free() */
> void my_xdr_free(xdrproc_t proc, char *objp)
> {
>     XDR x;
>     (*proc)(&x, objp, NULL/* This NULL stored at same pos of 'sizep'
> in xdr_bytes() */);
> }
> 
> int main(void)
> {
>     XDR xdrs;
>     char *opaque = NULL;
>     unsigned int size;
> 
>     xdrmem_create(&xdrs, xdr_buffer, sizeof(xdr_buffer), XDR_DECODE);
>     if (!xdr_bytes(&xdrs, &opaque, &size, ~0)) {
>         fprintf(stderr, "xdr_bytes() returns FALSE\n");
>         exit(1);
>     }
> 
>     /* Reproduce same stack-upping as call of xdr_free(xdr_bytes, &opaque).
>        This is needed to stack-up 0x0(invalid address) on position of
>        'sizsp' which is third argument of xdr_bytes(). */
>     my_xdr_free((xdrproc_t)my_xdr_bytes, (char *)&opaque);
> 
>     /* *** SEGV!! *** */
>     xdr_free((xdrproc_t)xdr_bytes, (char *)&opaque);
>     /* ************** */

Ok, the scenario here is

 - 'xdr_bytes' takes 4 arguments
 - 'xdrproc_t' takes 2 mandatory args + var-args
 - 'xdr_free' calls the 'xdrproc_t' function with only 2 arguments
 - 'xdr_bytes' unconditionally accesses its 3rd argument

So either

 - the cast from xdr_bytes -> xdrproc_t is invalid and thus
   xdr_bytes should not be used with xdr_free.

or

 - xdr_bytes impl in glibc is buggy and shouldn't access the
   3rd arg except when doing encode/decode operations.

Regardless of which is right, we want our code to work on all xdr
impls, so we must avoid problem 2. So I think we should just not
use xdr_free here. Just do a plain 'free(opaque)' instead.

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