[PATCH 4/5] sandbox: implement reset

[Rasmus Villemoes]

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.

> 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]. 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