[BUGFIX] lexer waits for space to terminate the varname

[BUGFIX] lexer waits for space to terminate the varname

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1.1: Type: text/plain, Size: 163 bytes --]

Hello. A varname may be terminated by any character which isn't in a set
[A-Za-z0-9_] and not only space. Here is a fix

-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #1.2: Type: text/html, Size: 203 bytes --]

[-- Attachment #2: lex_bugfix.diff --]
[-- Type: text/x-patch, Size: 617 bytes --]

diff --git a/kern/parser.c b/kern/parser.c
index e931853..feaee09 100644
--- a/kern/parser.c
+++ b/kern/parser.c
@@ -71,7 +71,10 @@ grub_parser_cmdline_state (grub_parser_state_t state, char c, char *result)
   for (transition = state_transitions; transition->from_state; transition++)
     {
       /* An exact match was found, use it.  */
-      if (transition->from_state == state && transition->input == c)
+      if (transition->from_state == state && 
+	  (transition->input == c || 
+	   (transition->input == ' ' && ! grub_isalpha (c) 
+	    && ! grub_isdigit (c) && c != '_')))
 	{
 	  found = 1;
 	  break;

Re: [BUGFIX] lexer waits for space to terminate the varname

[Bean]

Hi,

I think there is problem with this patch. Consider ${aa}, the closing
character "}" would be left out.

Although you can remedy this by swapping:

  { GRUB_PARSER_STATE_VARNAME, GRUB_PARSER_STATE_TEXT, ' ', 1},
  { GRUB_PARSER_STATE_VARNAME2, GRUB_PARSER_STATE_TEXT, '}', 0},

so that '}' would be handled before ' ', but it's still not a good
practice, as you have added a logic in code that would depend on the
order of state_transitions, which is not apparent for casual code
viewer. Also, ' ' might be used for other transition in the future,
this code could break that. I suggest you use {} to enclose the
variable that doesn't terminated with space, or add the terminating
character explicitly in the transition table:


  { GRUB_PARSER_STATE_VARNAME, GRUB_PARSER_STATE_TEXT, '/ ', 1},


On Sat, May 2, 2009 at 8:02 PM, Vladimir 'phcoder' Serbinenko
<phcoder@gmail.com> wrote:
> Hello. A varname may be terminated by any character which isn't in a set
> [A-Za-z0-9_] and not only space. Here is a fix
>
> --
> Regards
> Vladimir 'phcoder' Serbinenko
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>



-- 
Bean

Re: [BUGFIX] lexer waits for space to terminate the varname

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1882 bytes --]

Hello

On Sat, May 2, 2009 at 4:08 PM, Bean <bean123ch@gmail.com> wrote:

> Hi,
>
> I think there is problem with this patch. Consider ${aa}, the closing
> character "}" would be left out.
>
> Although you can remedy this by swapping:
>
>  { GRUB_PARSER_STATE_VARNAME, GRUB_PARSER_STATE_TEXT, ' ', 1},
>  { GRUB_PARSER_STATE_VARNAME2, GRUB_PARSER_STATE_TEXT, '}', 0},
>
It's not the same starting state. However similar issue exists with ' ' and
'\"'

>
> so that '}' would be handled before ' ', but it's still not a good
> practice, as you have added a logic in code that would depend on the
> order of state_transitions, which is not apparent for casual code
> viewer.

Perhaps we should switch to just ordering the transition rules instead of
separate code for default value

> Also, ' ' might be used for other transition in the future,
> this code could break that. I suggest you use {} to enclose the
> variable that doesn't terminated with space,

It can be a workaround  but the bug is still here. The following pieces of
code fail:
1) echo $hello;
2) echo $hello

or add the terminating
> character explicitly in the transition table:
>
>
>  { GRUB_PARSER_STATE_VARNAME, GRUB_PARSER_STATE_TEXT, '/ ', 1},
>
>
> On Sat, May 2, 2009 at 8:02 PM, Vladimir 'phcoder' Serbinenko
> <phcoder@gmail.com> wrote:
> > Hello. A varname may be terminated by any character which isn't in a set
> > [A-Za-z0-9_] and not only space. Here is a fix
> >
> > --
> > Regards
> > Vladimir 'phcoder' Serbinenko
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > http://lists.gnu.org/mailman/listinfo/grub-devel
> >
> >
>
>
>
> --
> Bean
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: Type: text/html, Size: 3315 bytes --]

Re: [BUGFIX] lexer waits for space to terminate the varname

[Bean]

Hi,

On Sun, May 3, 2009 at 5:12 PM, Vladimir 'phcoder' Serbinenko
<phcoder@gmail.com> wrote:
> Hello
>
> On Sat, May 2, 2009 at 4:08 PM, Bean <bean123ch@gmail.com> wrote:
>>
>> Hi,
>>
>> I think there is problem with this patch. Consider ${aa}, the closing
>> character "}" would be left out.
>>
>> Although you can remedy this by swapping:
>>
>>  { GRUB_PARSER_STATE_VARNAME, GRUB_PARSER_STATE_TEXT, ' ', 1},
>>  { GRUB_PARSER_STATE_VARNAME2, GRUB_PARSER_STATE_TEXT, '}', 0},
>
> It's not the same starting state. However similar issue exists with ' ' and
> '\"'

Oh right, I copy the wrong sample.

>>
>> so that '}' would be handled before ' ', but it's still not a good
>> practice, as you have added a logic in code that would depend on the
>> order of state_transitions, which is not apparent for casual code
>> viewer.
>
> Perhaps we should switch to just ordering the transition rules instead of
> separate code for default value
>>
>> Also, ' ' might be used for other transition in the future,
>> this code could break that. I suggest you use {} to enclose the
>> variable that doesn't terminated with space,
>
> It can be a workaround  but the bug is still here. The following pieces of
> code fail:
> 1) echo $hello;
> 2) echo $hello
>

Yeah, and there are other varieties as well, such as -. Perhaps it'd
be simpler to use your fix for now, but you should add some comment in
the source code to indicate its purpose.

>> or add the terminating
>> character explicitly in the transition table:
>>
>>
>>  { GRUB_PARSER_STATE_VARNAME, GRUB_PARSER_STATE_TEXT, '/ ', 1},
>>
>>
>> On Sat, May 2, 2009 at 8:02 PM, Vladimir 'phcoder' Serbinenko
>> <phcoder@gmail.com> wrote:
>> > Hello. A varname may be terminated by any character which isn't in a set
>> > [A-Za-z0-9_] and not only space. Here is a fix
>> >
>> > --
>> > Regards
>> > Vladimir 'phcoder' Serbinenko
>> >
>> > _______________________________________________
>> > Grub-devel mailing list
>> > Grub-devel@gnu.org
>> > http://lists.gnu.org/mailman/listinfo/grub-devel
>> >
>> >
>>
>>
>>
>> --
>> Bean
>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>
>
> --
> Regards
> Vladimir 'phcoder' Serbinenko
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>



-- 
Bean

Re: [BUGFIX] lexer waits for space to terminate the varname

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 2984 bytes --]

On Sun, May 3, 2009 at 2:37 PM, Bean <bean123ch@gmail.com> wrote:

> Hi,
>
> On Sun, May 3, 2009 at 5:12 PM, Vladimir 'phcoder' Serbinenko
> <phcoder@gmail.com> wrote:
> > Hello
> >
> > On Sat, May 2, 2009 at 4:08 PM, Bean <bean123ch@gmail.com> wrote:
> >>
> >> Hi,
> >>
> >> I think there is problem with this patch. Consider ${aa}, the closing
> >> character "}" would be left out.
> >>
> >> Although you can remedy this by swapping:
> >>
> >>  { GRUB_PARSER_STATE_VARNAME, GRUB_PARSER_STATE_TEXT, ' ', 1},
> >>  { GRUB_PARSER_STATE_VARNAME2, GRUB_PARSER_STATE_TEXT, '}', 0},
> >
> > It's not the same starting state. However similar issue exists with ' '
> and
> > '\"'
>
> Oh right, I copy the wrong sample.
>
> >>
> >> so that '}' would be handled before ' ', but it's still not a good
> >> practice, as you have added a logic in code that would depend on the
> >> order of state_transitions, which is not apparent for casual code
> >> viewer.
> >
> > Perhaps we should switch to just ordering the transition rules instead of
> > separate code for default value
> >>
> >> Also, ' ' might be used for other transition in the future,
> >> this code could break that. I suggest you use {} to enclose the
> >> variable that doesn't terminated with space,
> >
> > It can be a workaround  but the bug is still here. The following pieces
> of
> > code fail:
> > 1) echo $hello;
> > 2) echo $hello
> >
>
> Yeah, and there are other varieties as well, such as -. Perhaps it'd
> be simpler to use your fix for now, but you should add some comment in
> the source code to indicate its purpose.

What about making it rely on order of rules? It should make it more compact

>
>
> >> or add the terminating
> >> character explicitly in the transition table:
> >>
> >>
> >>  { GRUB_PARSER_STATE_VARNAME, GRUB_PARSER_STATE_TEXT, '/ ', 1},
> >>
> >>
> >> On Sat, May 2, 2009 at 8:02 PM, Vladimir 'phcoder' Serbinenko
> >> <phcoder@gmail.com> wrote:
> >> > Hello. A varname may be terminated by any character which isn't in a
> set
> >> > [A-Za-z0-9_] and not only space. Here is a fix
> >> >
> >> > --
> >> > Regards
> >> > Vladimir 'phcoder' Serbinenko
> >> >
> >> > _______________________________________________
> >> > Grub-devel mailing list
> >> > Grub-devel@gnu.org
> >> > http://lists.gnu.org/mailman/listinfo/grub-devel
> >> >
> >> >
> >>
> >>
> >>
> >> --
> >> Bean
> >>
> >>
> >> _______________________________________________
> >> Grub-devel mailing list
> >> Grub-devel@gnu.org
> >> http://lists.gnu.org/mailman/listinfo/grub-devel
> >
> >
> >
> > --
> > Regards
> > Vladimir 'phcoder' Serbinenko
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > http://lists.gnu.org/mailman/listinfo/grub-devel
> >
> >
>
>
>
> --
> Bean
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: Type: text/html, Size: 5034 bytes --]

Re: [BUGFIX] lexer waits for space to terminate the varname

[Bean]

On Sun, May 3, 2009 at 8:39 PM, Vladimir 'phcoder' Serbinenko
<phcoder@gmail.com> wrote:
>
>
> On Sun, May 3, 2009 at 2:37 PM, Bean <bean123ch@gmail.com> wrote:
>>
>> Hi,
>>
>> On Sun, May 3, 2009 at 5:12 PM, Vladimir 'phcoder' Serbinenko
>> <phcoder@gmail.com> wrote:
>> > Hello
>> >
>> > On Sat, May 2, 2009 at 4:08 PM, Bean <bean123ch@gmail.com> wrote:
>> >>
>> >> Hi,
>> >>
>> >> I think there is problem with this patch. Consider ${aa}, the closing
>> >> character "}" would be left out.
>> >>
>> >> Although you can remedy this by swapping:
>> >>
>> >>  { GRUB_PARSER_STATE_VARNAME, GRUB_PARSER_STATE_TEXT, ' ', 1},
>> >>  { GRUB_PARSER_STATE_VARNAME2, GRUB_PARSER_STATE_TEXT, '}', 0},
>> >
>> > It's not the same starting state. However similar issue exists with ' '
>> > and
>> > '\"'
>>
>> Oh right, I copy the wrong sample.
>>
>> >>
>> >> so that '}' would be handled before ' ', but it's still not a good
>> >> practice, as you have added a logic in code that would depend on the
>> >> order of state_transitions, which is not apparent for casual code
>> >> viewer.
>> >
>> > Perhaps we should switch to just ordering the transition rules instead
>> > of
>> > separate code for default value
>> >>
>> >> Also, ' ' might be used for other transition in the future,
>> >> this code could break that. I suggest you use {} to enclose the
>> >> variable that doesn't terminated with space,
>> >
>> > It can be a workaround  but the bug is still here. The following pieces
>> > of
>> > code fail:
>> > 1) echo $hello;
>> > 2) echo $hello
>> >
>>
>> Yeah, and there are other varieties as well, such as -. Perhaps it'd
>> be simpler to use your fix for now, but you should add some comment in
>> the source code to indicate its purpose.
>
> What about making it rely on order of rules? It should make it more compact

Hi,

I'm sorry I don't quite follow, could you give an example ?

-- 
Bean

Re: [BUGFIX] lexer waits for space to terminate the varname

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1.1: Type: text/plain, Size: 2336 bytes --]

