|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Minios-devel] [UNIKRAFT PATCH] lib/ukboot: Fix handling of main arguments
Hey Costin,
On 06.06.19, 12:30, "Costin Lupu" <costin.lup@xxxxxxxxx> wrote:
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?
The change that I mention as alternative is minimal and less invasive to the
existing code. You do not need to add it as exported symbol and most code can
stay as is. I also do not see why we would need the global approach. Do you
have a use case? I think this is conflicting how argc and argv is intended to
be used: The idea about them is that they are private to your main() function
of your program (ukplat_entry() in our case). It can do whatever with it and
this does not have any effect on something else.
As an example you can take getopt(): It starts re-ordering the pointers on argv
for doing its argument parsing.
On a global and shared data structure you would not want this.
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.
Actually, we do not need to declare `argc` as static, only `argv`. I was wrong.
`argc` is anyways handed over by-value on a function call, not by-reference
(like argv) - so we are good.
>
> 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?
In case the boot stack is released or used for something else, there should be
no reference to it. The buffer where the strings are stored (and where the
pointers of argv are pointing to) shouldn't be on the stack either. I mentioned
this because I wanted to say that we should double check that our platform are
storing this buffer on something else than the stack.
>> - 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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |