On Mon Feb 10, 2025 at 11:14 PM CET, Harald van Dijk wrote:
> On 10/02/2025 21:46, Sertonix wrote:
>> On Mon Feb 10, 2025 at 10:37 PM CET, Harald van Dijk wrote:
>>> On 10/02/2025 21:15, Sertonix wrote:
>>>> On Mon Feb 10, 2025 at 10:08 PM CET, Harald van Dijk wrote:
>>>>> Hi,
>>>>>
>>>>> On 10/02/2025 19:14, Sertonix wrote:
>>>>>> The test-test example in the comment wasn't actually fixed before as
>>>>>> one can see by running
>>>>>> env -i "test-test=test" busybox ash -c 'export -p'
>>>>>>
>>>>>> This previously incorrectly resulted in
>>>>>> export test
>>>>>
>>>>> You're right that that output is incorrect. However...
>>>>>
>>>>>> But now it correctly results in
>>>>>> export 'test-test=test'
>>>>>
>>>>> ...this is not the correct result either. As the comment in the code
>>>>> says, the output is meant to be usable for 'eval'. This output is not
>>>>> usable for 'eval' and would result in a "bad variable name" error.
>>>>
>>>> This is not the case. I changed the variable name checking to be more
>>>> relaxed for export so it works as expected. You can try this to verify:
>>>> env -i "test-test=test" busybox ash -c 'eval "$(export -p)"'
>>>
>>> Oh, apologies, I missed that, I tested the output in an unpatched
>>> busybox instead. I did not see in your message that you changed that as
>>> well.
>>>
>>> That change still looks wrong: unless there is some special permission
>>> that I am not seeing, 'export test-test=test' is supposed to either
>>> result in an error, or in the specified variable being assigned in the
>>> way that POSIX specifies variable assignments work. The way POSIX
>>> specifies variable assignments work mean the variable behaves like any
>>> other variable. The syntax might not always allow that variable to be
>>> referenced, but consider
>>>
>>> env -i ./busybox ash -c 'export foo=bar baz-=quux; export -p; set'
>>>
>>> *If* the assignment of baz- succeeds, then the requirements for 'export'
>>> say that it becomes a shell variable, and the requirements for 'set' say
>>> it gets printed there as well. That does not happen with your patch.
>>
>> The POSIX doc for set (and only for set) states that "the output shall
>> be suitable for reinput to the shell, setting or resetting, as far as
>> possible". The way I read this is that skipping a variable name that
>> can't be parsed again would be ok.
>
> There is no provision that permits variables to be skipped though. It
> simultaneously requires that all variables be printed, and that they be
> printed in a format that allows reinput into the shell. The
> impossibility of printing baz-=quux in a way that allows reinput into
> the shell does not negate the requirement that "set shall write the
> names and values of all shell variables in the collation sequence of the
> current locale."
The section about "Shell variables" mentions that "Shell variables
shall be initialized only from environment variables that have valid names".
So there is a destinction between shell variables and environment variables.
Unfortunatly it seems like the POSIX spec tries to avoides this discussion
by making some things undefined behavior:
It is unspecified whether environment variables that were passed to the
invoking
shell when it was invoked itself, but were not used to initialize shell
variables
(see 2.5.3 Shell Variables) because they had invalid names, are
included in the
invoked utility's environment.
So handling these names at all might be an extension to the POSIX spec
but one that (in my opinion) is needed for export -p to be used reliably.
> There are two ways around that conflict: either the shell can act in a
> way in which baz- is never accepted as anything that makes it a shell
> variable name, or the shell can add some extended syntax to permit
> variables with such a name to be set so that the set command can print
> them. This would be similar to how you extended the 'export' command to
> print such variable names.
>
> That said, even if you are not convinced on 'set', there is another way
> to show that this patch is problematic: it also affects the printing and
> argument processing of the 'readonly' command. Consider:
>
> $ env test-test=test ./busybox ash -c 'readonly test-test; readonly -p'
> readonly 'test-test=test'
>
> $ ./busybox ash -c "readonly 'test-test=test'"
> ash: readonly: line 0: test-test: bad variable name
You are right that case is still broken. yash (which I took some
inspiration from) seems to allow any name for readonly as well. When
making the change I suggest it would be more consistent to do that for
readonly as well.
>>> I believe my below comment still applies, the variable needs to be
>>> skipped when printing instead.
>>>
>>>>> The variable needs to be skipped when printing instead.
>>>>>
>>>>> Cheers,
>>>>> Harald van Dijk
_______________________________________________
busybox mailing list
[email protected]
https://lists.busybox.net/mailman/listinfo/busybox
