From 4c1b66246dc2e9c4cf54e6c79d7d2beb4d5d2c8f Mon Sep 17 00:00:00 2001 From: Ville Juven Date: Mon, 4 Apr 2022 13:11:06 +0300 Subject: [PATCH] env_dup: Fix copying of env between address environments If address environments are in use, it is not possible to simply memcpy from from one process to another. The current implementation of env_dup does precisely this and thus, it fails at once when it is attempted between two user processes. The solution is to use the kernel's heap as an intermediate buffer. This is a simple, effective and common way to do a fork(). Obviously this is not needed for kernel processes. --- binfmt/binfmt.h | 44 +++++++++++++++++++++++++++++++++++ binfmt/binfmt_exec.c | 10 ++++---- binfmt/binfmt_execmodule.c | 26 +++++++++++++++++---- include/cxx/cstdlib | 2 ++ include/nuttx/binfmt/binfmt.h | 9 ++++--- include/nuttx/sched.h | 3 ++- include/stdlib.h | 4 ++++ sched/environ/env_dup.c | 23 +++++++++--------- sched/environ/environ.h | 7 +++--- sched/group/group_create.c | 12 ---------- sched/init/nx_bringup.c | 2 +- sched/task/spawn.h | 1 + sched/task/task_create.c | 3 ++- sched/task/task_init.c | 12 +++++++++- sched/task/task_posixspawn.c | 11 +++++---- sched/task/task_spawn.c | 1 + sched/task/task_vfork.c | 9 +++++++ 17 files changed, 133 insertions(+), 46 deletions(-) diff --git a/binfmt/binfmt.h b/binfmt/binfmt.h index 13fb13fa6c..f66f8fc58f 100644 --- a/binfmt/binfmt.h +++ b/binfmt/binfmt.h @@ -117,6 +117,50 @@ void binfmt_freeargv(FAR char * const *argv); # define binfmt_freeargv(argv) #endif +/**************************************************************************** + * Name: binfmt_copyenv + * + * Description: + * In the kernel build, the environment exists in the parent's address + * environment and, hence, will be inaccessible when we switch to the + * address environment of the new process. So we do not have any real + * option other than to copy the parents envp list into an intermediate + * buffer that resides in neutral kernel memory. + * + * Input Parameters: + * envp - Allocated environment strings + * + * Returned Value: + * A non-zero copy is returned on success. + * + ****************************************************************************/ + +#ifndef CONFIG_DISABLE_ENVIRON +# define binfmt_copyenv(envp) binfmt_copyargv(envp) +#else +# define binfmt_copyenv(envp) (envp) +#endif + +/**************************************************************************** + * Name: binfmt_freeenv + * + * Description: + * Release the copied envp[] list. + * + * Input Parameters: + * envp - Allocated environment strings + * + * Returned Value: + * None + * + ****************************************************************************/ + +#ifndef CONFIG_DISABLE_ENVIRON +# define binfmt_freeenv(envp) binfmt_freeargv(envp) +#else +# define binfmt_freeenv(envp) +#endif + /**************************************************************************** * Name: builtin_initialize * diff --git a/binfmt/binfmt_exec.c b/binfmt/binfmt_exec.c index 5bac3a5e73..5b23488891 100644 --- a/binfmt/binfmt_exec.c +++ b/binfmt/binfmt_exec.c @@ -55,6 +55,8 @@ * program. * argv - A pointer to an array of string arguments. The end of the * array is indicated with a NULL entry. + * envp - A pointer to an array of environment strings. Terminated with + * a NULL entry. * exports - The address of the start of the caller-provided symbol * table. This symbol table contains the addresses of symbols * exported by the caller and made available for linking the @@ -69,8 +71,8 @@ ****************************************************************************/ int exec_spawn(FAR const char *filename, FAR char * const *argv, - FAR const struct symtab_s *exports, int nexports, - FAR const posix_spawnattr_t *attr) + FAR char * const *envp, FAR const struct symtab_s *exports, + int nexports, FAR const posix_spawnattr_t *attr) { FAR struct binary_s *bin; int pid; @@ -121,7 +123,7 @@ int exec_spawn(FAR const char *filename, FAR char * const *argv, /* Then start the module */ - pid = exec_module(bin, filename, argv); + pid = exec_module(bin, filename, argv, envp); if (pid < 0) { ret = pid; @@ -228,7 +230,7 @@ int exec(FAR const char *filename, FAR char * const *argv, { int ret; - ret = exec_spawn(filename, argv, exports, nexports, NULL); + ret = exec_spawn(filename, argv, NULL, exports, nexports, NULL); if (ret < 0) { set_errno(-ret); diff --git a/binfmt/binfmt_execmodule.c b/binfmt/binfmt_execmodule.c index b6db1860a5..15da3603e2 100644 --- a/binfmt/binfmt_execmodule.c +++ b/binfmt/binfmt_execmodule.c @@ -112,7 +112,8 @@ static void exec_ctors(FAR void *arg) ****************************************************************************/ int exec_module(FAR const struct binary_s *binp, - FAR const char *filename, FAR char * const *argv) + FAR const char *filename, FAR char * const *argv, + FAR char * const *envp) { FAR struct task_tcb_s *tcb; #if defined(CONFIG_ARCH_ADDRENV) && defined(CONFIG_BUILD_KERNEL) @@ -150,6 +151,18 @@ int exec_module(FAR const struct binary_s *binp, } } + /* Make a copy of the environment here */ + + if (envp) + { + envp = binfmt_copyenv(envp); + if (!envp) + { + ret = -ENOMEM; + goto errout_with_args; + } + } + #if defined(CONFIG_ARCH_ADDRENV) && defined(CONFIG_BUILD_KERNEL) /* Instantiate the address environment containing the user heap */ @@ -157,7 +170,7 @@ int exec_module(FAR const struct binary_s *binp, if (ret < 0) { berr("ERROR: up_addrenv_select() failed: %d\n", ret); - goto errout_with_args; + goto errout_with_envp; } binfo("Initialize the user heap (heapsize=%d)\n", binp->addrenv.heapsize); @@ -173,12 +186,12 @@ int exec_module(FAR const struct binary_s *binp, if (argv && argv[0]) { ret = nxtask_init(tcb, argv[0], binp->priority, NULL, - binp->stacksize, binp->entrypt, &argv[1]); + binp->stacksize, binp->entrypt, &argv[1], envp); } else { ret = nxtask_init(tcb, filename, binp->priority, NULL, - binp->stacksize, binp->entrypt, argv); + binp->stacksize, binp->entrypt, argv, envp); } if (ret < 0) @@ -187,7 +200,10 @@ int exec_module(FAR const struct binary_s *binp, goto errout_with_addrenv; } + /* The copied argv and envp can now be released */ + binfmt_freeargv(argv); + binfmt_freeenv(envp); #if defined(CONFIG_ARCH_ADDRENV) && defined(CONFIG_ARCH_KERNEL_STACK) /* Allocate the kernel stack */ @@ -281,7 +297,9 @@ errout_with_tcbinit: errout_with_addrenv: #if defined(CONFIG_ARCH_ADDRENV) && defined(CONFIG_BUILD_KERNEL) up_addrenv_restore(&oldenv); +errout_with_envp: #endif + binfmt_freeenv(envp); errout_with_args: binfmt_freeargv(argv); errout_with_tcb: diff --git a/include/cxx/cstdlib b/include/cxx/cstdlib index 980956301d..eace8b417a 100644 --- a/include/cxx/cstdlib +++ b/include/cxx/cstdlib @@ -42,7 +42,9 @@ namespace std // Environment variable support +#ifndef CONFIG_DISABLE_ENVIRON using ::get_environ_ptr; +#endif using ::getenv; using ::putenv; using ::clearenv; diff --git a/include/nuttx/binfmt/binfmt.h b/include/nuttx/binfmt/binfmt.h index 416cc6c4a4..6ebeec446e 100644 --- a/include/nuttx/binfmt/binfmt.h +++ b/include/nuttx/binfmt/binfmt.h @@ -258,7 +258,8 @@ int unload_module(FAR struct binary_s *bin); ****************************************************************************/ int exec_module(FAR const struct binary_s *binp, - FAR const char *filename, FAR char * const *argv); + FAR const char *filename, FAR char * const *argv, + FAR char * const *envp); /**************************************************************************** * Name: exec @@ -338,6 +339,8 @@ int exec(FAR const char *filename, FAR char * const *argv, * program. * argv - A pointer to an array of string arguments. The end of the * array is indicated with a NULL entry. + * envp - A pointer to an array of environment strings. Terminated with + * a NULL entry. * exports - The address of the start of the caller-provided symbol * table. This symbol table contains the addresses of symbols * exported by the caller and made available for linking the @@ -353,8 +356,8 @@ int exec(FAR const char *filename, FAR char * const *argv, ****************************************************************************/ int exec_spawn(FAR const char *filename, FAR char * const *argv, - FAR const struct symtab_s *exports, int nexports, - FAR const posix_spawnattr_t *attr); + FAR char * const *envp, FAR const struct symtab_s *exports, + int nexports, FAR const posix_spawnattr_t *attr); /**************************************************************************** * Name: binfmt_exit diff --git a/include/nuttx/sched.h b/include/nuttx/sched.h index 88659844f2..468b32dea3 100644 --- a/include/nuttx/sched.h +++ b/include/nuttx/sched.h @@ -933,6 +933,7 @@ FAR struct streamlist *nxsched_get_streams(void); * argv - A pointer to an array of input parameters. The array * should be terminated with a NULL argv[] value. If no * parameters are required, argv may be NULL. + * envp - A pointer to the program's environment, envp may be NULL * * Returned Value: * OK on success; negative error value on failure appropriately. (See @@ -945,7 +946,7 @@ FAR struct streamlist *nxsched_get_streams(void); int nxtask_init(FAR struct task_tcb_s *tcb, const char *name, int priority, FAR void *stack, uint32_t stack_size, main_t entry, - FAR char * const argv[]); + FAR char * const argv[], FAR char * const envp[]); /**************************************************************************** * Name: nxtask_uninit diff --git a/include/stdlib.h b/include/stdlib.h index 745f4286b5..d40d0e0fed 100644 --- a/include/stdlib.h +++ b/include/stdlib.h @@ -138,7 +138,11 @@ void arc4random_buf(FAR void *bytes, size_t nbytes); /* Environment variable support */ +#ifdef CONFIG_DISABLE_ENVIRON +# define get_environ_ptr() NULL +#else FAR char **get_environ_ptr(void); +#endif FAR char *getenv(FAR const char *name); int putenv(FAR const char *string); int clearenv(void); diff --git a/sched/environ/env_dup.c b/sched/environ/env_dup.c index 58f15bb9d5..87e9274c77 100644 --- a/sched/environ/env_dup.c +++ b/sched/environ/env_dup.c @@ -50,9 +50,9 @@ * private, exact duplicate of the parent task's environment. * * Input Parameters: - * group - The child task group to receive the newly allocated copy of the - * parent task groups environment structure. - * + * group - The child task group to receive the newly allocated copy of + * the parent task groups environment structure. + * envcp - Pointer to the environment strings to copy. * Returned Value: * zero on success * @@ -61,14 +61,14 @@ * ****************************************************************************/ -int env_dup(FAR struct task_group_s *group) +int env_dup(FAR struct task_group_s *group, FAR char * const *envcp) { FAR struct tcb_s *ptcb = this_task(); FAR char **envp = NULL; size_t envc = 0; int ret = OK; - DEBUGASSERT(group != NULL && ptcb != NULL && ptcb->group != NULL); + DEBUGASSERT(group != NULL); /* Pre-emption must be disabled throughout the following because the * environment may be shared. @@ -76,13 +76,13 @@ int env_dup(FAR struct task_group_s *group) sched_lock(); - /* Does the parent task have an environment? */ + /* Is there an environment ? */ - if (ptcb->group != NULL && ptcb->group->tg_envp != NULL) + if (envcp || (envcp = (FAR char * const *)ptcb->group->tg_envp)) { - /* Yes.. The parent task has an environment allocation. */ + /* Count the strings */ - while (ptcb->group->tg_envp[envc] != NULL) + while (envcp[envc] != NULL) { envc++; } @@ -113,8 +113,7 @@ int env_dup(FAR struct task_group_s *group) while (envc-- > 0) { - envp[envc] = group_malloc(group, - strlen(ptcb->group->tg_envp[envc]) + 1); + envp[envc] = group_malloc(group, strlen(envcp[envc]) + 1); if (envp[envc] == NULL) { while (envp[++envc] != NULL) @@ -127,7 +126,7 @@ int env_dup(FAR struct task_group_s *group) break; } - strcpy(envp[envc], ptcb->group->tg_envp[envc]); + strcpy(envp[envc], envcp[envc]); } } } diff --git a/sched/environ/environ.h b/sched/environ/environ.h index 5787801f08..15aeceb0f5 100644 --- a/sched/environ/environ.h +++ b/sched/environ/environ.h @@ -33,8 +33,8 @@ ****************************************************************************/ #ifdef CONFIG_DISABLE_ENVIRON -# define env_dup(group) (0) -# define env_release(group) (0) +# define env_dup(group, envp) (0) +# define env_release(group) (0) #else /**************************************************************************** @@ -65,6 +65,7 @@ extern "C" * Input Parameters: * group - The child task group to receive the newly allocated copy of the * parent task groups environment structure. + * envp - Pointer to the environment strings. * * Returned Value: * zero on success @@ -74,7 +75,7 @@ extern "C" * ****************************************************************************/ -int env_dup(FAR struct task_group_s *group); +int env_dup(FAR struct task_group_s *group, FAR char * const *envp); /**************************************************************************** * Name: env_release diff --git a/sched/group/group_create.c b/sched/group/group_create.c index acfc79b875..e5347ff541 100644 --- a/sched/group/group_create.c +++ b/sched/group/group_create.c @@ -37,7 +37,6 @@ #include #include -#include "environ/environ.h" #include "sched/sched.h" #include "group/group.h" @@ -201,15 +200,6 @@ int group_allocate(FAR struct task_tcb_s *tcb, uint8_t ttype) group_inherit_identity(group); - /* Duplicate the parent tasks environment */ - - ret = env_dup(group); - if (ret < 0) - { - tcb->cmn.group = NULL; - goto errout_with_taskinfo; - } - /* Initial user space semaphore */ nxsem_init(&group->tg_info->ta_sem, 0, 1); @@ -243,8 +233,6 @@ int group_allocate(FAR struct task_tcb_s *tcb, uint8_t ttype) return OK; -errout_with_taskinfo: - group_free(group, group->tg_info); errout_with_member: #ifdef HAVE_GROUP_MEMBERS kmm_free(group->tg_members); diff --git a/sched/init/nx_bringup.c b/sched/init/nx_bringup.c index 8ec1d743d7..5391c3336a 100644 --- a/sched/init/nx_bringup.c +++ b/sched/init/nx_bringup.c @@ -292,7 +292,7 @@ static inline void nx_start_application(void) #ifndef CONFIG_ARCH_ADDRENV attr.stacksize = CONFIG_INIT_STACKSIZE; #endif - ret = exec_spawn(CONFIG_INIT_FILEPATH, argv, + ret = exec_spawn(CONFIG_INIT_FILEPATH, argv, NULL, CONFIG_INIT_SYMTAB, CONFIG_INIT_NEXPORTS, &attr); DEBUGASSERT(ret >= 0); #endif diff --git a/sched/task/spawn.h b/sched/task/spawn.h index d1d6ba4a2c..521b065064 100644 --- a/sched/task/spawn.h +++ b/sched/task/spawn.h @@ -49,6 +49,7 @@ struct spawn_parms_s FAR const posix_spawn_file_actions_t *file_actions; FAR const posix_spawnattr_t *attr; FAR char * const *argv; + FAR char * const *envp; /* Parameters that differ for posix_spawn[p] and task_spawn */ diff --git a/sched/task/task_create.c b/sched/task/task_create.c index e85ed653d3..380f5459c0 100644 --- a/sched/task/task_create.c +++ b/sched/task/task_create.c @@ -91,7 +91,8 @@ static int nxthread_create(FAR const char *name, uint8_t ttype, /* Initialize the task */ - ret = nxtask_init(tcb, name, priority, stack_ptr, stack_size, entry, argv); + ret = nxtask_init(tcb, name, priority, stack_ptr, stack_size, entry, argv, + NULL); if (ret < OK) { kmm_free(tcb); diff --git a/sched/task/task_init.c b/sched/task/task_init.c index 126484b802..5cbb774fba 100644 --- a/sched/task/task_init.c +++ b/sched/task/task_init.c @@ -36,6 +36,7 @@ #include #include "sched/sched.h" +#include "environ/environ.h" #include "group/group.h" #include "task/task.h" @@ -83,7 +84,8 @@ int nxtask_init(FAR struct task_tcb_s *tcb, const char *name, int priority, FAR void *stack, uint32_t stack_size, - main_t entry, FAR char * const argv[]) + main_t entry, FAR char * const argv[], + FAR char * const envp[]) { uint8_t ttype = tcb->cmn.flags & TCB_FLAG_TTYPE_MASK; FAR struct tls_info_s *info; @@ -103,6 +105,14 @@ int nxtask_init(FAR struct task_tcb_s *tcb, const char *name, int priority, return ret; } + /* Duplicate the parent tasks environment */ + + ret = env_dup(tcb->cmn.group, envp); + if (ret < 0) + { + goto errout_with_group; + } + /* Associate file descriptors with the new task */ ret = group_setuptaskfiles(tcb); diff --git a/sched/task/task_posixspawn.c b/sched/task/task_posixspawn.c index 881d604002..6368a85511 100644 --- a/sched/task/task_posixspawn.c +++ b/sched/task/task_posixspawn.c @@ -85,7 +85,8 @@ static int nxposix_spawn_exec(FAR pid_t *pidp, FAR const char *path, FAR const posix_spawnattr_t *attr, - FAR char * const argv[]) + FAR char * const argv[], + FAR char * const envp[]) { FAR const struct symtab_s *symtab; int nsymbols; @@ -107,7 +108,7 @@ static int nxposix_spawn_exec(FAR pid_t *pidp, FAR const char *path, /* Start the task */ - pid = exec_spawn(path, (FAR char * const *)argv, symtab, nsymbols, attr); + pid = exec_spawn(path, argv, envp, symtab, nsymbols, attr); if (pid < 0) { ret = -pid; @@ -184,7 +185,8 @@ static int nxposix_spawn_proxy(int argc, FAR char *argv[]) /* Start the task */ ret = nxposix_spawn_exec(g_spawn_parms.pid, g_spawn_parms.u.posix.path, - g_spawn_parms.attr, g_spawn_parms.argv); + g_spawn_parms.attr, g_spawn_parms.argv, + g_spawn_parms.envp); #ifdef CONFIG_SCHED_HAVE_PARENT if (ret == OK) @@ -335,7 +337,7 @@ int posix_spawn(FAR pid_t *pid, FAR const char *path, if ((file_actions == NULL || *file_actions == NULL) && (attr == NULL || (attr->flags & POSIX_SPAWN_SETSIGMASK) == 0)) { - return nxposix_spawn_exec(pid, path, attr, argv); + return nxposix_spawn_exec(pid, path, attr, argv, envp); } /* Otherwise, we will have to go through an intermediary/proxy task in @@ -365,6 +367,7 @@ int posix_spawn(FAR pid_t *pid, FAR const char *path, g_spawn_parms.file_actions = file_actions ? *file_actions : NULL; g_spawn_parms.attr = attr; g_spawn_parms.argv = argv; + g_spawn_parms.envp = envp; g_spawn_parms.u.posix.path = path; /* Get the priority of this (parent) task */ diff --git a/sched/task/task_spawn.c b/sched/task/task_spawn.c index 327bf8c6f0..6374a6e873 100644 --- a/sched/task/task_spawn.c +++ b/sched/task/task_spawn.c @@ -365,6 +365,7 @@ int task_spawn(FAR const char *name, main_t entry, g_spawn_parms.file_actions = file_actions ? *file_actions : NULL; g_spawn_parms.attr = attr; g_spawn_parms.argv = argv; + g_spawn_parms.envp = envp; g_spawn_parms.u.task.name = name; g_spawn_parms.u.task.entry = entry; diff --git a/sched/task/task_vfork.c b/sched/task/task_vfork.c index fe87899d1b..64e1f7d35c 100644 --- a/sched/task/task_vfork.c +++ b/sched/task/task_vfork.c @@ -37,6 +37,7 @@ #include #include "sched/sched.h" +#include "environ/environ.h" #include "group/group.h" #include "task/task.h" @@ -151,6 +152,14 @@ FAR struct task_tcb_s *nxtask_setup_vfork(start_t retaddr) goto errout_with_tcb; } + /* Duplicate the parent tasks environment */ + + ret = env_dup(child->cmn.group, get_environ_ptr()); + if (ret < 0) + { + goto errout_with_tcb; + } + /* Associate file descriptors with the new task */ ret = group_setuptaskfiles(child);