[TESTDAY] xl cpupool-create segfaults if given invalid configuration

[TESTDAY] xl cpupool-create segfaults if given invalid configuration

[George Dunlap]

# 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.

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

So is readdata() leavnig the config in an inconsistent state?  Or is
config already cleaned up?

 -George

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

[Ian Campbell]

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

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

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [TESTDAY] xl cpupool-create segfaults if given invalid configuration"):
> 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".

I will look at this tomorrow, but:

> (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)

Perhaps we should do this now.  I don't think there's any reason to
fear the squeeze version of bison.

Ian.

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

[Ian Campbell]

On Thu, 2012-08-23 at 19:15 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [TESTDAY] xl cpupool-create segfaults if given invalid configuration"):
> > 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".
> 
> I will look at this tomorrow, but:
> 
> > (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)
> 
> Perhaps we should do this now.  I don't think there's any reason to
> fear the squeeze version of bison.

I'd be happy with that.

When regenerating with Lenny's bison I got the following spurious
changes, no doubt due to some automated tree wide cleanup, which would
be nice to dispose of too.

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	Fri Aug 24 10:07:28 2012 +0100
@@ -819,7 +819,7 @@ int yydebug;
 # define YYMAXDEPTH 10000
 #endif
 
-
+\f

 
 #if YYERROR_VERBOSE
 
@@ -1030,7 +1030,7 @@ yysyntax_error (char *yyresult, int yyst
     }
 }
 #endif /* YYERROR_VERBOSE */
-
+\f

 
 /*-----------------------------------------------.
 | Release the memory associated to this symbol.  |
@@ -1101,7 +1101,7 @@ yydestruct (yymsg, yytype, yyvaluep, yyl
 	break;
     }
 }
-
+\f

 
 /* Prevent warnings from -Wmissing-prototypes.  */
 
@@ -1689,11 +1689,3 @@ yyreturn:
 
 
 
-
-/*
- * Local variables:
- * mode: C
- * c-basic-offset: 4
- * indent-tabs-mode: nil
- * End:
- */
diff -r af7143d97fa2 tools/libxl/libxlu_cfg_y.h
--- a/tools/libxl/libxlu_cfg_y.h	Tue Aug 14 15:59:38 2012 +0100
+++ b/tools/libxl/libxlu_cfg_y.h	Fri Aug 24 10:07:28 2012 +0100
@@ -85,11 +85,3 @@ typedef struct YYLTYPE
 #endif
 
 
-
-/*
- * Local variables:
- * mode: C
- * c-basic-offset: 4
- * indent-tabs-mode: nil
- * End:
- */

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

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [TESTDAY] xl cpupool-create segfaults if given invalid configuration"):
> When regenerating with Lenny's bison I got the following spurious
> changes, no doubt due to some automated tree wide cleanup, which would
> be nice to dispose of too.

OK.  Shall I just cause bison to rerun on my squeeze workstation and
commit the result then ?

Ian.

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

[Ian Campbell]

On Fri, 2012-08-24 at 11:46 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [TESTDAY] xl cpupool-create segfaults if given invalid configuration"):
> > When regenerating with Lenny's bison I got the following spurious
> > changes, no doubt due to some automated tree wide cleanup, which would
> > be nice to dispose of too.
> 
> OK.  Shall I just cause bison to rerun on my squeeze workstation and
> commit the result then ?

Please!

Ian,

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

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [TESTDAY] xl cpupool-create segfaults if given invalid configuration"):
> On Fri, 2012-08-24 at 11:46 +0100, Ian Jackson wrote:
> > OK.  Shall I just cause bison to rerun on my squeeze workstation and
> > commit the result then ?
> 
> Please!

In this context, I found this helpful.

Ian.

# HG changeset patch
# User Ian Jackson <Ian.Jackson@eu.citrix.com>
# Date 1345806920 -3600
# Node ID 7769c3a0511edea42140c6c16608e16195a15182
# Parent  4ca40e0559c33205fb5163b10249a0fd5fda39b9
libxl: provide "make realclean" target

This removes all the autogenerated files.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

diff -r 4ca40e0559c3 -r 7769c3a0511e tools/libxl/Makefile
--- a/tools/libxl/Makefile	Thu Aug 23 19:12:28 2012 +0100
+++ b/tools/libxl/Makefile	Fri Aug 24 12:15:20 2012 +0100
@@ -212,8 +212,10 @@ clean:
 	$(RM) -f _*.h *.o *.so* *.a $(CLIENTS) $(DEPS)
 	$(RM) -f _*.c *.pyc _paths.*.tmp _*.api-for-check
 	$(RM) -f testidl.c.new testidl.c
