diff --git a/TODO b/TODO index 9282df6df60..9b8776acd00 100644 --- a/TODO +++ b/TODO @@ -1,4 +1,4 @@ -NuttX TODO List (Last updated December 3, 2016) +NuttX TODO List (Last updated December 17, 2016) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This file summarizes known NuttX bugs, limitations, inconsistencies with @@ -101,7 +101,7 @@ o Task/Scheduler (sched/) Status: Open Priority: Medium Low for now - Title: ISSUES WITH atexit() AND on_exit() + Title: ISSUES WITH atexit(), on_exit(), AND pthread_cleanup_pop() Description: These functions execute with the following bad properties: 1. They run with interrupts disabled, diff --git a/include/nuttx/sched.h b/include/nuttx/sched.h index ec438727f81..81d72c6e600 100644 --- a/include/nuttx/sched.h +++ b/include/nuttx/sched.h @@ -322,6 +322,18 @@ struct child_status_s }; #endif +/* struct pthread_cleanup_s ******************************************************/ + +#ifdef CONFIG_PTHREAD_CLEANUP +/* This structure describes one element of the pthread cleanup stack */ + +struct pthread_cleanup_s +{ + pthread_cleanup_t pc_cleaner; /* Cleanup callback address */ + FAR void *pc_arg; /* Argument that accompanies the callback */ +}; +#endif + /* struct dspace_s ***************************************************************/ /* This structure describes a reference counted D-Space region. This must be a * separately allocated "break-away" structure that can be owned by a task and @@ -682,6 +694,17 @@ struct pthread_tcb_s pthread_addr_t arg; /* Startup argument */ FAR void *joininfo; /* Detach-able info to support join */ + /* Clean-up stack *************************************************************/ + +#ifdef CONFIG_PTHREAD_CLEANUP + /* tos - The index to the next avaiable entry at the top of the stack. + * stack - The pre-allocated clean-up stack memory. + */ + + uint8_t tos; + struct pthread_cleanup_s stack[CONFIG_PTHREAD_CLEANUP_STACKSIZE]; +#endif + /* POSIX Thread Specific Data *************************************************/ #if CONFIG_NPTHREAD_KEYS > 0 diff --git a/include/pthread.h b/include/pthread.h index ac495e44c29..28bb0add975 100644 --- a/include/pthread.h +++ b/include/pthread.h @@ -265,6 +265,12 @@ typedef struct pthread_barrier_s pthread_barrier_t; typedef bool pthread_once_t; #define __PTHREAD_ONCE_T_DEFINED 1 +#ifdef CONFIG_PTHREAD_CLEANUP +/* This type describes the pthread cleanup callback (non-standard) */ + +typedef CODE void (*pthread_cleanup_t)(FAR void *arg); +#endif + /* Forward references */ struct sched_param; /* Defined in sched.h */ @@ -336,6 +342,15 @@ int pthread_cancel(pthread_t thread); int pthread_setcancelstate(int state, FAR int *oldstate); void pthread_testcancel(void); +/* A thread may set up cleanup functions to execut when the thread exits or + * is canceled. + */ + +#ifdef CONFIG_PTHREAD_CLEANUP +void pthread_cleanup_pop(int execute); +void pthread_cleanup_push(pthread_cleanup_t routine, FAR void *arg); +#endif + /* A thread can await termination of another thread and retrieve the return * value of the thread. */ diff --git a/include/sys/syscall.h b/include/sys/syscall.h index d0bfcd95977..5c13ec70e3a 100644 --- a/include/sys/syscall.h +++ b/include/sys/syscall.h @@ -406,7 +406,7 @@ # define SYS_pthread_setspecific (__SYS_pthread+26) # define SYS_pthread_yield (__SYS_pthread+27) -# ifndef CONFIG_SMP +# ifdef CONFIG_SMP # define SYS_pthread_setaffinity_np (__SYS_pthread+28) # define SYS_pthread_getaffinity_np (__SYS_pthread+29) # define __SYS_pthread_signals (__SYS_pthread+30) @@ -418,9 +418,17 @@ # define SYS_pthread_cond_timedwait (__SYS_pthread_signals+0) # define SYS_pthread_kill (__SYS_pthread_signals+1) # define SYS_pthread_sigmask (__SYS_pthread_signals+2) -# define __SYS_mqueue (__SYS_pthread_signals+3) +# define __SYS_pthread_cleanup (__SYS_pthread_signals+3) # else -# define __SYS_mqueue __SYS_pthread_signals +# define __SYS_pthread_cleanup __SYS_pthread_signals +# endif + +# ifdef CONFIG_PTHREAD_CLEANUP +# define __SYS_pthread_cleanup_push (__SYS_pthread_cleanup+0) +# define __SYS_pthread_cleanup_pop (__SYS_pthread_cleanup+1) +# define __SYS_mqueue (__SYS_pthread_cleanup+2) +# else +# define __SYS_mqueue __SYS_pthread_cleanup # endif #else diff --git a/sched/Kconfig b/sched/Kconfig index 4b0e068e6fb..6f938bf45b6 100644 --- a/sched/Kconfig +++ b/sched/Kconfig @@ -537,6 +537,24 @@ config NPTHREAD_KEYS The number of items of thread- specific data that can be retained +config PTHREAD_CLEANUP + bool "pthread cleanup stack" + default n + ---help--- + Select to enable support for pthread exit cleanup stacks. This + enables the interfaces pthread_cleanup_push() and + pthread_cleanup_pop(). + +config PTHREAD_CLEANUP_STACKSIZE + int "pthread cleanup stack size" + default 1 + range 1 32 + depends on PTHREAD_CLEANUP + ---help--- + The maximum number of cleanup actions that may be pushed by + pthread_clean_push(). This setting will increase the size of EVERY + pthread task control block by about 8 * CONFIG_PTHREAD_CLEANUP_STACKSIZE. + endmenu # Pthread Options menu "Performance Monitoring" diff --git a/sched/pthread/Make.defs b/sched/pthread/Make.defs index d27938aa379..a08315c732c 100644 --- a/sched/pthread/Make.defs +++ b/sched/pthread/Make.defs @@ -55,6 +55,10 @@ ifeq ($(CONFIG_SMP),y) CSRCS += pthread_setaffinity.c pthread_getaffinity.c endif +ifeq ($(CONFIG_PTHREAD_CLEANUP),y) +CSRCS += pthread_cleanup.c +endif + # Include pthread build support DEPPATH += --dep-path pthread diff --git a/sched/pthread/pthread.h b/sched/pthread/pthread.h index 6371e9df330..be7a20de2ea 100644 --- a/sched/pthread/pthread.h +++ b/sched/pthread/pthread.h @@ -96,6 +96,11 @@ struct task_group_s; /* Forward reference */ void weak_function pthread_initialize(void); int pthread_schedsetup(FAR struct pthread_tcb_s *tcb, int priority, start_t start, pthread_startroutine_t entry); + +#ifdef CONFIG_PTHREAD_CLEANUP +void pthread_cleanup_popall(FAR struct pthread_tcb_s *tcb); +#endif + int pthread_completejoin(pid_t pid, FAR void *exit_value); void pthread_destroyjoin(FAR struct task_group_s *group, FAR struct join_s *pjoin); diff --git a/sched/pthread/pthread_cancel.c b/sched/pthread/pthread_cancel.c index a8ba2ee4c93..e651259bc0f 100644 --- a/sched/pthread/pthread_cancel.c +++ b/sched/pthread/pthread_cancel.c @@ -53,7 +53,7 @@ int pthread_cancel(pthread_t thread) { - FAR struct tcb_s *tcb; + FAR struct pthread_tcb_s *tcb; /* First, make sure that the handle references a valid thread */ @@ -66,8 +66,8 @@ int pthread_cancel(pthread_t thread) return ESRCH; } - tcb = sched_gettcb((pid_t)thread); - if (!tcb) + tcb = (FAR struct pthread_tcb_s *)sched_gettcb((pid_t)thread); + if (tcb == NULL) { /* The pid does not correspond to any known thread. The thread * has probably already exited. @@ -76,13 +76,15 @@ int pthread_cancel(pthread_t thread) return ESRCH; } + DEBUGASSERT((tcb-cmn.flags & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_PTHREAD); + /* Check to see if this thread has the non-cancelable bit set in its * flags. Suppress context changes for a bit so that the flags are stable. * (the flags should not change in interrupt handling. */ sched_lock(); - if ((tcb->flags & TCB_FLAG_NONCANCELABLE) != 0) + if ((tcb->cmn.flags & TCB_FLAG_NONCANCELABLE) != 0) { /* Then we cannot cancel the thread now. Here is how this is * supposed to work: @@ -97,7 +99,7 @@ int pthread_cancel(pthread_t thread) * processing." */ - tcb->flags |= TCB_FLAG_CANCEL_PENDING; + tcb->cmn.flags |= TCB_FLAG_CANCEL_PENDING; sched_unlock(); return OK; } @@ -108,11 +110,17 @@ int pthread_cancel(pthread_t thread) * same as pthread_exit(PTHREAD_CANCELED). */ - if (tcb == this_task()) + if (tcb == (FAR struct pthread_tcb_s *)this_task()) { pthread_exit(PTHREAD_CANCELED); } +#ifdef CONFIG_PTHREAD_CLEANUP + /* Perform any stack pthread clean-up callbacks */ + + pthread_cleanup_popall(tcb); +#endif + /* Complete pending join operations */ (void)pthread_completejoin((pid_t)thread, PTHREAD_CANCELED); diff --git a/sched/pthread/pthread_cleanup.c b/sched/pthread/pthread_cleanup.c new file mode 100644 index 00000000000..304d572f210 --- /dev/null +++ b/sched/pthread/pthread_cleanup.c @@ -0,0 +1,212 @@ +/**************************************************************************** + * sched/pthread/pthread_cleanup.c + * + * Copyright (C) 2016 Gregory Nutt. All rights reserved. + * Author: Gregory Nutt + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * + * 1. Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in + * the documentation and/or other materials provided with the + * distribution. + * 3. Neither the name NuttX nor the names of its contributors may be + * used to endorse or promote products derived from this software + * without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE + * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, + * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS + * OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED + * AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN + * ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE + * POSSIBILITY OF SUCH DAMAGE. + * + ****************************************************************************/ + +/**************************************************************************** + * Included Files + ****************************************************************************/ + +#include + +#include + +#include +#include + +#include "sched/sched.h" +#include "pthread/pthread.h" + +#ifdef CONFIG_PTHREAD_CLEANUP + +/**************************************************************************** + * Private Functions + ****************************************************************************/ + +/**************************************************************************** + * Name: pthread_cleanup_pop_tcb + * + * Description: + * The pthread_cleanup_pop_tcb() function will remove the routine at the top + * of the calling thread's cancellation cleanup stack and optionally + * invoke it (if 'execute' is non-zero). + * + * Input Parameters: + * tcb - The TCB of the pthread that is exiting or being canceled. + * + * Return Value: + * None + * + ****************************************************************************/ + +static void pthread_cleanup_pop_tcb(FAR struct pthread_tcb_s *tcb, int execute) +{ + if (tcb->tos > 0) + { + unsigned int ndx; + + /* Get the index to the last cleaner function pushed onto the stack */ + + ndx = tcb->tos - 1; + DEBUGASSERT(ndx < CONFIG_PTHREAD_CLEANUP_STACKSIZE); + + /* Should we execute the cleanup routine at the top of the stack? */ + + if (execute != 0) + { + FAR struct pthread_cleanup_s *cb; + + /* Yes.. Execute the clean-up routine. + * + * 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 atexit() callbacks. + */ + + cb = &tcb->stack[ndx]; + cb->pc_cleaner(cb->pc_arg); + } + + tcb->tos = ndx; + } +} + +/**************************************************************************** + * Public Functions + ****************************************************************************/ + +/**************************************************************************** + * Name: pthread_cleanup_push + * pthread_cleanup_pop + * + * Description: + * The pthread_cleanup_pop() function will remove the routine at the top + * of the calling thread's cancellation cleanup stack and optionally + * invoke it (if 'execute' is non-zero). + * + * The pthread_cleanup_push() function will push the specified cancellation + * cleanup handler routine onto the calling thread's cancellation cleanup + * stack. The cancellation cleanup handler will be popped from the + * cancellation cleanup stack and invoked with the argument arg when: + * + * - The thread exits (that is, calls pthread_exit()). + * - The thread acts upon a cancellation request. + * - The thread calls pthread_cleanup_pop() with a non-zero execute argument. + * + * Input Parameters: + * routine - The cleanup routine to be pushed on the the cleanup stack. + * arg - An argument that will accompany the callback. + * execute - Execute the popped cleanup function immediately. + * + * Returned Value: + * None + * + ****************************************************************************/ + +void pthread_cleanup_pop(int execute) +{ + FAR struct pthread_tcb_s *tcb = (FAR struct pthread_tcb_s *)this_task(); + irqstate_t flags; + + /* We don't assert if called from a non-pthread; we just don't do anything */ + + DEBUGASSERT(tcb != NULL); + + flags = enter_critical_section(); + if ((tcb->cmn.flags & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_PTHREAD) + { + pthread_cleanup_pop_tcb(tcb, execute); + } + + leave_critical_section(flags); +} + +void pthread_cleanup_push(pthread_cleanup_t routine, FAR void *arg) +{ + FAR struct pthread_tcb_s *tcb = (FAR struct pthread_tcb_s *)this_task(); + irqstate_t flags; + + /* We don't assert if called from a non-pthread; we just don't do anything */ + + DEBUGASSERT(tcb != NULL); + DEBUGASSERT(tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE); + + flags = enter_critical_section(); + if ((tcb->cmn.flags & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_PTHREAD && + tcb->tos < CONFIG_PTHREAD_CLEANUP_STACKSIZE) + { + unsigned int ndx = tcb->tos; + + tcb->tos++; + tcb->stack[ndx].pc_cleaner = routine; + tcb->stack[ndx].pc_arg = arg; + } + + leave_critical_section(flags); +} + +/**************************************************************************** + * Name: pthread_cleanup_popall + * + * Description: + * The pthread_cleanup_popall() is an internal function that will pop and + * execute all clean-up functions. This function is only called from within + * the pthread_exit() and pthread_cancellation() logic + * + * Input Parameters: + * tcb - The TCB of the pthread that is exiting or being canceled. + * + * Returned Value: + * None + * + ****************************************************************************/ + +void pthread_cleanup_popall(FAR struct pthread_tcb_s *tcb) +{ + irqstate_t flags; + + DEBUGASSERT(tcb != NULL); + DEBUGASSERT((tcb->cmn.flags & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_PTHREAD); + + /* Pop and execute each cleanup routine */ + + flags = enter_critical_section(); + while (tcb->tos > 0) + { + pthread_cleanup_pop_tcb(tcb, 1); + } + + leave_critical_section(flags); +} + +#endif /* CONFIG_PTHREAD_CLEANUP */ diff --git a/sched/pthread/pthread_exit.c b/sched/pthread/pthread_exit.c index 15b6240ee79..545148a4eb2 100644 --- a/sched/pthread/pthread_exit.c +++ b/sched/pthread/pthread_exit.c @@ -80,6 +80,9 @@ void pthread_exit(FAR void *exit_value) sinfo("exit_value=%p\n", exit_value); + DEBUGASSERT(tcb != NULL); + DEBUGASSERT((tcb->flags & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_PTHREAD); + /* Block any signal actions that would awaken us while were * are performing the JOIN handshake. */ @@ -91,6 +94,12 @@ void pthread_exit(FAR void *exit_value) } #endif +#ifdef CONFIG_PTHREAD_CLEANUP + /* Perform any stack pthread clean-up callbacks */ + + pthread_cleanup_popall((FAR struct pthread_tcb_s *)tcb); +#endif + /* Complete pending join operations */ status = pthread_completejoin(getpid(), exit_value); diff --git a/sched/task/task_exithook.c b/sched/task/task_exithook.c index 3f75ec7ca18..94ed2786a20 100644 --- a/sched/task/task_exithook.c +++ b/sched/task/task_exithook.c @@ -75,7 +75,8 @@ static inline void task_atexit(FAR struct tcb_s *tcb) * callbacks. * * REVISIT: This is a security problem In the PROTECTED and KERNEL builds: - * We must not call the registered function in supervisor mode! + * We must not call the registered function in supervisor mode! See also + * on_exit() and pthread_cleanup_pop() callbacks. */ if (group && group->tg_nmembers == 1) @@ -138,7 +139,8 @@ static inline void task_onexit(FAR struct tcb_s *tcb, int status) * callbacks. * * REVISIT: This is a security problem In the PROTECTED and KERNEL builds: - * We must not call the registered function in supervisor mode! + * We must not call the registered function in supervisor mode! See also + * atexit() and pthread_cleanup_pop() callbacks. */ if (group && group->tg_nmembers == 1) diff --git a/syscall/syscall.csv b/syscall/syscall.csv index 8ecf52854fc..a28d0d01a65 100644 --- a/syscall/syscall.csv +++ b/syscall/syscall.csv @@ -64,6 +64,8 @@ "pthread_barrier_init","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","int","FAR pthread_barrier_t*","FAR const pthread_barrierattr_t*","unsigned int" "pthread_barrier_wait","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","int","FAR pthread_barrier_t*" "pthread_cancel","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","int","pthread_t" +"pthread_cleanup_pop","pthread.h","defined(CONFIG_PTHREAD_CLEANUP)","void","int" +"pthread_cleanup_push","pthread.h","defined(CONFIG_PTHREAD_CLEANUP)","void","pthread_cleanup_t","FAR void*" "pthread_cond_broadcast","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","int","FAR pthread_cond_t*" "pthread_cond_destroy","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","int","FAR pthread_cond_t*" "pthread_cond_init","pthread.h","!defined(CONFIG_DISABLE_PTHREAD)","int","FAR pthread_cond_t*","FAR const pthread_condattr_t*" diff --git a/syscall/syscall_lookup.h b/syscall/syscall_lookup.h index 0c16b8ce1e6..60f194fc203 100644 --- a/syscall/syscall_lookup.h +++ b/syscall/syscall_lookup.h @@ -304,6 +304,10 @@ SYSCALL_LOOKUP(up_assert, 2, STUB_up_assert) SYSCALL_LOOKUP(pthread_kill, 2, STUB_pthread_kill) SYSCALL_LOOKUP(pthread_sigmask, 3, STUB_pthread_sigmask) # endif +# ifdef CONFIG_PTHREAD_CLEANUP + SYSCALL_LOOKUP(pthread_cleanup_push, 2, STUB_pthread_cleanup_push) + SYSCALL_LOOKUP(pthread_cleanup_pop, 1, STUB_pthread_cleanup_pop) +# endif #endif /* The following are defined only if message queues are enabled */ diff --git a/syscall/syscall_stublookup.c b/syscall/syscall_stublookup.c index f655e0800d0..2b7b1eafca5 100644 --- a/syscall/syscall_stublookup.c +++ b/syscall/syscall_stublookup.c @@ -311,6 +311,10 @@ uintptr_t STUB_pthread_kill(int nbr, uintptr_t parm1, uintptr_t parm2); uintptr_t STUB_pthread_sigmask(int nbr, uintptr_t parm1, uintptr_t parm2, uintptr_t parm3); +uintptr_t STUB_pthread_cleanup_pop(int nbr, uintptr_t parm1); +uintptr_t STUB_pthread_cleanup_push(int nbr, uintptr_t parm1, + uintptr_t parm2); + /* The following are defined only if message queues are enabled */ uintptr_t STUB_mq_close(int nbr, uintptr_t parm1);