On Sun, May 3, 2009 at 2:52 PM, Bean <bean123ch@gmail.com> wrote:

> On Sun, May 3, 2009 at 8:39 PM, Vladimir 'phcoder' Serbinenko
> <phcoder@gmail.com> wrote:
> >
> >
> > On Sun, May 3, 2009 at 2:37 PM, Bean <bean123ch@gmail.com> wrote:
> >>
> >> Hi,
> >>
> >> On Sun, May 3, 2009 at 5:12 PM, Vladimir 'phcoder' Serbinenko
> >> <phcoder@gmail.com> wrote:
> >> > Hello
> >> >
> >> > On Sat, May 2, 2009 at 4:08 PM, Bean <bean123ch@gmail.com> wrote:
> >> >>
> >> >> Hi,
> >> >>
> >> >> I think there is problem with this patch. Consider ${aa}, the closing
> >> >> character "}" would be left out.
> >> >>
> >> >> Although you can remedy this by swapping:
> >> >>
> >> >>  { GRUB_PARSER_STATE_VARNAME, GRUB_PARSER_STATE_TEXT, ' ', 1},
> >> >>  { GRUB_PARSER_STATE_VARNAME2, GRUB_PARSER_STATE_TEXT, '}', 0},
> >> >
> >> > It's not the same starting state. However similar issue exists with '
> '
> >> > and
> >> > '\"'
> >>
> >> Oh right, I copy the wrong sample.
> >>
> >> >>
> >> >> so that '}' would be handled before ' ', but it's still not a good
> >> >> practice, as you have added a logic in code that would depend on the
> >> >> order of state_transitions, which is not apparent for casual code
> >> >> viewer.
> >> >
> >> > Perhaps we should switch to just ordering the transition rules instead
> >> > of
> >> > separate code for default value
> >> >>
> >> >> Also, ' ' might be used for other transition in the future,
> >> >> this code could break that. I suggest you use {} to enclose the
> >> >> variable that doesn't terminated with space,
> >> >
> >> > It can be a workaround  but the bug is still here. The following
> pieces
> >> > of
> >> > code fail:
> >> > 1) echo $hello;
> >> > 2) echo $hello
> >> >
> >>
> >> Yeah, and there are other varieties as well, such as -. Perhaps it'd
> >> be simpler to use your fix for now, but you should add some comment in
> >> the source code to indicate its purpose.
> >
> > What about making it rely on order of rules? It should make it more
> compact
>
> Hi,
>
> I'm sorry I don't quite follow, could you give an example ?
>
Something like this. Compile tested only


>
> --
> Bean
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #1.2: Type: text/html, Size: 4023 bytes --]

