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

[Jim Fehlig]

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 :-). IMO something along the lines 
of the following would suffice:

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;