[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,
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.
- 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
|
