[PATCH] boot parameters: quoting of environment variables revisited

[PATCH] boot parameters: quoting of environment variables revisited

[Werner Almesberger]

When passing boot parameters, they can be quoted as follows:
param="value"

Unfortunately, when passing environment variables this way, the
quoting causes confusion: in 2.6.7 (etc.), only the variable name
was placed in the environment, which caused it to be ignored.
I've sent a patch that adjusted the name, but this patch was
dropped. Instead, apparently a different fix was attempted in
2.6.9, but this now yields param="value in the environment (note
the embeded double quote), which isn't much better.

I've attached a patch for 2.6.9 that fixes this. This time, I'm
shifting the value. Maybe you like it better this way :-)

- Werner

---------------------------------- cut here -----------------------------------

Signed-off-by: Werner Almesberger <werner@almesberger.net>

--- linux-2.6.9/init/main.c.orig	Tue Oct 19 19:07:45 2004
+++ linux-2.6.9/init/main.c	Tue Oct 19 19:11:05 2004
@@ -310,6 +310,13 @@
 	if (val) {
 		/* Environment option */
 		unsigned int i;
+
+		/* If the value was quoted, shift it. */
+		if (val[-1] == '"') {
+			memmove(val-1,val,strlen(val)+1);
+			val--;
+		}
+
 		for (i = 0; envp_init[i]; i++) {
 			if (i == MAX_INIT_ENVS) {
 				panic_later = "Too many boot env vars at `%s'";

-- 
  _________________________________________________________________________
 / Werner Almesberger, Buenos Aires, Argentina     werner@almesberger.net /
/_http://www.almesberger.net/____________________________________________/

Re: [PATCH] boot parameters: quoting of environment variables revisited

[Andrew Morton]

Werner Almesberger <werner@almesberger.net> wrote:
>
> When passing boot parameters, they can be quoted as follows:
>  param="value"
> 
>  Unfortunately, when passing environment variables this way, the
>  quoting causes confusion: in 2.6.7 (etc.), only the variable name
>  was placed in the environment, which caused it to be ignored.

Bummer.

>  I've sent a patch that adjusted the name, but this patch was
>  dropped. Instead, apparently a different fix was attempted in
>  2.6.9, but this now yields param="value in the environment (note
>  the embeded double quote), which isn't much better.

Yes, Len's patch purported to fix the same thing.  I should have got you to
review&test that change.  Didn't think of it, sorry.

>  I've attached a patch for 2.6.9 that fixes this. This time, I'm
>  shifting the value. Maybe you like it better this way :-)

hm.  The environment string handling and the "command line" string handling
appear to be identical in there.  How come only one of them has the
problem?  That function makes my eyes bleed.

Re: [PATCH] boot parameters: quoting of environment variables revisited

[Werner Almesberger]

Andrew Morton wrote:
> hm.  The environment string handling and the "command line" string handling
> appear to be identical in there.  How come only one of them has the
> problem?  That function makes my eyes bleed.

The joy of "clever" string manipulation, I suppose :-(

The difference between the two branches is that the "command line"
thing uses only the parameter name, which cannot be quoted (well,
at least kernel/params.c doesn't let this happen). So the whole
problem can't occur.

If you quote the parameter name, the quotes will happily end up
in the argument, thanks to kernel/params.c.

Perhaps a better long-term solution would be to fix all this in
kernel/params.c, and remove any quote special-casing from
init/main.c. It just scares me to touch such a highly sensitive
area of the kernel ... :)

- Werner

-- 
  _________________________________________________________________________
 / Werner Almesberger, Buenos Aires, Argentina     werner@almesberger.net /
/_http://www.almesberger.net/____________________________________________/

Re: [PATCH] boot parameters: quoting of environment variables revisited

[Rusty Russell]

On Wed, 2004-10-20 at 08:23, Werner Almesberger wrote:
> When passing boot parameters, they can be quoted as follows:
> param="value"
> 
> Unfortunately, when passing environment variables this way, the
> quoting causes confusion: in 2.6.7 (etc.), only the variable name
> was placed in the environment, which caused it to be ignored.
> I've sent a patch that adjusted the name, but this patch was
> dropped. Instead, apparently a different fix was attempted in
> 2.6.9, but this now yields param="value in the environment (note
> the embeded double quote), which isn't much better.
> 
> I've attached a patch for 2.6.9 that fixes this. This time, I'm
> shifting the value. Maybe you like it better this way :-)

Sorry, I had a patch lying around for this which I didn't send to
Andrew.

