Re: [Minios-devel] [UNIKRAFT PATCH] lib/ukboot: Fix handling of main arguments

[Costin Lupu <costin.lup@xxxxxxxxx>]

Hi Simon,

I've read your comments but I couldn't find out why you don't agree with
global approach. So, what are the reasons why you would prefer declaring
the arguments static rather than global?

Personally I find making the arguments global to be more elegant and
offer a unified approach, since they would have the same location for
all platforms.

Also, please see a comment inline.

On 6/6/19 12:34 PM, Simon Kuenzer wrote:
> Hey Costin,
> 
> thanks a lot for the patch. I think the actual problem is that the boot
> stack could be released later after boot for something else, so it is
> not guaranteed anymore that the arguments are safe and staying.
> However, I don't agree to make argv and argc global available. Just
> allocate them in the bss section.
> See my comments inline.
> 
> On 05.06.19 21:55, Costin Lupu wrote:
>> When the command line is parsed and split into arguments, each argument
>> is saved in an array of pointers allocated on the stack ('argv' array in
>> ukplat_entry_argp()). This means that the array can be overwritten any
>> time before starting the main thread which needs the arguments. This
>> patch fixes that by making the array global, encapsulated by the
>> 'main_arg' structure.
>>
>> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
>> ---
>>   include/uk/plat/bootstrap.h | 11 ++++++++++-
>>   lib/ukboot/boot.c           | 33 ++++++++++++---------------------
>>   lib/ukboot/exportsyms.uk    |  1 +
>>   plat/linuxu/setup.c         |  7 ++++++-
>>   4 files changed, 29 insertions(+), 23 deletions(-)
>>
>> diff --git a/include/uk/plat/bootstrap.h b/include/uk/plat/bootstrap.h
>> index 0652ccd1..a9e404e8 100644
>> --- a/include/uk/plat/bootstrap.h
>> +++ b/include/uk/plat/bootstrap.h
>> @@ -36,6 +36,7 @@
>>   #ifndef __UKPLAT_BOOTSTRAP__
>>   #define __UKPLAT_BOOTSTRAP__
>>   +#include <uk/config.h>
>>   #include <uk/arch/types.h>
>>   #include <uk/essentials.h>
>>   @@ -43,6 +44,14 @@
>>   extern "C" {
>>   #endif
>>   +/* Main arguments structure */
>> +struct main_arg {
>> +    int argc;
>> +    char *argv[CONFIG_LIBUKBOOT_MAXNBARGS];
>> +};
>> +/* Main arguments */
>> +extern struct main_arg global_main_arg;
>> +
>>   /**
>>    * Called by platform library during initialization
>>    * This function has to be provided by a non-platform library for
>> bootstrapping
>> @@ -52,7 +61,7 @@ extern "C" {
>>    * @param argc Number of arguments
>>    * @param args Array to '\0'-terminated arguments
>>    */
>> -void ukplat_entry(int argc, char *argv[]) __noreturn;
>> +void ukplat_entry(struct main_arg *arg) __noreturn;
>>     /**
>>    * Called by platform library during initialization
>> diff --git a/lib/ukboot/boot.c b/lib/ukboot/boot.c
>> index 4dcc0afb..5cc3b573 100644
>> --- a/lib/ukboot/boot.c
>> +++ b/lib/ukboot/boot.c
>> @@ -68,17 +68,13 @@ extern int liblwip_init(void);
>>   #endif /* CONFIG_LIBLWIP */
>>     static void main_thread_func(void *arg) __noreturn;
>> -
>> -struct thread_main_arg {
>> -    int argc;
>> -    char **argv;
>> -};
>> +struct main_arg global_main_arg;
>>     static void main_thread_func(void *arg)
>>   {
>>       int i;
>>       int ret;
>> -    struct thread_main_arg *tma = arg;
>> +    struct main_arg *tma = arg;
>>         uk_pr_info("Pre-init table at %p - %p\n",
>>              __preinit_array_start, &__preinit_array_end);
>> @@ -145,26 +141,26 @@ static void main_thread_func(void *arg)
>>   /* defined in <uk/plat.h> */
>>   void ukplat_entry_argp(char *arg0, char *argb, __sz argb_len)
>>   {
>> -    char *argv[CONFIG_LIBUKBOOT_MAXNBARGS];
> 
> Just declare
>     char *argv[CONFIG_LIBUKBOOT_MAXNBARGS];
> as
>     static char *argv[CONFIG_LIBUKBOOT_MAXNBARGS];
> 
> The same for argc.
> 
> We also need to double-check that our string buffer with the arguments
> (where argv is pointing to), is not allocated on the stack in the
> platform. If this is the case, we are safe.
> 

I'm not sure I follow this, you mean to check if the strings aren't
allocated on this stack? Or to check if it comes from bss?

>> -    int argc = 0;
>> +    global_main_arg.argc = 0;
>>         if (arg0) {
>> -        argv[0] = arg0;
>> -        argc += 1;
>> +        global_main_arg.argv[0] = arg0;
>> +        global_main_arg.argc += 1;
>>       }
>>       if (argb && argb_len) {
>> -        argc += uk_argnparse(argb, argb_len, arg0 ? &argv[1] : &argv[0],
>> +        global_main_arg.argc += uk_argnparse(argb, argb_len,
>> +                     arg0 ? &global_main_arg.argv[1]
>> +                      : &global_main_arg.argv[0],
>>                        arg0 ? (CONFIG_LIBUKBOOT_MAXNBARGS - 1)
>>                         : CONFIG_LIBUKBOOT_MAXNBARGS);
>>       }
>> -    ukplat_entry(argc, argv);
>> +    ukplat_entry(&global_main_arg);
> 
> If you declare the arguments as global you do not need to hand them over
> anymore, but as I said I would not change this at all.
> 
>>   }
>>     /* defined in <uk/plat.h> */
>> -void ukplat_entry(int argc, char *argv[])
>> +void ukplat_entry(struct main_arg *arg)
>>   {
>>       const uk_ctor_func_t *cfn;
>> -    struct thread_main_arg tma;
>>   #if CONFIG_LIBUKALLOC
>>       struct uk_alloc *a = NULL;
>>       int rc;
>> @@ -234,19 +230,14 @@ void ukplat_entry(int argc, char *argv[])
>>       s = uk_sched_default_init(a);
>>       if (unlikely(!s))
>>           UK_CRASH("Could not initialize the scheduler\n");
>> -#endif
>>   -    tma.argc = argc;
>> -    tma.argv = argv;
>> -
>> -#if CONFIG_LIBUKSCHED
>> -    main_thread = uk_thread_create("main", main_thread_func, &tma);
>> +    main_thread = uk_thread_create("main", main_thread_func, arg);
>>       if (unlikely(!main_thread))
>>           UK_CRASH("Could not create main thread\n");
>>       uk_sched_start(s);
>>   #else
>>       /* Enable interrupts before starting the application */
>>       ukplat_lcpu_enable_irq();
>> -    main_thread_func(&tma);
>> +    main_thread_func(arg);
>>   #endif
>>   }
>> diff --git a/lib/ukboot/exportsyms.uk b/lib/ukboot/exportsyms.uk
>> index 3edc6c6a..adbb84fb 100644
>> --- a/lib/ukboot/exportsyms.uk
>> +++ b/lib/ukboot/exportsyms.uk
>> @@ -1,3 +1,4 @@
>>   ukplat_entry_argp
>>   ukplat_entry
>> +global_main_arg
>>   main
>> diff --git a/plat/linuxu/setup.c b/plat/linuxu/setup.c
>> index 50454437..4f6f5688 100644
>> --- a/plat/linuxu/setup.c
>> +++ b/plat/linuxu/setup.c
>> @@ -172,6 +172,11 @@ void _liblinuxuplat_entry(int argc, char *argv[])
>>       argv += (ret - 1);
>>       argv[0] = progname;
> 
> Make also sure that argv and argc are declared as static here, instead.
> 
>>   +    /* Setup global arguments */
>> +    global_main_arg.argc = argc;
>> +    for (int i = 0; i < argc; i++)
>> +        global_main_arg.argv[i] = argv[i];
>> +
>>       /*
>>        * Allocate heap memory
>>        */
>> @@ -186,5 +191,5 @@ void _liblinuxuplat_entry(int argc, char *argv[])
>>       /*
>>        * Enter Unikraft
>>        */
>> -    ukplat_entry(argc, argv);
>> +    ukplat_entry(&global_main_arg);
>>   }
>>
> 
> Thanks,
> 
> Simon
> 
> _______________________________________________
> Minios-devel mailing list
> Minios-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/minios-devel

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel