[PATCH] data: Avoid attempting to assign readonly shell vars

[PATCH] data: Avoid attempting to assign readonly shell vars

[Richard Tollerton]

Attempting to set a read-only shell variable kills the shell. This is
required POSIX behavior, it can't be disabled, and it's implemented by
both dash and bash (although it seems to have only started happening in
bash 4.3 and later, and only when run as /bin/sh).

This breaks `bitbake -c devshell` if bash 4.3 is installed, because
BASHOPTS (and a boatload of other shell variables) are readonly.

The fix is to attempt to modify the variable in a subshell before doing
the assignment.

Signed-off-by: Richard Tollerton <rich.tollerton@ni.com>
---
 lib/bb/data.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/bb/data.py b/lib/bb/data.py
index eb628c7..6c8d644 100644
--- a/lib/bb/data.py
+++ b/lib/bb/data.py
@@ -231,6 +231,10 @@ def emit_var(var, o=sys.__stdout__, d = init(), all=False):
         o.write("%s() {\n%s\n}\n" % (varExpanded, val))
         return 1
 
+    # If the variable is readonly, attempting to set it will KILL THE SCRIPT.
+    # Avoid this by attempting to modify the variable in a subshell.
+    o.write('(unset %s 2>/dev/null) &&' % (varExpanded))
+
     if export:
         o.write('export ')
 
-- 
2.1.3

[PATCH] data: escape '$' in shell variable assignment

[Richard Tollerton]

Running bitbake inside make results in the exported environment variable
MAKEOVERRIDES="${-*-command-variables-*-}", which the shell chokes on
when trying to expand it. But of course, it probably shouldn't have been
trying to expand it in the first place -- so just escape the dollar
sign.

Signed-off-by: Richard Tollerton <rich.tollerton@ni.com>
---
 lib/bb/data.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/bb/data.py b/lib/bb/data.py
index 6c8d644..e9d8290 100644
--- a/lib/bb/data.py
+++ b/lib/bb/data.py
@@ -242,6 +242,7 @@ def emit_var(var, o=sys.__stdout__, d = init(), all=False):
     # to a shell, we need to escape the quotes in the var
     alter = re.sub('"', '\\"', val)
     alter = re.sub('\n', ' \\\n', alter)
+    alter = re.sub('\\$', '\\\\$', alter)
     o.write('%s="%s"\n' % (varExpanded, alter))
     return 0
 
-- 
2.1.3

Re: [PATCH] data: Avoid attempting to assign readonly shell vars

[Richard Purdie]

On Wed, 2014-12-17 at 19:11 -0600, Richard Tollerton wrote:
> Attempting to set a read-only shell variable kills the shell. This is
> required POSIX behavior, it can't be disabled, and it's implemented by
> both dash and bash (although it seems to have only started happening in
> bash 4.3 and later, and only when run as /bin/sh).
> 
> This breaks `bitbake -c devshell` if bash 4.3 is installed, because
> BASHOPTS (and a boatload of other shell variables) are readonly.
> 
> The fix is to attempt to modify the variable in a subshell before doing
> the assignment.

Isn't this going to involve fork() calls for every single shell
variable? There must be a better way to do this?

Cheers,

Richard

Re: [PATCH] data: Avoid attempting to assign readonly shell vars

[Richard Tollerton]

Richard Purdie <richard.purdie@linuxfoundation.org> writes:

> On Wed, 2014-12-17 at 19:11 -0600, Richard Tollerton wrote:
>> The fix is to attempt to modify the variable in a subshell before doing
>> the assignment.
>
> Isn't this going to involve fork() calls for every single shell
> variable?

