[PATCH] param: fixup quote parsing of kernel arguments

[PATCH] param: fixup quote parsing of kernel arguments

[Arthur Gautier]

When starting kernel with arguments like:
  init=/bin/sh -c "echo arguments"
the trailing double quote is not removed which results in following command
being executed:
  /bin/sh -c 'echo arguments"'

This commit removes the trailing double quote.

Signed-off-by: Arthur Gautier <baloo@gandi.net>
---
 kernel/params.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel/params.c b/kernel/params.c
index 728e05b..2118546 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -156,8 +156,11 @@ static char *next_arg(char *args, char **param, char **val)
 			if (args[i] == '=')
 				equals = i;
 		}
-		if (args[i] == '"')
+		if (args[i] == '"') {
+			if (!equals)
+				args[i] = '\0';
 			in_quote = !in_quote;
+		}
 	}
 
 	*param = args;
-- 
2.1.4

Re: [PATCH] param: fixup quote parsing of kernel arguments

[Rusty Russell]

Arthur Gautier <baloo@gandi.net> writes:
> When starting kernel with arguments like:
>   init=/bin/sh -c "echo arguments"
> the trailing double quote is not removed which results in following command
> being executed:
>   /bin/sh -c 'echo arguments"'
>
> This commit removes the trailing double quote.
>
> Signed-off-by: Arthur Gautier <baloo@gandi.net>

Hi Arthur,

        Thanks, I'd not considered quotes outside '='.  But this
fixes it in a weird way: we handle quotes below, we just don't do
anything for the "raw value" case:

	for (i = 0; args[i]; i++) {
		if (isspace(args[i]) && !in_quote)
			break;
		if (equals == 0) {
			if (args[i] == '=')
				equals = i;
		}
		if (args[i] == '"')
			in_quote = !in_quote;
	}

	*param = args;
	if (!equals)
		*val = NULL;
	else {
		args[equals] = '\0';
		*val = args + equals + 1;

		/* Don't include quotes in value. */
		if (**val == '"') {
			(*val)++;
			if (args[i-1] == '"')
				args[i-1] = '\0';
		}
		if (quoted && args[i-1] == '"')
			args[i-1] = '\0';
	}

The logical fix is to just always remove the close quotes in both
cases:

diff --git a/kernel/params.c b/kernel/params.c
index 728e05b167de..a22d6a759b1a 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -173,9 +173,9 @@ static char *next_arg(char *args, char **param, char **val)
 			if (args[i-1] == '"')
 				args[i-1] = '\0';
 		}
-		if (quoted && args[i-1] == '"')
-			args[i-1] = '\0';
 	}
+	if (quoted && args[i-1] == '"')
+		args[i-1] = '\0';
 
 	if (args[i]) {
 		args[i] = '\0';

Does this work for you?

Thanks,
Rusty.

> ---
>  kernel/params.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/params.c b/kernel/params.c
> index 728e05b..2118546 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -156,8 +156,11 @@ static char *next_arg(char *args, char **param, char **val)
>  			if (args[i] == '=')
>  				equals = i;
>  		}
> -		if (args[i] == '"')
> +		if (args[i] == '"') {
> +			if (!equals)
> +				args[i] = '\0';
>  			in_quote = !in_quote;
> +		}
>  	}
>  
>  	*param = args;
> -- 
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] param: fixup quote parsing of kernel arguments

[Arthur Gautier]

