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

[Eric Snowberg]

> On Jun 13, 2017, at 6:59 AM, Daniel Kiper <address@hidden> wrote:
> 
> 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?

My patch does not unescape paths, it leaves them alone.  I think there is some 
confusion here.  The example you provided above is currently broken in the 
upstream code.  You would need this patch to get the result you requested.

before patch:
“set a=\f” sets $a to ‘f'

after patch:
“set a=\f” sets $a to ‘\f'


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

But this isn’t the way GRUB was designed:

grub-core/kern/disk.c

/* Return the location of the first ',', if any, which is not
   escaped by a '\'.  */
static const char *
find_part_sep (const char *name)

There are many other places in the code that make this same assumption.  You 
want this changed now?  This will impact every platform.