validate/env: add env name begins with digit check

[Mashimiao]

Signed-off-by: Ma Shimiao mashimiao.fnst@cn.fujitsu.com

env name should not begin with a digit

[liangchenye]

LGTM!
@mrunalp is it worth to update the runtime spec as well?

[wking]

On Thu, May 26, 2016 at 03:56:10AM -0700, 梁辰晔 (Liang Chenye) wrote:

@mrunalp is it worth to update the runtime spec as well?

In the in-flight opencontainers/runtime-spec#427, I'm suggesting we
punt to POSIX instead of applying our own restrictions. And in POSIX
1, the charset and leading-digit restrictions are for “Environment
variable names used by the utilities in the Shell and Utilities volume
of IEEE Std 1003.1-2001” and are not a restriction on environment
variables in general. Also from 1:

Other applications may have difficulty dealing with environment
variable names that start with a digit. For this reason, use of such
names is not recommended anywhere.

So it's legal but not recommended. I think we want to land
opencontainers/runtime-spec#427 and follow POSIX, allowing folks to
put whatever they want in these values (but recommending they don't do
anything crazy), and then leave it to them to pick up the pieces if
they do push the envelope.

[Mashimiao]

@wking I think you just past half sentence in POSIX 1, the charset and leading-digit restrictions are for “Environment variable names used by the utilities in the Shell and Utilities volume of IEEE Std 1003.1-2001 consist solely of uppercase letters, digits, and the '_' (underscore) from the characters defined in Portable Character Set and do not begin with a digit.”
It definitely says environment variable names can't begin with a digit.

[liangchenye]

@wking sorry, I did not elaborate clearly. I mean runtime spec misses do not begin with a digit which is restricted in the refereed IEEE Std 1003.1-2001. My previous implementation only based on the runtime spec description (without reading IEEE Std 1003.1-2001).
I think it is also worth to complete this in the runtime spec.

[wking]

On Thu, May 26, 2016 at 06:21:53PM -0700, Ma Shimiao wrote:

I think you just past half sentence in POSIX 1, the charset and
leading-digit restrictions are for “Environment variable names used
by the utilities in the Shell and Utilities volume of IEEE Std
1003.1-2001 consist solely of…
…
It definitely says environment variable names can't begin with a digit.

No, it says “Environment variable names used by the utilities in the
Shell and Utilities volume of IEEE Std 1003.1-2001” (emphasis mine).
And ‘runc’ is not a utility listed in 1.

The “Other applications may have difficulty…” line I quoted earlier
means “you can try using additional characters if you like, but other
applications written by folks who chose to implement the spec narrowly
may not like your choices”. But that's on the user, and if they know
(for whatever reason) that the applications they're working with do
handle additional characters, the POSIX spec is not going to stand in
their way.

[Mashimiao]

@wking yes, runc is not in a utility list. But most configed processes in runc may call or just be utilities in the list. And "Other applications may have difficulty dealing with environment variable names that start with a digit. For this reason, use of such names is not recommended anywhere." I think anywhere that's the point. If nowhere is recommended, then we restrict it that is legal and reasonable.

[wking]

On Thu, May 26, 2016 at 11:09:33PM -0700, Ma Shimiao wrote:

But most configed processes in runc may call or just be utilities in
the list.

The POSIX spec is about the environment variables used by those
utilities. It does not say that they will crash and burn if you pass
them environment variables starting with a leading digit. For
example:

$ env - 123=5 HOME=$HOME env
123=5
HOME=/home/wking

works fine (at least with GNU coreutils 8.23 running on glibc 2.21).

If nowhere is recommended, then we restrict it is OK.

This is the distinction between MUST 1 and SHOULD 2. We can
strongly recommend that restriction (SHOULD), but I don't see a
benefit to requiring that restriction (MUST).

And the wording in opencontainers/runtime-spec#427 is just formal for
“the POSIX folks have thought about this a lot more than we want to.
Just do what they say”.

[Mashimiao]

