Re: [Xen-devel] [TESTDAY] xl cpupool-create segfaults if given invalid configuration

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2012-08-14 at 18:33 +0100, George Dunlap wrote:
> # xl cpupool-create 'name="pool2" sched="credit2"'
> command line:2: config parsing error near `sched': syntax error,
> unexpected IDENT, expecting NEWLINE or ';'
> Failed to parse config file: Invalid argument
> *** glibc detected *** xl: free(): invalid pointer: 0x0000000001a79a10 ***
> Segmentation fault (core dumped)
> 
> Looking at the code in xl_cmdimpl.c:main_cpupoolcreate() it calls:
> * xlu_cfg_init()
> * xlu_cfg_readdata()
> 
> if the readdata() fails, it calls xlu_cfg_destroy() before returning.

I think the issue is the parser has:
        %destructor { xlu__cfg_set_free($$); }  value valuelist values
which frees the current "setting" but does not remove it from the list
of settings.

xlu_cfg_destroy then walks the list of settings and frees it again.

This stuff is more of an Ian J thing but I wonder if when we hit the
syntax error that $$ still refers to the last value parsed, which we
think we are finished with but actually aren't? i.e. we've already
stashed it in the cfg and the reference in $$ is now "stale".

IOW, I wonder if the patch at the end is the right thing to do. It seems
to work for me but I don't know if it is good bison practice or not.

(aside; I had to find and install the Lenny version of bison to make the
autogen diff readable. We should bump this to at least Squeeze in 4.3. I
also trimmed the diff to the generated files to just the relevant
looking bit -- in particular I trimmed a lot of stuff which appeared to
be semi-automated modifications touching the generated files, e.g. the
addition of emacs blocks and removal of page breaks ^L)

> Other callers to readdata() seem to call exit(1) if readdata() fails.

For 4.2 I would be happy with a patch to make this user do the same as a
lower risk fix than changing the parser.

Ian.

diff -r af7143d97fa2 tools/libxl/libxlu_cfg_y.c
--- a/tools/libxl/libxlu_cfg_y.c        Tue Aug 14 15:59:38 2012 +0100
+++ b/tools/libxl/libxlu_cfg_y.c        Wed Aug 15 17:34:25 2012 +0100
@@ -1418,7 +1418,7 @@ yyreduce:
     {
         case 4:
 #line 50 "libxlu_cfg_y.y"
-    { xlu__cfg_set_store(ctx,(yyvsp[(1) - (3)].string),(yyvsp[(3) - 
(3)].setting),(yylsp[(3) - (3)]).first_line); ;}
+    { xlu__cfg_set_store(ctx,(yyvsp[(1) - (3)].string),(yyvsp[(3) - 
(3)].setting),(yylsp[(3) - (3)]).first_line); (yyvsp[(3) - (3)].setting) = 
NULL; ;}
     break;
 
   case 10:
diff -r af7143d97fa2 tools/libxl/libxlu_cfg_y.y
--- a/tools/libxl/libxlu_cfg_y.y        Tue Aug 14 15:59:38 2012 +0100
+++ b/tools/libxl/libxlu_cfg_y.y        Wed Aug 15 17:34:25 2012 +0100
@@ -47,7 +47,7 @@
 file: /* empty */
  |     file setting
 
-setting: IDENT '=' value      { xlu__cfg_set_store(ctx,$1,$3,@3.first_line); }
+setting: IDENT '=' value      { xlu__cfg_set_store(ctx,$1,$3,@3.first_line); 
$3 = NULL; }
                      endstmt
  |      endstmt
  |      error NEWLINE



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel