[libvirt] [PATCH v1] xenParseXLDiskSrc: protect against a NULL pointer reference

[Wim ten Have]

On Fri, 19 May 2017 08:12:08 -0600
Jim Fehlig <jfehlig at suse.com> wrote:

> On 05/19/2017 06:38 AM, Wim Ten Have wrote:
> > From: Wim ten Have <wim.ten.have at oracle.com>
> >
> > 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 at 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 at oracle.com>
> > ---
> >  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;
>