@wking

$ env - 123=5 HOME=$HOME env
123=5
HOME=/home/wking

This seems OK. But I'm not sure this is the right way to use environment variables.
I think there is no program run as env .... COMMAND
Usually, we need set runtime environment and export environment first, like export test=123
and then use envrionment variables.
from my experience, you can't export a environment name begin with a digit.
$ export 1test=43
bash: export: `1test=43': not a valid identifier
Do you find any system can export environment name begin with a digit?

[wking]

On Fri, May 27, 2016 at 12:07:15AM -0700, Ma Shimiao wrote:

This seems OK. But I'm not sure this is the right way to use
environment variables. I think there is no program run as env .... COMMAND

I have one here 1.

Usually, we need set runtime environment and export environment
first, like export test=123 and then use envrionment variables.

That's one way to set an environment variable in a child process, but
using env(1) is another, equally valid way (and it makes it easier to
clear the old environment).

Do you find any system can export environment name begin with a
digit?

I checked BusyBox v1.24.1 and dash 0.5.7 and neither of them liked
‘export 1test=43’. But Python 3.4.3 has no problem exporting such a
variable:

$ python3 -c "import subprocess as s; s.Popen(['env'], env={'1test': '43'})"
1test=43

I expect the general pattern is “parsing shell commands is complicated
enough without these odd characters, so most shells don't bother. But
in situations where the parsing is easier, utilities won't go out of
their way to reject such names”.

[Mashimiao]

@wking Yes, some programs accept environment variable name begin with digit. But there still some programs can't handle it as POSIX says.
Compared with follow which standard, I think guarantee all configured processes works well with environment variables is more important. and POSIX also says begin with a digit is not recommend anywhere.
Restricting environment variables to be can't begin with digit will not affect those programs who can handle it too much(in my opionion, there is no need you have to use environment variable name begin with digit), but it can help those who can't handle it works well and give a security environment.

[wking]

On Fri, May 27, 2016 at 12:51:35AM -0700, Ma Shimiao wrote:

Compared with follow which standard, I think guarantee all
configured processes works well with environment variables is more
important.

I think a linter that warns folks about this sort of thing is fine,
and we can certainly have ocitools print a warning in this case (with
an equivalent to GCC's -Werror for folks who want ocitools to die in
that case). But I think going beyond POSIX and always erroring in
this case is overly “protective”. Sometimes the user really will know
what they're doing ;).

[Mashimiao]

ping @liangchenye @mrunalp How do you think?

[liangchenye]

As @wking pointed out, IEEE std 1003.1-2001 says clearly that begin with a digit is not a MUST restriction: Other applications may have difficulty dealing with environment variable names that start with a digit. For this reason, use of such names is not recommended anywhere. So @Mashimiao, I think there is no need to adopt this commit and add a warning log is a good solution.

Also I think we need to clarify it in runtime spec.

Environment variable names used by the utilities in the Shell and Utilities volume of IEEE Std 1003.1-2001 consist solely of uppercase letters, digits, and the '_' (underscore) from the characters defined in Portable Character Set and do not begin with a digit.

There is no 'MUST/SHOULD' restriction in the above paragraph. My understanding is: no matter what kind of restriction it has, literally the restriction of uppercase letters, digits, and the '_' should be same with the restriction of do not begin with a digit.
How about change the statement in runtime spec to
The left hand side MUST consist solely of letters, digits, and underscores _ from the characters defined in Portable Character Set and SHOULD not begin with a digit as outlined in IEEE Std 1003.1-2001 ?

[wking]

@liangchenye, how does the language in opencontainers/runtime-spec#427 look to you?

[liangchenye]

@wking I like that :)

[Mashimiao]

As you both think adding warning is better, I agree to updating the PR.

[mrunalp]

s/begin/beginning/

[Mashimiao]

@mrunalp fixed, thanks.

[mrunalp]

LGTM

[Mashimiao]

ping @liangchenye

[liangchenye]

LGTM