Re: [PATCH] parser: Remove escape from the state transitions

[Daniel Kiper]

On Wed, Jun 07, 2017 at 03:43:34PM -0600, Eric Snowberg wrote:
>
> > On Jun 7, 2017, at 3:36 PM, Daniel Kiper <address@hidden> wrote:
> >
> > On Wed, Jun 07, 2017 at 02:48:04PM -0600, Eric Snowberg wrote:
> >>> On Jun 7, 2017, at 1:23 PM, Daniel Kiper <address@hidden> wrote:
> >>> On Mon, Jun 05, 2017 at 08:29:47AM -0600, Eric Snowberg wrote:
> >>>>> On Jun 2, 2017, at 4:41 AM, Daniel Kiper <address@hidden> wrote:
> >>>>> On Tue, May 30, 2017 at 04:07:52PM -0600, Eric Snowberg wrote:
> >>>>>> Remove GRUB_PARSER_STATE_ESC with state GRUB_PARSER_STATE_TEXT from
> >>>>>> the list of not allowed characters.
> >>>>>>
> >>>>>> This fixes a problem where a properly escaped comma is in the disk 
> >>>>>> path.
> >>>>>>
> >>>>>> For example: 
> >>>>>> /address@hidden/address@hidden/LSI,address@hidden/address@hidden:a
> >>>>>>
> >>>>>> During grub install, the search.fs_uuid is correctly stored in the
> >>>>>> core.img.
> >>>>>>
> >>>>>> As seen here:
> >>>>>>
> >>>>>> 001e380: 7365 6172 6368 2e66 735f 7575 6964 2039  search.fs_uuid 9
> >>>>>> 001e390: 6462 6137 6333 362d 6431 6432 2d34 6163  dba7c36-d1d2-4ac
> >>>>>> 001e3a0: 642d 6135 3037 2d30 3634 6132 3462 3538  d-a507-064a24b58
> >>>>>> 001e3b0: 3666 3420 726f 6f74 2069 6565 6531 3237  6f4 root ieee127
> >>>>>> 001e3c0: 352f 2f70 6369 4033 3036 2f70 6369 4031  
> >>>>>> 5//address@hidden/address@hidden
> >>>>>> 001e3d0: 2f4c 5349 5c2c 6d72 7361 7340 302f 6469  
> >>>>>> /LSI\,address@hidden/di
> >>>>>> 001e3e0: 736b 4030 3a61 200a 7365 7420 7072 6566  address@hidden:a 
> >>>>>> .set pref
> >>>>>> 001e3f0: 6978 3d28 2472 6f6f 7429 272f 6772 7562  ix=($root)'/grub
> >>>>>> 001e400: 3227 0a00 0000 0000 0000 0003 0000 0010  2'..............
> >>>>>> 001e410: 2f67 7275 6232 0000                      /grub2..
> >>>>>>
> >>>>>> Before this change the following args would be sent to
> >>>>>> grub_cmd_do_search:
> >>>>>>
> >>>>>> key: 9dba7c36-d1d2-4acd-a507-064a24b586f4
> >>>>>> var: root
> >>>>>> hint: 
> >>>>>> ieee1275//address@hidden/address@hidden/LSI,address@hidden/address@hidden:a
> >>>>>>
> >>>>>> The hint above is not correct.  It should be:
> >>>>>>
> >>>>>> hint: 
> >>>>>> ieee1275//address@hidden/address@hidden/LSI\,address@hidden/address@hidden:a
> >>>>>>
> >>>>>> Later on, when it tries to use this disk, it incorrectly truncates
> >>>>>> the device name since the comma isn’t escaped and tries to do the
> >>>>>> grub_disk_open with ieee1275//address@hidden/address@hidden/LSI.
> >>>>>>
> >>>>>> Signed-off-by: Eric Snowberg <address@hidden>
> >>>>>> ---
> >>>>>> grub-core/kern/parser.c |    1 -
> >>>>>> 1 files changed, 0 insertions(+), 1 deletions(-)
> >>>>>>
> >>>>>> diff --git a/grub-core/kern/parser.c b/grub-core/kern/parser.c
> >>>>>> index 78175aa..be88baa 100644
> >>>>>> --- a/grub-core/kern/parser.c
> >>>>>> +++ b/grub-core/kern/parser.c
> >>>>>> @@ -30,7 +30,6 @@ static struct grub_parser_state_transition 
> >>>>>> state_transitions[] = {
> >>>>>> {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_QUOTE, '\'', 0},
> >>>>>> {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_DQUOTE, '\"', 0},
> >>>>>> {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_VAR, '$', 0},
> >>>>>> -  {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_ESC, '\\', 0},
> >>>>>
> >>>>> I have a feeling that this way you break parser for everybody.
> >>>>
> >>>> Can you explain what would break on other platforms with this change?
> >>>
> >>> After your patch embedded config is parsed incorrectly, e.g. "set a=\f"
> >>> sets $a to "\f" instead of "f”.
> >>
> >> That is a useful config?
> >
> > Sorry, I do not understand the question.
> >
> >>>>> Could you explain why it is valid for all?
> >>>>
> >>>> Any platform containing a device with a comma in it would have the
> >>>> problem I identified above.
> >>>
> >>> So, I think that you should store unescaped strings in core.img if it is 
> >>> required.
> >>
> >> Throughout GRUB a comma is escaped unless it is before a partition number.
> >> If I store it unescaped in the core.img I would still have the same problem
> >> above when going into grub_disk_open.

Hmmm... Does it (un)escape paths?

> > I understand that this can be a problem for you and I am happy that you wish
> > to fix it. However, I do not accept breakage of the parser as a solution for
> > this problem. At least in such form (by the way, even if we assume that 
> > patch
> > is correct it seems to me incomplete after closer look). Sorry. So, I am 
> > looking
> > forward for another patch(es) which properly fixes the issue.
>
> Could you provide a recommendation on how to fix it then?

I think that we should store and use everywhere if possible unescaped
texts. Escaped versions should be used only if required, e.g. if we
pass something to a shell. On the other hand if we get as an input
escaped form we should unsecape it immediately. This way we avoid
a lot of problems coming from mixing escaped and unescaped forms
in various places.

Daniel