On Fri, 19 May 2017 08:12:08 -0600
Jim Fehlig <jfehlig@xxxxxxxx> wrote:
> On 05/19/2017 06:38 AM, Wim Ten Have wrote:
> > From: Wim ten Have <wim.ten.have@xxxxxxxxxx>
> >
> > Working larger code changes whilst testing functionality and domxml
> > conversion methodology for xen-xl (xenconfig) a cumbersome caveat surfaced
> > that potentially can take libvirtd out with a SEGV when parsing complex
> > disk xl.cfg directives.
> >
> > This patch also includes tests/xlconfigdata adjustments to illustrate
> > specific disk xl.cfg directive and that way updating test 2-ways.
> >
> > Running the tests with defensive code fix all will run without trouble,
> > running it without the code fix testing will trap with below listed
> > debug transcript.
> >
> > <wtenhave@nina:21-ba$h> VIR_TEST_DEBUG=1 ./run gdb ./tests/xlconfigtest
> > TEST: xlconfigtest
> > 1) Xen XL-2-XML Parse fullvirt-ovmf ... OK
> > 2) Xen XL-2-XML Format fullvirt-ovmf ... OK
> > 3) Xen XL-2-XML Parse paravirt-maxvcpus ... OK
> > 4) Xen XL-2-XML Format paravirt-maxvcpus ... OK
> > 5) Xen XL-2-XML Parse new-disk ... OK
> > 6) Xen XL-2-XML Format new-disk ... OK
> > 7) Xen XL-2-XML Format disk-positional-parms-full ... OK
> > 8) Xen XL-2-XML Format disk-positional-parms-partial ... Program received signal SIGSEGV, Segmentation fault.
> >
> > (gdb) where
> > xlcfg=0x66d2b0 "/home/wtenhave/WORK/libvirt/XOSS/BUGS/libvirt/tests/xlconfigdata/test-disk-positional-parms-partial.cfg",
> > xml=0x66d320 "/home/wtenhave/WORK/libvirt/XOSS/BUGS/libvirt/tests/xlconfigdata/test-disk-positional-parms-partial.xml", replaceVars=false) at xlconfigtest.c:152
> > body=0x40f32d <testCompareHelper>, data=0x7fffffffd990) at testutils.c:180
> >
> > (gdb) frame 1
> > 319 if (STRPREFIX(srcstr, "rbd:")) {
> >
> > (gdb) print srcstr
> > $1 = 0x0
>
> I'm always hesitant to critique verbose commit messages, but in this case I
> think the real fix gets lost in the verbosity :-).
critique taken O:-)
> IMO something along the lines of the following would suffice:
Exactly what is coming up.
> xenconfig: fix handling of NULL disk source
>
> It is possible to crash libvirtd when converting xl native config to domXML when
> the xl config contains an empty disk source, e.g. an empty CDROM. Fix by
> checking that the disk source is non-NULL before parsing it.
>
> >
> > Signed-off-by: Wim ten Have <wim.ten.have@xxxxxxxxxx>
> > ---
> > src/xenconfig/xen_xl.c | 3 ++-
> > tests/xlconfigdata/test-disk-positional-parms-partial.cfg | 2 +-
> > tests/xlconfigdata/test-disk-positional-parms-partial.xml | 6 ++++++
> > 3 files changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
> > index 4f24d45..958956a 100644
> > --- a/src/xenconfig/xen_xl.c
> > +++ b/src/xenconfig/xen_xl.c
> > @@ -316,7 +316,8 @@ xenParseXLDiskSrc(virDomainDiskDefPtr disk, char *srcstr)
> > char *tmpstr = NULL;
> > int ret = -1;
> >
> > - if (STRPREFIX(srcstr, "rbd:")) {
> > + if (srcstr &&
> > + STRPREFIX(srcstr, "rbd:")) {
>
> How about checking for a NULL source on entry to this function? Something like
> the below diff?
>
> Looks good otherwise!
>
> Regards,
> Jim
>
> diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
> index 4f24d457c..cac440cd4 100644
> --- a/src/xenconfig/xen_xl.c
> +++ b/src/xenconfig/xen_xl.c
> @@ -316,6 +316,10 @@ xenParseXLDiskSrc(virDomainDiskDefPtr disk, char *srcstr)
> char *tmpstr = NULL;
> int ret = -1;
>
> + /* A NULL source is valid, e.g. an empty CDROM */
> + if (srcstr == NULL)
> + return 0;
> +
> if (STRPREFIX(srcstr, "rbd:")) {
> if (!(tmpstr = virStringReplace(srcstr, "\\\\", "\\")))
> goto cleanup;
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
