[PATCH 4/5] sandbox: implement reset

[Heinrich Schuchardt]

On 27.10.20 15:36, Rasmus Villemoes wrote:
> On 27/10/2020 14.33, Heinrich Schuchardt wrote:
>> On 27.10.20 13:12, Rasmus Villemoes wrote:
>>> On 25/10/2020 07.04, Heinrich Schuchardt wrote:
>>>> Up to now the sandbox would shutdown upon a cold reset request. Instead it
>>>> should be reset.
>>>>
>>>> In our coding we use static variables. The only safe way to return to an
>>>> initial state is to relaunch the U-Boot binary.
>>>>
>>>> The reset implementation uses a longjmp() to return to the main() function
>>>> and then relaunches U-Boot using execv().
>>>>
>>>
>>> That seems to be needlessly fragile.
>>>
>>> 1. getopt_long can permute the elements of the argv array
>>> 2. From reading "man longjmp", I'm not sure argc and argv are actually
>>> guaranteed to have the values they had originally upon reaching the
>>> setjmp() the second time
>>>
>>> Now, 1. is probably mostly when there's a mix of options and positional
>>> arguments, and ./u-boot doesn't take the latter. And 2. possibly also
>>> doesn't apply because we don't currently modify argc or argv in main()
>>> itself - but that could change with some future refactoring.
>>>
>>> So perhaps it works, and maybe that's even guaranteed with the current
>>> code and APIs that are used. But, is there any reason to muck with a
>>> complex beast like setjmp/longjmp when we could just
>>
>> 1) argc is passed by value.
>
> Yes, but which value. To be concrete, on x86-64, upon entry to main(),
> the OS-supplied value of argc is in %rdi. When you get to setjmp() the
> second time, how do you know that the value of argc you use to call
> os_relaunch matches that original value?
>
>> argv is defined as "char * const argv[]". It is interesting that
>> getopt_long() ignores the const keyword which explicitly forbids
>> permuting the sequence. Cf.
>> https://sourceware.org/git/?p=glibc.git;a=blob;f=posix/getopt.c;h=8d251dd5fabb46e17f5d593bf12d0619a8867bee;hb=HEAD#l730
>>
>> But even if the sequence is permuted why should the result be different
>> if getopt_long is called again?
>
> It probably would not be different. But why risk some hard-to-debug
> scenario? There are indeed some cases where setjmp/longjmp is the best
> solution to a problem. I really don't see that this is one of those.

Could you, please, explain how you would do a cold reset on the sandbox
without longjmp().

Take this example: U-Boot calls an EFI binary which calls the Reset()
service in the UEFI API implemented via efi_reset_system_boottime().

Best regards

Heinrich

>
>> 2) longjmp() restores all registers including the stack pointer.
>
> Citation needed. man longjmp says "longjmp() may restore the values of
> other registers in addition to the stack pointer and program counter",
> which certainly doesn't suggest that it unconditionally restores every
> register to the state it had at the point of setjmp().
>
> Again, I think it all works currently, but only because we don't do
> something that modifies argc or argv after the setjmp call. POSIX
> (https://pubs.opengroup.org/onlinepubs/9699919799/):
>
> ... except that the values of objects of automatic storage duration are
> unspecified if they meet all the following conditions:
>
>     They are local to the function containing the corresponding setjmp()
> invocation.
>
>     They do not have volatile-qualified type.
>
>     They are changed between the setjmp() invocation and longjmp() call.
>
> argc and argv satisfy the first two bullets. Anybody adding something to
> main() somewhere after that setjmp() call that modifies argc or argv
> (not the pointed-to things, just argv itself) would tick off that last
> bullet. So that would at the very least warrant a big comment above the
> setjmp() that argc and argv are not to be modfied within main().
>
>
>> architecture. Local variables() in main are also on the stack.
>
> No, they are not. That may be how a compiler from 1980 would do things.
>
>>> (one can argue whether memcpy'ing the argv array is sufficient, or if
>>> one should really strdup() each element, since one is allowed to modify
>>> the strings, though again, I don't think we do that currently).
>>
>> No C program is allowed to change the strings passed to main() in argv.
>
> Wrong. C99, 5.1.2.2.1  Program startup:
>
> The  parameters argc and argv and  *the  strings  pointed  to  by  the
> argv array*  shall be  modifiable  by  the  program,  and  retain  their
>  last-stored  values  between  program startup and program termination.
>
> (emphasis mine). This is a perfectly compliant program:
>
> #include <string.h>
> int main(int argc, char *argv[])
> {
>   if (argc) memset(argv[0], '#', strlen(argv[0]));
>   return 0;
> }
>
> [it also so happens that on linux, that modification ends up being
> visible in /proc/$pid/cmdline].

This shows why you should not change argv[].

Yes, u-boot/sandbox does not make use of
> that currently, and it's hard to think of a reason to do stuff like that.
>
>> Unfortunately Simon forgot to add const.
>>
>> The following does not compile:
>>
>> int main(int argc, const char *argv[])
>> {
>>         argv[0][1] = 'a';
>>         return 0;
>> }
>
> Of course not, but that's not particularly relevant. That prototype of
> main() is not standards-conformant (see 5.1.2.2.1 again), except perhaps
> as an implementation-defined extension (so most likely any real compiler
> will accept it, and then of course will prevent you from doing stores
> through the argv[i] pointers).
>
> Rasmus
>