-#	$(RM) -f $(AUTOSRCS) $(AUTOINCS)
 
 distclean: clean
 
+realclean: distclean
+	$(RM) -f $(AUTOSRCS) $(AUTOINCS)
+
 -include $(DEPS)

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

[Ian Campbell]

On Fri, 2012-08-24 at 12:17 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [TESTDAY] xl cpupool-create segfaults if given invalid configuration"):
> > On Fri, 2012-08-24 at 11:46 +0100, Ian Jackson wrote:
> > > OK.  Shall I just cause bison to rerun on my squeeze workstation and
> > > commit the result then ?
> > 
> > Please!
> 
> In this context, I found this helpful.
> 
> Ian.
> 
> # HG changeset patch
> # User Ian Jackson <Ian.Jackson@eu.citrix.com>
> # Date 1345806920 -3600
> # Node ID 7769c3a0511edea42140c6c16608e16195a15182
> # Parent  4ca40e0559c33205fb5163b10249a0fd5fda39b9
> libxl: provide "make realclean" target
> 
> This removes all the autogenerated files.
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> 
> diff -r 4ca40e0559c3 -r 7769c3a0511e tools/libxl/Makefile
> --- a/tools/libxl/Makefile	Thu Aug 23 19:12:28 2012 +0100
> +++ b/tools/libxl/Makefile	Fri Aug 24 12:15:20 2012 +0100
> @@ -212,8 +212,10 @@ clean:
>  	$(RM) -f _*.h *.o *.so* *.a $(CLIENTS) $(DEPS)
>  	$(RM) -f _*.c *.pyc _paths.*.tmp _*.api-for-check
>  	$(RM) -f testidl.c.new testidl.c
> -#	$(RM) -f $(AUTOSRCS) $(AUTOINCS)
>  
>  distclean: clean
>  
> +realclean: distclean
> +	$(RM) -f $(AUTOSRCS) $(AUTOINCS)
> +
>  -include $(DEPS)

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

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [TESTDAY] xl cpupool-create segfaults if given invalid configuration"):
> On Fri, 2012-08-24 at 12:17 +0100, Ian Jackson wrote:
> > Ian Campbell writes ("Re: [Xen-devel] [TESTDAY] xl cpupool-create segfaults if given invalid configuration"):
> > > On Fri, 2012-08-24 at 11:46 +0100, Ian Jackson wrote:
> > > > OK.  Shall I just cause bison to rerun on my squeeze workstation and
> > > > commit the result then ?
> > > 
> > > Please!

Now done.

> > libxl: provide "make realclean" target
> > 
> > This removes all the autogenerated files.
> > 
> > Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

and this also

Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian.

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

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [TESTDAY] xl cpupool-create segfaults if given invalid configuration"):
> 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.

Sorry for this, this is my fault and I have dropped the fix.

> 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; }

I don't think this is correct.  It may happen to work with this
version of bison but I don't think you're allowed to assign to $3.

Looking at the code I think this handling of the XLU_ConfigSettings
and flex is all wrong.

Ian.

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

[Ian Campbell]

On Thu, 2012-08-30 at 16:44 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [TESTDAY] xl cpupool-create segfaults if given invalid configuration"):
> > 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.
> 
> Sorry for this, this is my fault and I have dropped the fix.
> 
> > 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; }
> 
> I don't think this is correct.  It may happen to work with this
> version of bison but I don't think you're allowed to assign to $3.

I suspected this might not be ok.

> Looking at the code I think this handling of the XLU_ConfigSettings
> and flex is all wrong.

Does this mean you know what the right fix is and/or a patch is in
progress?

Ian.

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

[Ian Jackson]

Ian Jackson writes ("Re: [Xen-devel] [TESTDAY] xl cpupool-create segfaults if given invalid configuration"):
> I don't think this is correct.  It may happen to work with this
> version of bison but I don't think you're allowed to assign to $3.

I think this fixes it.

Ian.

From: Ian Jackson <ian.jackson@eu.citrix.com>
Subject: [PATCH] libxl: fix double free on some config parser errors

If libxlu_cfg_y.y encountered a config file error, the code generated
by bison would sometimes _both_ run the %destructor _and_ call
xlu__cfg_set_store for the same XLU_ConfigSetting* semantic value.
The result would be a double free.