[-- Attachment #2: lex_bugfix.diff --]
[-- Type: text/x-diff, Size: 2081 bytes --]

diff --git a/kern/parser.c b/kern/parser.c
index 685ab22..ee6b169 100644
--- a/kern/parser.c
+++ b/kern/parser.c
@@ -47,8 +47,8 @@ static struct grub_parser_state_transition state_transitions[] =
 
   { GRUB_PARSER_STATE_QVAR, GRUB_PARSER_STATE_QVARNAME2, '{', 0},
   { GRUB_PARSER_STATE_QVAR, GRUB_PARSER_STATE_QVARNAME, 0, 1},
-  { GRUB_PARSER_STATE_QVARNAME, GRUB_PARSER_STATE_DQUOTE, ' ', 1},
   { GRUB_PARSER_STATE_QVARNAME, GRUB_PARSER_STATE_TEXT, '\"', 0},
+  { GRUB_PARSER_STATE_QVARNAME, GRUB_PARSER_STATE_DQUOTE, ' ', 1},
   { GRUB_PARSER_STATE_QVARNAME2, GRUB_PARSER_STATE_DQUOTE, '}', 0},
 
   { 0, 0, 0, 0}
@@ -60,9 +60,7 @@ grub_parser_state_t
 grub_parser_cmdline_state (grub_parser_state_t state, char c, char *result)
 {
   struct grub_parser_state_transition *transition;
-  struct grub_parser_state_transition *next_match = 0;
   struct grub_parser_state_transition default_transition;
-  int found = 0;
 
   default_transition.to_state = state;
   default_transition.keep_value = 1;
@@ -70,26 +68,24 @@ grub_parser_cmdline_state (grub_parser_state_t state, char c, char *result)
   /* Look for a good translation.  */
   for (transition = state_transitions; transition->from_state; transition++)
     {
+      if (transition->from_state != state)
+	continue;
       /* An exact match was found, use it.  */
-      if (transition->from_state == state && transition->input == c)
-	{
-	  found = 1;
-	  break;
-	}
+      if (transition->input == c)
+	break;
+
+      if (transition->input == ' ' && ! grub_isalpha (c) 
+	  && ! grub_isdigit (c) && c != '_')
+	break;
 
       /* A less perfect match was found, use this one if no exact
 	 match can be found.  */
-      if (transition->from_state == state && transition->input == 0)
-	next_match = transition;
+      if (transition->input == 0)
+	break;
     }
 
-  if (! found)
-    {
-      if (next_match)
-	transition = next_match;
-      else
-	transition = &default_transition;
-    }
+  if (! transition->from_state)
+    transition = &default_transition;
 
   if (transition->keep_value)
     *result = c;

Re: [BUGFIX] lexer waits for space to terminate the varname

[Bean]

Hi,

Oh right, it looks more compact. Although you should test the corner
cases to make sure no problem is introduced by the new code.

On Sun, May 3, 2009 at 9:28 PM, Vladimir 'phcoder' Serbinenko
<phcoder@gmail.com> wrote:
>
> On Sun, May 3, 2009 at 2:52 PM, Bean <bean123ch@gmail.com> wrote:
>>
>> On Sun, May 3, 2009 at 8:39 PM, Vladimir 'phcoder' Serbinenko
>> <phcoder@gmail.com> wrote:
>> >
>> >
>> > On Sun, May 3, 2009 at 2:37 PM, Bean <bean123ch@gmail.com> wrote:
>> >>
>> >> Hi,
>> >>
>> >> On Sun, May 3, 2009 at 5:12 PM, Vladimir 'phcoder' Serbinenko
>> >> <phcoder@gmail.com> wrote:
>> >> > Hello
>> >> >
>> >> > On Sat, May 2, 2009 at 4:08 PM, Bean <bean123ch@gmail.com> wrote:
>> >> >>
>> >> >> Hi,
>> >> >>
>> >> >> I think there is problem with this patch. Consider ${aa}, the
>> >> >> closing
>> >> >> character "}" would be left out.
>> >> >>
>> >> >> Although you can remedy this by swapping:
>> >> >>
>> >> >>  { GRUB_PARSER_STATE_VARNAME, GRUB_PARSER_STATE_TEXT, ' ', 1},
>> >> >>  { GRUB_PARSER_STATE_VARNAME2, GRUB_PARSER_STATE_TEXT, '}', 0},
>> >> >
>> >> > It's not the same starting state. However similar issue exists with '
>> >> > '
>> >> > and
>> >> > '\"'
>> >>
>> >> Oh right, I copy the wrong sample.
>> >>
>> >> >>
>> >> >> so that '}' would be handled before ' ', but it's still not a good
>> >> >> practice, as you have added a logic in code that would depend on the
>> >> >> order of state_transitions, which is not apparent for casual code
>> >> >> viewer.
>> >> >
>> >> > Perhaps we should switch to just ordering the transition rules
>> >> > instead
>> >> > of
>> >> > separate code for default value
>> >> >>
>> >> >> Also, ' ' might be used for other transition in the future,
>> >> >> this code could break that. I suggest you use {} to enclose the
>> >> >> variable that doesn't terminated with space,
>> >> >
>> >> > It can be a workaround  but the bug is still here. The following
>> >> > pieces
>> >> > of
>> >> > code fail:
>> >> > 1) echo $hello;
>> >> > 2) echo $hello
>> >> >
>> >>
>> >> Yeah, and there are other varieties as well, such as -. Perhaps it'd
>> >> be simpler to use your fix for now, but you should add some comment in
>> >> the source code to indicate its purpose.
>> >
>> > What about making it rely on order of rules? It should make it more
>> > compact
>>
>> Hi,
>>
>> I'm sorry I don't quite follow, could you give an example ?
>
> Something like this. Compile tested only
>
>>
>> --
>> Bean
>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>
>
> --
> Regards
> Vladimir 'phcoder' Serbinenko
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>



-- 
Bean