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

2013/9/20 Daniel P. Berrange <berrange(a)redhat.com&gt;: > 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); /* ************** */