AFAICT 2.4 didn't remove quotes, but I have no problem with removing
them now, and for __setup for that matter.  Hope noone relies on it.

This seems to work here...
Rusty.
Name: Remove quotes around environment variables
Status: Booted on 2.6-bk
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

As noticed by Joey Hess (and thanks for Christoph for forwarding it).
Also requirements from Werner Almesberger.

If someone passes 'foo="some value"' the param engine removes the
quotes and hands 'foo' and 'some value'.  The __setup() parameters
expect a single string, and so we try to regenerate it from the two
parts.  Finally, we try to place it as an environment variable.

Werner wants quotes stripped out of the environment variable.  It
makes sense to do that for __setup, too (so it sees 'foo=some value'),
since __setup functions don't usually handle quotes.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .23831-linux-2.6.9-bk3/init/main.c .23831-linux-2.6.9-bk3.updated/init/main.c
--- .23831-linux-2.6.9-bk3/init/main.c	2004-10-19 14:34:23.000000000 +1000
+++ .23831-linux-2.6.9-bk3.updated/init/main.c	2004-10-20 14:48:20.000000000 +1000
@@ -287,8 +287,15 @@ static int __init unknown_bootoption(cha
 {
 	/* Change NUL term back to "=", to make "param" the whole string. */
 	if (val) {
-		if (val[-1] == '"') val[-2] = '=';
-		else val[-1] = '=';
+		/* param=val or param="val"? */
+		if (val == param+strlen(param)+1)
+			val[-1] = '=';
+		else if (val == param+strlen(param)+2) {
+			val[-2] = '=';
+			memmove(val-1, val, strlen(val)+1);
+			val--;
+		} else
+			BUG();
 	}
 
 	/* Handle obsolete-style parameters */

-- 
Anyone who quotes me in their signature is an idiot -- Rusty Russell

Re: [PATCH] boot parameters: quoting of environment variablesrevisited

[Len Brown]

On Wed, 2004-10-20 at 02:21, Rusty Russell wrote:
> On Wed, 2004-10-20 at 08:23, Werner Almesberger wrote:
> > When passing boot parameters, they can be quoted as follows:
> > param="value"
> > 
> > Unfortunately, when passing environment variables this way, the
> > quoting causes confusion: in 2.6.7 (etc.), only the variable name
> > was placed in the environment, which caused it to be ignored.

2.6.8.1 was also broken for kernel parameters.
acpi_os_string="Brand X" resulted in acpi_os_string="" in the kernel.

> > I've sent a patch that adjusted the name, but this patch was
> > dropped. Instead, apparently a different fix was attempted in
> > 2.6.9, but this now yields param="value in the environment (note
> > the embeded double quote), which isn't much better.

In 2.6.9 acpi_os_string="Brand X" correctly results in
acpi_os_string="Brand X" in the kernel -- so at least we got that part
right.

> AFAICT 2.4 didn't remove quotes, but I have no problem with removing
> them now, and for __setup for that matter.  Hope noone relies on it.

I verified that this new patch doesn't break the acpi_os_string="Brand
X" kernel parameter.

I'm not sure what quoted parameters for init's environment are used for,
but it looks like FOO="FOO BAR" now results in
FOO=FOO BAR
in the environment.

thanks,
-Len

Re: [PATCH] boot parameters: quoting of environment variablesrevisited

[Rusty Russell]

On Wed, 2004-10-20 at 17:16, Len Brown wrote:
> I verified that this new patch doesn't break the acpi_os_string="Brand
> X" kernel parameter.

I can't find where acpi_os_string is handled: grepping the latest kernel
gives nothing, but I'd expect the quotes to be stripped.

> I'm not sure what quoted parameters for init's environment are used for,
> but it looks like FOO="FOO BAR" now results in
> FOO=FOO BAR
> in the environment.

Yes, as did Werner's patch.  I don't know if this is right, but it seems
sensible.

Rusty.
-- 
Anyone who quotes me in their signature is an idiot -- Rusty Russell

Re: [PATCH] boot parameters: quoting of environmentvariablesrevisited

[Len Brown]

On Wed, 2004-10-20 at 03:35, Rusty Russell wrote:
> On Wed, 2004-10-20 at 17:16, Len Brown wrote:
> > I verified that this new patch doesn't break the
> acpi_os_string="Brand X" kernel parameter.
> 
> I can't find where acpi_os_string is handled: grepping the latest
> kernel gives nothing, but I'd expect the quotes to be stripped.

s/acpi_os_string/acpi_os_name

go to bed len...

Re: [PATCH] boot parameters: quoting of environmentvariablesrevisited

[Rusty Russell]

On Wed, 2004-10-20 at 17:53, Len Brown wrote:
> On Wed, 2004-10-20 at 03:35, Rusty Russell wrote:
> > On Wed, 2004-10-20 at 17:16, Len Brown wrote:
> > > I verified that this new patch doesn't break the
> > acpi_os_string="Brand X" kernel parameter.
> > 
> > I can't find where acpi_os_string is handled: grepping the latest
> > kernel gives nothing, but I'd expect the quotes to be stripped.
> 
> s/acpi_os_string/acpi_os_name

Ah, looks like that code strips the " anyway, so the fact that we're now
doing it just means it can be simplified.

Cheers!
Rusty.
-- 
Anyone who quotes me in their signature is an idiot -- Rusty Russell

Re: [PATCH] boot parameters: quoting of environmentvariablesrevisited

[Rusty Russell]

On Wed, 2004-10-20 at 17:53, Len Brown wrote:
> On Wed, 2004-10-20 at 03:35, Rusty Russell wrote:
> > On Wed, 2004-10-20 at 17:16, Len Brown wrote:
> > > I verified that this new patch doesn't break the
> > acpi_os_string="Brand X" kernel parameter.

OK, so this is the patch then.  We strip " when we hand to __setup or
put into the environment.

Rusty.
Name: Remove quotes around environment variables
Status: Booted on 2.6-bk
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

As noticed by Joey Hess (and thanks for Christoph for forwarding it).
Also requirements from Werner Almesberger.

If someone passes 'foo="some value"' the param engine removes the
quotes and hands 'foo' and 'some value'.  The __setup() parameters
expect a single string, and so we try to regenerate it from the two
parts.  Finally, we try to place it as an environment variable.

Werner wants quotes stripped out of the environment variable.  It
makes sense to do that for __setup, too (so it sees 'foo=some value'),
since __setup functions don't usually handle quotes.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .23831-linux-2.6.9-bk3/init/main.c .23831-linux-2.6.9-bk3.updated/init/main.c
--- .23831-linux-2.6.9-bk3/init/main.c	2004-10-19 14:34:23.000000000 +1000
+++ .23831-linux-2.6.9-bk3.updated/init/main.c	2004-10-20 14:48:20.000000000 +1000
@@ -287,8 +287,15 @@ static int __init unknown_bootoption(cha
 {
 	/* Change NUL term back to "=", to make "param" the whole string. */
 	if (val) {
-		if (val[-1] == '"') val[-2] = '=';
-		else val[-1] = '=';
+		/* param=val or param="val"? */
+		if (val == param+strlen(param)+1)
+			val[-1] = '=';
+		else if (val == param+strlen(param)+2) {
+			val[-2] = '=';
+			memmove(val-1, val, strlen(val)+1);
+			val--;
+		} else
+			BUG();
 	}
 
 	/* Handle obsolete-style parameters */

-- 
Anyone who quotes me in their signature is an idiot -- Rusty Russell

Re: [PATCH] boot parameters: quoting of environment variablesrevisited

[Werner Almesberger]

Len Brown wrote:
> I'm not sure what quoted parameters for init's environment are used for,
> but it looks like FOO="FOO BAR" now results in
> FOO=FOO BAR
> in the environment.

E.g. when passing data into a UML kernel, it's handy if you can
use parts of the host environment, such as PATH or cwd, even if
the user has a somewhat strange setup, with spaces in them.

That would be for things like "run this test script from my
PATH, using files in the current directory, as 'init' under that
kernel".

- Werner

-- 
  _________________________________________________________________________
 / Werner Almesberger, Buenos Aires, Argentina     werner@almesberger.net /
/_http://www.almesberger.net/____________________________________________/

Re: [PATCH] boot parameters: quoting of environment variables revisited

[Werner Almesberger]

Rusty Russell wrote:
> If someone passes 'foo="some value"' the param engine removes the
> quotes and hands 'foo' and 'some value'.  The __setup() parameters
> expect a single string, and so we try to regenerate it from the two
> parts.

Ah, that's where the fix in 2.6.9 came from :-) Yes, better to
fix it properly for __setup, too. I didn't even pay attention
to the __setup case, but with that added, this really seems to
want to be done in the parser, perhaps along with "foo=b a r",
or such ...

- Werner

-- 
  _________________________________________________________________________
 / Werner Almesberger, Buenos Aires, Argentina     werner@almesberger.net /
/_http://www.almesberger.net/____________________________________________/