This appears to be because of the use of a mid-rule action.  There is
some discussion of the problems with destructors and mid-rule action
error handling in "(bison)Mid-Rule Actions".  This area is complex and
best avoided.

So fix the bug by abolishing the use of a mid-rule action, which was
in any case not necessary here.

Also while we are there rename the nonterminal rule "setting" to
"assignment", to avoid confusion with the token type "setting", which
had an identically name in a different namespace.  This was especially
confusing because the nonterminal "setting" did not have "setting" as
the type of its semantic value!  (In fact the nonterminal, now called
"assignment", does not have a value so it does not have a value type.)

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

---
 tools/libxl/libxlu_cfg_y.c |  120 +++++++++++++++++++++-----------------------
 tools/libxl/libxlu_cfg_y.y |    6 +-
 2 files changed, 61 insertions(+), 65 deletions(-)

diff --git a/tools/libxl/libxlu_cfg_y.c b/tools/libxl/libxlu_cfg_y.c
index 7d1f6c8..5214386 100644
--- a/tools/libxl/libxlu_cfg_y.c
+++ b/tools/libxl/libxlu_cfg_y.c
@@ -380,11 +380,11 @@ union yyalloc
 /* YYNTOKENS -- Number of terminals.  */
 #define YYNTOKENS  12
 /* YYNNTS -- Number of nonterminals.  */
-#define YYNNTS  10
+#define YYNNTS  9
 /* YYNRULES -- Number of rules.  */
-#define YYNRULES  20
+#define YYNRULES  19
 /* YYNRULES -- Number of states.  */
-#define YYNSTATES  29
+#define YYNSTATES  28
 
 /* YYTRANSLATE(YYLEX) -- Bison symbol number corresponding to YYLEX.  */
 #define YYUNDEFTOK  2
@@ -430,28 +430,26 @@ static const yytype_uint8 yytranslate[] =
    YYRHS.  */
 static const yytype_uint8 yyprhs[] =
 {
-       0,     0,     3,     4,     7,     8,    14,    16,    19,    21,
-      23,    25,    30,    32,    34,    35,    37,    41,    44,    50,
-      51
+       0,     0,     3,     4,     7,    12,    14,    17,    19,    21,
+      23,    28,    30,    32,    33,    35,    39,    42,    48,    49
 };
 
 /* YYRHS -- A `-1'-separated list of the rules' RHS.  */
 static const yytype_int8 yyrhs[] =
 {
-      13,     0,    -1,    -1,    13,    14,    -1,    -1,     3,     7,
-      17,    15,    16,    -1,    16,    -1,     1,     6,    -1,     6,
-      -1,     8,    -1,    18,    -1,     9,    21,    19,    10,    -1,
-       4,    -1,     5,    -1,    -1,    20,    -1,    20,    11,    21,
-      -1,    18,    21,    -1,    20,    11,    21,    18,    21,    -1,
-      -1,    21,     6,    -1
+      13,     0,    -1,    -1,    13,    14,    -1,     3,     7,    16,
+      15,    -1,    15,    -1,     1,     6,    -1,     6,    -1,     8,
+      -1,    17,    -1,     9,    20,    18,    10,    -1,     4,    -1,
+       5,    -1,    -1,    19,    -1,    19,    11,    20,    -1,    17,
+      20,    -1,    19,    11,    20,    17,    20,    -1,    -1,    20,
+       6,    -1
 };
 
 /* YYRLINE[YYN] -- source line where rule number YYN was defined.  */
 static const yytype_uint8 yyrline[] =
 {
-       0,    47,    47,    48,    50,    50,    52,    53,    55,    56,
-      58,    59,    61,    62,    64,    65,    66,    68,    69,    71,
-      73
+       0,    47,    47,    48,    50,    52,    53,    55,    56,    58,
+      59,    61,    62,    64,    65,    66,    68,    69,    71,    73
 };
 #endif
 
@@ -461,7 +459,7 @@ static const yytype_uint8 yyrline[] =
 static const char *const yytname[] =
 {
   "$end", "error", "$undefined", "IDENT", "STRING", "NUMBER", "NEWLINE",
-  "'='", "';'", "'['", "']'", "','", "$accept", "file", "setting", "$@1",
+  "'='", "';'", "'['", "']'", "','", "$accept", "file", "assignment",
   "endstmt", "value", "atom", "valuelist", "values", "nlok", 0
 };
 #endif
@@ -479,17 +477,15 @@ static const yytype_uint16 yytoknum[] =
 /* YYR1[YYN] -- Symbol number of symbol that rule YYN derives.  */
 static const yytype_uint8 yyr1[] =
 {
-       0,    12,    13,    13,    15,    14,    14,    14,    16,    16,
-      17,    17,    18,    18,    19,    19,    19,    20,    20,    21,
-      21
+       0,    12,    13,    13,    14,    14,    14,    15,    15,    16,
+      16,    17,    17,    18,    18,    18,    19,    19,    20,    20
 };
 
 /* YYR2[YYN] -- Number of symbols composing right hand side of rule YYN.  */
 static const yytype_uint8 yyr2[] =
 {
-       0,     2,     0,     2,     0,     5,     1,     2,     1,     1,
-       1,     4,     1,     1,     0,     1,     3,     2,     5,     0,
-       2
+       0,     2,     0,     2,     4,     1,     2,     1,     1,     1,
+       4,     1,     1,     0,     1,     3,     2,     5,     0,     2
 };
 
 /* YYDEFACT[STATE-NAME] -- Default rule to reduce with in state
@@ -497,15 +493,15 @@ static const yytype_uint8 yyr2[] =
    means the default is an error.  */
 static const yytype_uint8 yydefact[] =
 {
-       2,     0,     1,     0,     0,     8,     9,     3,     6,     7,
-       0,    12,    13,    19,     4,    10,    14,     0,    20,    19,
-       0,    15,     5,    17,    11,    19,    16,    19,    18
+       2,     0,     1,     0,     0,     7,     8,     3,     5,     6,
+       0,    11,    12,    18,     0,     9,    13,     4,    19,    18,
+       0,    14,    16,    10,    18,    15,    18,    17
 };
 
 /* YYDEFGOTO[NTERM-NUM].  */
 static const yytype_int8 yydefgoto[] =
 {
-      -1,     1,     7,    17,     8,    14,    15,    20,    21,    16
+      -1,     1,     7,     8,    14,    15,    20,    21,    16
 };
 
 /* YYPACT[STATE-NUM] -- Index in YYTABLE of the portion describing
@@ -513,15 +509,15 @@ static const yytype_int8 yydefgoto[] =
 #define YYPACT_NINF -17
 static const yytype_int8 yypact[] =
 {
-     -17,     1,   -17,    -3,     5,   -17,   -17,   -17,   -17,   -17,
-      10,   -17,   -17,   -17,   -17,   -17,    12,     0,   -17,   -17,
-      11,     9,   -17,    16,   -17,   -17,    12,   -17,    16
+     -17,     2,   -17,    -5,    -3,   -17,   -17,   -17,   -17,   -17,
+      10,   -17,   -17,   -17,    14,   -17,    12,   -17,   -17,   -17,
+      11,    -4,     6,   -17,   -17,    12,   -17,     6
 };
 
 /* YYPGOTO[NTERM-NUM].  */
 static const yytype_int8 yypgoto[] =
 {
-     -17,   -17,   -17,   -17,     6,   -17,   -16,   -17,   -17,   -14
+     -17,   -17,   -17,     9,   -17,   -16,   -17,   -17,   -13
 };
 
 /* YYTABLE[YYPACT[STATE-NUM]].  What to do in state STATE-NUM.  If
@@ -531,25 +527,25 @@ static const yytype_int8 yypgoto[] =
 #define YYTABLE_NINF -1
 static const yytype_uint8 yytable[] =
 {
-      19,     2,     3,     9,     4,    23,     5,     5,     6,     6,
-      27,    26,    10,    28,    11,    12,    11,    12,    18,    13,
-      25,    24,    18,    22
+      19,     9,     2,     3,    10,     4,    22,    24,     5,    26,
+       6,    25,    18,    27,    11,    12,    11,    12,    18,    13,
+       5,    23,     6,    17
 };
 
 static const yytype_uint8 yycheck[] =
 {
-      16,     0,     1,     6,     3,    19,     6,     6,     8,     8,
-      26,    25,     7,    27,     4,     5,     4,     5,     6,     9,
-      11,    10,     6,    17
+      16,     6,     0,     1,     7,     3,    19,    11,     6,    25,
+       8,    24,     6,    26,     4,     5,     4,     5,     6,     9,
+       6,    10,     8,    14
 };
 
 /* YYSTOS[STATE-NUM] -- The (internal number of the) accessing
    symbol of state STATE-NUM.  */
 static const yytype_uint8 yystos[] =
 {
-       0,    13,     0,     1,     3,     6,     8,    14,    16,     6,
-       7,     4,     5,     9,    17,    18,    21,    15,     6,    18,
-      19,    20,    16,    21,    10,    11,    21,    18,    21
+       0,    13,     0,     1,     3,     6,     8,    14,    15,     6,
+       7,     4,     5,     9,    16,    17,    20,    15,     6,    17,
+      18,    19,    20,    10,    11,    20,    17,    20
 };
 
 #define yyerrok		(yyerrstatus = 0)