On Wed, Apr 08, 2015 at 03:29:43PM +0930, Rusty Russell wrote:
> Arthur Gautier <baloo@gandi.net> writes:
> > When starting kernel with arguments like:
> >   init=/bin/sh -c "echo arguments"
> > the trailing double quote is not removed which results in following command
> > being executed:
> >   /bin/sh -c 'echo arguments"'
> >
> > This commit removes the trailing double quote.
> >
> > Signed-off-by: Arthur Gautier <baloo@gandi.net>
> 
> Hi Arthur,
> 
>         Thanks, I'd not considered quotes outside '='.  But this
> fixes it in a weird way: we handle quotes below, we just don't do
> anything for the "raw value" case:
> 
> 	for (i = 0; args[i]; i++) {
> 		if (isspace(args[i]) && !in_quote)
> 			break;
> 		if (equals == 0) {
> 			if (args[i] == '=')
> 				equals = i;
> 		}
> 		if (args[i] == '"')
> 			in_quote = !in_quote;
> 	}
> 
> 	*param = args;
> 	if (!equals)
> 		*val = NULL;
> 	else {
> 		args[equals] = '\0';
> 		*val = args + equals + 1;
> 
> 		/* Don't include quotes in value. */
> 		if (**val == '"') {
> 			(*val)++;
> 			if (args[i-1] == '"')
> 				args[i-1] = '\0';
> 		}
> 		if (quoted && args[i-1] == '"')
> 			args[i-1] = '\0';
> 	}
> 
> The logical fix is to just always remove the close quotes in both
> cases:
> 
> diff --git a/kernel/params.c b/kernel/params.c
> index 728e05b167de..a22d6a759b1a 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -173,9 +173,9 @@ static char *next_arg(char *args, char **param, char **val)
>  			if (args[i-1] == '"')
>  				args[i-1] = '\0';
>  		}
> -		if (quoted && args[i-1] == '"')
> -			args[i-1] = '\0';
>  	}
> +	if (quoted && args[i-1] == '"')
> +		args[i-1] = '\0';
>  
>  	if (args[i]) {
>  		args[i] = '\0';
> 
> Does this work for you?
> 

Hi Rusty,

This does indeed fixes my issue and I agree with the fix but I've also
noticed a problem when parsing commands like:

  char * input = "var0=\"val=ue\" \"var1\"=value \"var2=value\" \"echo foo\"";
  char buf[255];
  char *args = buf;
  char * param, *val;

  memcpy(buf, input, strlen(input)+1);

  while(*args)
  {
          args = next_arg(args, &param, &val);
          printf("%s=%s\n", param, val);
  }

This parses commandline like:

  var0=val=ue
  var1"=value
  var2=value
  echo foo=(null)

As you may notice when using doublequote for keys, the final doublequote
is not removed. I'm not sure this should be considered as a problem nor
this should be expected but my patch was fixing this issue as well.

-- 
\o/ Arthur
 G  Gandi.net

Re: [PATCH] param: fixup quote parsing of kernel arguments

[Rusty Russell]

Arthur Gautier <baloo@gandi.net> writes:
> On Wed, Apr 08, 2015 at 03:29:43PM +0930, Rusty Russell wrote:
>> Arthur Gautier <baloo@gandi.net> writes:
>> > When starting kernel with arguments like:
>> >   init=/bin/sh -c "echo arguments"
>> > the trailing double quote is not removed which results in following command
>> > being executed:
>> >   /bin/sh -c 'echo arguments"'
>> >
>> > This commit removes the trailing double quote.
>> >
>> > Signed-off-by: Arthur Gautier <baloo@gandi.net>
>> 
>> Hi Arthur,
>> 
>>         Thanks, I'd not considered quotes outside '='.  But this
>> fixes it in a weird way: we handle quotes below, we just don't do
>> anything for the "raw value" case:
>> 
>> 	for (i = 0; args[i]; i++) {
>> 		if (isspace(args[i]) && !in_quote)
>> 			break;
>> 		if (equals == 0) {
>> 			if (args[i] == '=')
>> 				equals = i;
>> 		}
>> 		if (args[i] == '"')
>> 			in_quote = !in_quote;
>> 	}
>> 
>> 	*param = args;
>> 	if (!equals)
>> 		*val = NULL;
>> 	else {
>> 		args[equals] = '\0';
>> 		*val = args + equals + 1;
>> 
>> 		/* Don't include quotes in value. */
>> 		if (**val == '"') {
>> 			(*val)++;
>> 			if (args[i-1] == '"')
>> 				args[i-1] = '\0';
>> 		}
>> 		if (quoted && args[i-1] == '"')
>> 			args[i-1] = '\0';
>> 	}
>> 
>> The logical fix is to just always remove the close quotes in both
>> cases:
>> 
>> diff --git a/kernel/params.c b/kernel/params.c
>> index 728e05b167de..a22d6a759b1a 100644
>> --- a/kernel/params.c
>> +++ b/kernel/params.c
>> @@ -173,9 +173,9 @@ static char *next_arg(char *args, char **param, char **val)
>>  			if (args[i-1] == '"')
>>  				args[i-1] = '\0';
>>  		}
>> -		if (quoted && args[i-1] == '"')
>> -			args[i-1] = '\0';
>>  	}
>> +	if (quoted && args[i-1] == '"')
>> +		args[i-1] = '\0';
>>  
>>  	if (args[i]) {
>>  		args[i] = '\0';
>> 
>> Does this work for you?
>> 
>
> Hi Rusty,
>
> This does indeed fixes my issue and I agree with the fix but I've also
> noticed a problem when parsing commands like:
>
>   char * input = "var0=\"val=ue\" \"var1\"=value \"var2=value\" \"echo foo\"";
>   char buf[255];
>   char *args = buf;
>   char * param, *val;
>
>   memcpy(buf, input, strlen(input)+1);
>
>   while(*args)
>   {
>           args = next_arg(args, &param, &val);
>           printf("%s=%s\n", param, val);
>   }
>
> This parses commandline like:
>
>   var0=val=ue
>   var1"=value
>   var2=value
>   echo foo=(null)
>
> As you may notice when using doublequote for keys, the final doublequote
> is not removed. I'm not sure this should be considered as a problem nor
> this should be expected but my patch was fixing this issue as well.

Indeed.  And the following isn't parse correctly either:

        foo="one ""two"

Though it *looks* like the code handles this, it doesn't: you'll get
'one ""two'.

Your first case was interesting because it was a more practical example.

I've applied my simple fix for now; if you want to do more thorough quote
handling (ie. use memmove), I'd love to see patches.

Thanks!
Rusty.