Yes :(

I'm not quite sure how to rigorously check the performance difference;
is there a decent test case that exercises this and only this?

> There must be a better way to do this?

Not unless we rewrite how we set up the environment. My reading of POSIX
is that no query facility exists for determining the read-only status of
a variable [1] [2], and that the only permitted action for attempting to
set a read-only variable is to exit [3] [4].

I suppose that enforcing a hard dependency on bash (via #!/usr/bin/env
bash) could work around this, since this only seems to appear in POSIX
mode; but I'm not yet convinced that the bash execution behavior when
run as bash (vs sh) won't change in the future. Should I bring this up
on bug-bash?

The only *unambiguous* alternative that I see is to rewrite environment
setup to use env. That is, instead of generating:

export A=B
export C=D
...
do_fn () { fn; }
do_fn

We could do:

env - \
A=B \
C=D \
/bin/sh -c 'do_fn() { fn; }; do_fn;'

But obviously that is an extremely intrusive change.

[1] http://pubs.opengroup.org/onlinepubs/009695399/utilities/test.html
[2] http://stackoverflow.com/questions/4440842/test-if-a-variable-is-read-only
[3] http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_09_01
[4]
http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_08_01

> Cheers,
>
> Richard

-- 
Richard Tollerton <rich.tollerton@ni.com>

Re: [PATCH] data: Avoid attempting to assign readonly shell vars

[Richard Purdie]

On Thu, 2014-12-18 at 14:18 -0600, Richard Tollerton wrote:
> Richard Purdie <richard.purdie@linuxfoundation.org> writes:
> > On Wed, 2014-12-17 at 19:11 -0600, Richard Tollerton wrote:
> >> The fix is to attempt to modify the variable in a subshell before doing
> >> the assignment.
> >
> > Isn't this going to involve fork() calls for every single shell
> > variable?
> 
> Yes :(
> 
> I'm not quite sure how to rigorously check the performance difference;
> is there a decent test case that exercises this and only this?
> 
> > There must be a better way to do this?
> 
> Not unless we rewrite how we set up the environment. My reading of POSIX
> is that no query facility exists for determining the read-only status of
> a variable [1] [2], and that the only permitted action for attempting to
> set a read-only variable is to exit [3] [4].
> 
> I suppose that enforcing a hard dependency on bash (via #!/usr/bin/env
> bash) could work around this, since this only seems to appear in POSIX
> mode; but I'm not yet convinced that the bash execution behavior when
> run as bash (vs sh) won't change in the future. Should I bring this up
> on bug-bash?

It might be an idea, it seems crazy that scripts can't tell if a given
action is going to result in their termination.

As for addressing the problem, from my reading of the bash environment
variables, there looks like a finite list of them which could cause
problems, so a blacklist may be an option, albeit an annoying one.

There is a correctness issue here since if we set something, we really
do want it set or an error in many cases.

Cheers,

Richard

Re: [PATCH] data: Avoid attempting to assign readonly shell vars

[Richard Tollerton]

Richard Purdie <richard.purdie@linuxfoundation.org> writes:

> On Thu, 2014-12-18 at 14:18 -0600, Richard Tollerton wrote:
>> Richard Purdie <richard.purdie@linuxfoundation.org> writes:
>> > There must be a better way to do this?
>> 
>> Not unless we rewrite how we set up the environment. My reading of POSIX
>> is that no query facility exists for determining the read-only status of
>> a variable [1] [2], and that the only permitted action for attempting to
>> set a read-only variable is to exit [3] [4].
>> 
>> I suppose that enforcing a hard dependency on bash (via #!/usr/bin/env
>> bash) could work around this, since this only seems to appear in POSIX
>> mode; but I'm not yet convinced that the bash execution behavior when
>> run as bash (vs sh) won't change in the future. Should I bring this up
>> on bug-bash?
>
> It might be an idea, it seems crazy that scripts can't tell if a given
> action is going to result in their termination.

Looks like bash's behavior of not exiting on readonly assignment
attempts is fairly longstanding and explicit, although not particularly
well documented [1]. So yes, switching from sh to bash would work around
this.

[1] http://article.gmane.org/gmane.comp.shells.bash.bugs/18756/


> As for addressing the problem, from my reading of the bash environment
> variables, there looks like a finite list of them which could cause
> problems, so a blacklist may be an option, albeit an annoying one.
>
> There is a correctness issue here since if we set something, we really
> do want it set or an error in many cases.

What do you think about this for a fix:

This only appears to be an issue with code that's using bb.data as a
part of a shell script generation, which AFAIK is only done from
terminal.py to build a run.do_terminal. (Otherwise, nothing would be
building right now.) What if we run e.g. `env -i run.do_terminal`
instead of `run.do_terminal`? That will completely clear the environment
prior to execution.

I'll try to take a stab at implementing this.

> Cheers,
>
> Richard

-- 
Richard Tollerton <rich.tollerton@ni.com>

Re: [PATCH] data: Avoid attempting to assign readonly shell vars

[Richard Tollerton]

Richard Tollerton <rich.tollerton@ni.com> writes:

> What do you think about this for a fix:
>
> This only appears to be an issue with code that's using bb.data as a
> part of a shell script generation, which AFAIK is only done from
> terminal.py to build a run.do_terminal. (Otherwise, nothing would be
> building right now.) What if we run e.g. `env -i run.do_terminal`
> instead of `run.do_terminal`? That will completely clear the environment
> prior to execution.
>
> I'll try to take a stab at implementing this.

Never mind, this was a stupid idea -- bash in POSIX mode continues to set
BASHOPTS under `env -i`, so this buys us nothing.

New idea: Terminal allows passing in an environment directly, which is
currently unused. What if we passed the execution environment in through
that, instead of encoding it into the script?

(Yes, this probably belongs in oe-core more than bitbake-devel at this
point...)


-- 
Richard Tollerton <rich.tollerton@ni.com>