@@ -1081,7 +1077,7 @@ yydestruct (yymsg, yytype, yyvaluep, yylocationp, ctx)
 	{ free((yyvaluep->string)); };
 
 /* Line 1000 of yacc.c  */
-#line 1085 "libxlu_cfg_y.c"
+#line 1081 "libxlu_cfg_y.c"
 	break;
       case 4: /* "STRING" */
 
@@ -1090,7 +1086,7 @@ yydestruct (yymsg, yytype, yyvaluep, yylocationp, ctx)
 	{ free((yyvaluep->string)); };
 
 /* Line 1000 of yacc.c  */
-#line 1094 "libxlu_cfg_y.c"
+#line 1090 "libxlu_cfg_y.c"
 	break;
       case 5: /* "NUMBER" */
 
@@ -1099,43 +1095,43 @@ yydestruct (yymsg, yytype, yyvaluep, yylocationp, ctx)
 	{ free((yyvaluep->string)); };
 
 /* Line 1000 of yacc.c  */
-#line 1103 "libxlu_cfg_y.c"
+#line 1099 "libxlu_cfg_y.c"
 	break;
-      case 17: /* "value" */
+      case 16: /* "value" */
 
 /* Line 1000 of yacc.c  */
 #line 43 "libxlu_cfg_y.y"
 	{ xlu__cfg_set_free((yyvaluep->setting)); };
 
 /* Line 1000 of yacc.c  */
-#line 1112 "libxlu_cfg_y.c"
+#line 1108 "libxlu_cfg_y.c"
 	break;
-      case 18: /* "atom" */
+      case 17: /* "atom" */
 
 /* Line 1000 of yacc.c  */
 #line 40 "libxlu_cfg_y.y"
 	{ free((yyvaluep->string)); };
 
 /* Line 1000 of yacc.c  */
-#line 1121 "libxlu_cfg_y.c"
+#line 1117 "libxlu_cfg_y.c"
 	break;
-      case 19: /* "valuelist" */
+      case 18: /* "valuelist" */
 
 /* Line 1000 of yacc.c  */
 #line 43 "libxlu_cfg_y.y"
 	{ xlu__cfg_set_free((yyvaluep->setting)); };
 
 /* Line 1000 of yacc.c  */
-#line 1130 "libxlu_cfg_y.c"
+#line 1126 "libxlu_cfg_y.c"
 	break;
-      case 20: /* "values" */
+      case 19: /* "values" */
 
 /* Line 1000 of yacc.c  */
 #line 43 "libxlu_cfg_y.y"
 	{ xlu__cfg_set_free((yyvaluep->setting)); };
 
 /* Line 1000 of yacc.c  */
-#line 1139 "libxlu_cfg_y.c"
+#line 1135 "libxlu_cfg_y.c"
 	break;
 
       default:
@@ -1466,67 +1462,67 @@ yyreduce:
         case 4:
 
 /* Line 1455 of yacc.c  */
-#line 50 "libxlu_cfg_y.y"
-    { xlu__cfg_set_store(ctx,(yyvsp[(1) - (3)].string),(yyvsp[(3) - (3)].setting),(yylsp[(3) - (3)]).first_line); ;}
+#line 51 "libxlu_cfg_y.y"
+    { xlu__cfg_set_store(ctx,(yyvsp[(1) - (4)].string),(yyvsp[(3) - (4)].setting),(yylsp[(3) - (4)]).first_line); ;}
     break;
 
-  case 10:
+  case 9:
 
 /* Line 1455 of yacc.c  */
 #line 58 "libxlu_cfg_y.y"
     { (yyval.setting)= xlu__cfg_set_mk(ctx,1,(yyvsp[(1) - (1)].string)); ;}
     break;
 
-  case 11:
+  case 10:
 
 /* Line 1455 of yacc.c  */
 #line 59 "libxlu_cfg_y.y"
     { (yyval.setting)= (yyvsp[(3) - (4)].setting); ;}
     break;
 
-  case 12:
+  case 11:
 
 /* Line 1455 of yacc.c  */
 #line 61 "libxlu_cfg_y.y"
     { (yyval.string)= (yyvsp[(1) - (1)].string); ;}
     break;
 
-  case 13:
+  case 12:
 
 /* Line 1455 of yacc.c  */
 #line 62 "libxlu_cfg_y.y"
     { (yyval.string)= (yyvsp[(1) - (1)].string); ;}
     break;
 
-  case 14:
+  case 13:
 
 /* Line 1455 of yacc.c  */
 #line 64 "libxlu_cfg_y.y"
     { (yyval.setting)= xlu__cfg_set_mk(ctx,0,0); ;}
     break;
 
-  case 15:
+  case 14:
 
 /* Line 1455 of yacc.c  */
 #line 65 "libxlu_cfg_y.y"
     { (yyval.setting)= (yyvsp[(1) - (1)].setting); ;}
     break;
 
-  case 16:
+  case 15:
 
 /* Line 1455 of yacc.c  */
 #line 66 "libxlu_cfg_y.y"
     { (yyval.setting)= (yyvsp[(1) - (3)].setting); ;}
     break;
 
-  case 17:
+  case 16:
 
 /* Line 1455 of yacc.c  */
 #line 68 "libxlu_cfg_y.y"
     { (yyval.setting)= xlu__cfg_set_mk(ctx,2,(yyvsp[(1) - (2)].string)); ;}
     break;
 
-  case 18:
+  case 17:
 
 /* Line 1455 of yacc.c  */
 #line 69 "libxlu_cfg_y.y"
@@ -1536,7 +1532,7 @@ yyreduce:
 
 
 /* Line 1455 of yacc.c  */
-#line 1540 "libxlu_cfg_y.c"
+#line 1536 "libxlu_cfg_y.c"
       default: break;
     }
   YY_SYMBOL_PRINT ("-> $$ =", yyr1[yyn], &yyval, &yyloc);
diff --git a/tools/libxl/libxlu_cfg_y.y b/tools/libxl/libxlu_cfg_y.y
index f0a0559..29aedca 100644
--- a/tools/libxl/libxlu_cfg_y.y
+++ b/tools/libxl/libxlu_cfg_y.y
@@ -45,10 +45,10 @@
 %%
 
 file: /* empty */
- |     file setting
+ |     file assignment
 
-setting: IDENT '=' value      { xlu__cfg_set_store(ctx,$1,$3,@3.first_line); }
-                     endstmt
+assignment: IDENT '=' value endstmt
+                            { xlu__cfg_set_store(ctx,$1,$3,@3.first_line); }
  |      endstmt
  |      error NEWLINE
 
-- 
tg: (5babc64..) t/xen/xl.cfg.mem-fix (depends on: base)

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

[Ian Campbell]

On Thu, 2012-08-30 at 17:49 +0100, Ian Jackson wrote:
> Ian Jackson writes ("Re: [Xen-devel] [TESTDAY] xl cpupool-create segfaults if given invalid configuration"):
> > I don't think this is correct.  It may happen to work with this
> > version of bison but I don't think you're allowed to assign to $3.
> 
> I think this fixes it.

It works for me.
> 
> Ian.
> 
> From: Ian Jackson <ian.jackson@eu.citrix.com>
> Subject: [PATCH] libxl: fix double free on some config parser errors
> 
> If libxlu_cfg_y.y encountered a config file error, the code generated
> by bison would sometimes _both_ run the %destructor _and_ call
> xlu__cfg_set_store for the same XLU_ConfigSetting* semantic value.
> The result would be a double free.
> 
> This appears to be because of the use of a mid-rule action.  There is
> some discussion of the problems with destructors and mid-rule action
> error handling in "(bison)Mid-Rule Actions".  This area is complex and
> best avoided.
> 
> So fix the bug by abolishing the use of a mid-rule action, which was
> in any case not necessary here.
> 
> Also while we are there rename the nonterminal rule "setting" to
> "assignment", to avoid confusion with the token type "setting", which
> had an identically name in a different namespace.  This was especially
> confusing because the nonterminal "setting" did not have "setting" as
> the type of its semantic value!  (In fact the nonterminal, now called
> "assignment", does not have a value so it does not have a value type.)
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

I shall apply in a moment, thanks.