From ab43681f15717d0add34a9e0d6766be36c94dfa5 Mon Sep 17 00:00:00 2001 From: Gregory Nutt Date: Thu, 8 Dec 2016 10:24:40 -0600 Subject: [PATCH] Update TODO and some comments. --- TODO | 41 ++++++++++++++++++++--- arch/arm/src/armv7-m/up_signal_dispatch.c | 12 ------- include/nuttx/sched.h | 3 +- sched/pthread/pthread_cancel.c | 11 ++++-- sched/task/task_exithook.c | 8 +++++ 5 files changed, 54 insertions(+), 21 deletions(-) diff --git a/TODO b/TODO index 9b8776acd0..b6658bb388 100644 --- a/TODO +++ b/TODO @@ -108,9 +108,30 @@ o Task/Scheduler (sched/) 2. They run in supervisor mode (if applicable), and 3. They do not obey any setup of PIC or address environments. Do they need to? + 4. In the case of task_delete() and pthread_cancel, these + callbacks will run on the thread of execution and address + context of the caller of task. That is very bad! The fix for all of these issues it to have the callbacks - run on the caller's thread (as with signal handlers). + run on the caller's thread as is currently done with + signal handlers. Signals are delivered differently in + PROTECTED and KERNEL modes: The deliver is involes a + signal handling trampoline function in the user address + space and two signal handlers: One to call the signal + handler trampoline in user mode (SYS_signal_handler) and + on in with the signal handler trampoline to return to + supervisor mode (SYS_signal_handler_return) + + The primary difference is in the location of the signal + handling trampoline: + + - In PROTECTED mode, there is on a single user space blob + with a header at the beginning of the block (at a well- + known location. There is a pointer to the signal handler + trampoline function in that header. + - In the KERNEL mode, a special process signal handler + trampoline is used at a well-known location in every + process address space (ARCH_DATA_RESERVE->ar_sigtramp). Status: Open Priority: Medium Low. This is an important change to some less important interfaces. For the average user, these @@ -145,16 +166,26 @@ o Task/Scheduler (sched/) Priority: Low Title: REMOVE TASK_DELETE - Description: Need to remove or fix task delete. This interface is non- + Description: Need to remove or fix task delete(). This interface is non- standard and not safe. Arbitrary deleting tasks can cause - serious problems such as memory leaks. Better to remove it - than to retain it as a latent bug. + serious problems such as memory leaks and resources like + semaphores left in bad states. + + Task/process exit callbacks registered via atexit() or + on_exit() will not work correctly with task_delete(): In + that case the callback would execute in the context the + caller of task_delete() cancel, not in the context of the + exiting task (or process). + + Better to remove task_delete() than to retain it as a latent + bug. Currently used within the OS and also part of the implementation of pthread_cancel() and task_restart() (which should also go for the same reasons). It is used in NxWM::CNxConsole to terminate console tasks and also in - apps/netutils/thttpd to kill CGI tasks that timeout. + apps/netutils/thttpd to kill CGI tasks that timeout. So not + so simple to remove. Status: Open Priority: Low and not easily removable. diff --git a/arch/arm/src/armv7-m/up_signal_dispatch.c b/arch/arm/src/armv7-m/up_signal_dispatch.c index 4a03a26b21..9ec7d15152 100644 --- a/arch/arm/src/armv7-m/up_signal_dispatch.c +++ b/arch/arm/src/armv7-m/up_signal_dispatch.c @@ -46,18 +46,6 @@ #if ((defined(CONFIG_BUILD_PROTECTED) && defined(__KERNEL__)) || \ defined(CONFIG_BUILD_KERNEL)) && !defined(CONFIG_DISABLE_SIGNALS) -/**************************************************************************** - * Pre-processor Definitions - ****************************************************************************/ - -/**************************************************************************** - * Private Data - ****************************************************************************/ - -/**************************************************************************** - * Private Functions - ****************************************************************************/ - /**************************************************************************** * Public Functions ****************************************************************************/ diff --git a/include/nuttx/sched.h b/include/nuttx/sched.h index 81d72c6e60..1db8b70a24 100644 --- a/include/nuttx/sched.h +++ b/include/nuttx/sched.h @@ -323,10 +323,9 @@ struct child_status_s #endif /* struct pthread_cleanup_s ******************************************************/ - -#ifdef CONFIG_PTHREAD_CLEANUP /* This structure describes one element of the pthread cleanup stack */ +#ifdef CONFIG_PTHREAD_CLEANUP struct pthread_cleanup_s { pthread_cleanup_t pc_cleaner; /* Cleanup callback address */ diff --git a/sched/pthread/pthread_cancel.c b/sched/pthread/pthread_cancel.c index e651259bc0..fcaba69079 100644 --- a/sched/pthread/pthread_cancel.c +++ b/sched/pthread/pthread_cancel.c @@ -116,9 +116,16 @@ int pthread_cancel(pthread_t thread) } #ifdef CONFIG_PTHREAD_CLEANUP - /* Perform any stack pthread clean-up callbacks */ + /* Perform any stack pthread clean-up callbacks. + * + * REVISIT: In this case, the clean-up callback will execute on the + * thread of the caller of pthread cancel, not on the thread of + * the thread-to-be-canceled. Is that an issue? Presumably they + * are both within the same group and within the same process address + * space. + */ - pthread_cleanup_popall(tcb); + pthread_cleanup_popall(tcb); #endif /* Complete pending join operations */ diff --git a/sched/task/task_exithook.c b/sched/task/task_exithook.c index 94ed2786a2..96856d8792 100644 --- a/sched/task/task_exithook.c +++ b/sched/task/task_exithook.c @@ -77,6 +77,10 @@ static inline void task_atexit(FAR struct tcb_s *tcb) * REVISIT: This is a security problem In the PROTECTED and KERNEL builds: * We must not call the registered function in supervisor mode! See also * on_exit() and pthread_cleanup_pop() callbacks. + * + * REVISIT: In the case of task_delete(), the callback would execute in + * the context the caller of task_delete() cancel, not in the context of + * the exiting task (or process). */ if (group && group->tg_nmembers == 1) @@ -141,6 +145,10 @@ static inline void task_onexit(FAR struct tcb_s *tcb, int status) * REVISIT: This is a security problem In the PROTECTED and KERNEL builds: * We must not call the registered function in supervisor mode! See also * atexit() and pthread_cleanup_pop() callbacks. + * + * REVISIT: In the case of task_delete(), the callback would execute in + * he context the caller of task_delete() cancel, not in the context of + * the exiting task (or process). */ if (group && group->tg_nmembers == 1)