From b90da3f27b4c307c77ceecea7ba225f7fede5c20 Mon Sep 17 00:00:00 2001 From: Gregory Nutt Date: Tue, 22 Dec 2015 11:48:17 -0600 Subject: [PATCH] waitpid: CRITICAL BUGFIX. Add a reference counting mechansim to prevent wait from using stale memory that was freed by the exiting task --- ChangeLog | 3 +++ arch | 2 +- configs | 2 +- include/nuttx/sched.h | 2 ++ mm/mm_heap/mm_free.c | 8 -------- mm/mm_heap/mm_zalloc.c | 4 ---- sched/group/Make.defs | 4 ++++ sched/group/group.h | 6 +++++- sched/group/group_free.c | 24 ++---------------------- sched/group/group_leave.c | 32 +++++++++++++++++--------------- sched/sched/sched_waitpid.c | 12 ++++++++---- sched/semaphore/sem_holder.c | 10 +--------- sched/semaphore/sem_wait.c | 20 -------------------- 13 files changed, 44 insertions(+), 85 deletions(-) diff --git a/ChangeLog b/ChangeLog index 88315935f4..091fc549ce 100755 --- a/ChangeLog +++ b/ChangeLog @@ -11244,4 +11244,7 @@ Entinger (2015-12-15). * configs/launchxl-tms57004: Add basic board support for TI LaunchXL- TMS57004. Not much to see there yet (2015-12-15). + * waitpid: CRITICAL BUGFIX. Add a reference count to prevent waitpid + from using stale memory released by the waited-for task group + (2015-12-22). diff --git a/arch b/arch index a8666b5bb6..1519d53e18 160000 --- a/arch +++ b/arch @@ -1 +1 @@ -Subproject commit a8666b5bb6841d514d1fccedb0071df38d78217a +Subproject commit 1519d53e182663632e4b115baa1b3b15ce9840a3 diff --git a/configs b/configs index 196f9f3f3f..b30cbace6a 160000 --- a/configs +++ b/configs @@ -1 +1 @@ -Subproject commit 196f9f3f3f067ccb3c285078a704a2a5eb37b7ea +Subproject commit b30cbace6afab3caed2dcfec09071af60a2b0d7f diff --git a/include/nuttx/sched.h b/include/nuttx/sched.h index 4eaacbd229..07ec74cd82 100644 --- a/include/nuttx/sched.h +++ b/include/nuttx/sched.h @@ -154,6 +154,7 @@ #define GROUP_FLAG_NOCLDWAIT (1 << 0) /* Bit 0: Do not retain child exit status */ #define GROUP_FLAG_ADDRENV (1 << 1) /* Bit 1: Group has an address environment */ #define GROUP_FLAG_PRIVILEGED (1 << 2) /* Bit 2: Group is privileged */ +#define GROUP_FLAG_DELETED (1 << 3) /* Bit 3: Group has been deleted but not yet freed */ /* Values for struct child_status_s ch_flags */ @@ -418,6 +419,7 @@ struct task_group_s /* waitpid support ************************************************************/ /* Simple mechanism used only when there is no support for SIGCHLD */ + uint8_t tg_nwaiters; /* Number of waiters */ sem_t tg_exitsem; /* Support for waitpid */ int *tg_statloc; /* Location to return exit status */ #endif diff --git a/mm/mm_heap/mm_free.c b/mm/mm_heap/mm_free.c index a08682676d..57cb867f8d 100644 --- a/mm/mm_heap/mm_free.c +++ b/mm/mm_heap/mm_free.c @@ -44,14 +44,6 @@ #include -/**************************************************************************** - * Pre-processor Definitions - ****************************************************************************/ - -/**************************************************************************** - * Private Functions - ****************************************************************************/ - /**************************************************************************** * Public Functions ****************************************************************************/ diff --git a/mm/mm_heap/mm_zalloc.c b/mm/mm_heap/mm_zalloc.c index deb3408d67..a9d5af9525 100644 --- a/mm/mm_heap/mm_zalloc.c +++ b/mm/mm_heap/mm_zalloc.c @@ -43,10 +43,6 @@ #include -/**************************************************************************** - * Pre-processor Definitions - ****************************************************************************/ - /**************************************************************************** * Public Functions ****************************************************************************/ diff --git a/sched/group/Make.defs b/sched/group/Make.defs index e3b4cba93b..91a025e1fd 100644 --- a/sched/group/Make.defs +++ b/sched/group/Make.defs @@ -42,6 +42,10 @@ CSRCS += task_reparent.c ifeq ($(CONFIG_SCHED_CHILD_STATUS),y) CSRCS += group_childstatus.c endif +else +ifeq ($(CONFIG_SCHED_WAITPID),y) +CSRCS += group_waiter.c +endif endif ifeq ($(CONFIG_ARCH_ADDRENV),y) diff --git a/sched/group/group.h b/sched/group/group.h index 2dc74e709a..a508d5d462 100644 --- a/sched/group/group.h +++ b/sched/group/group.h @@ -1,7 +1,7 @@ /**************************************************************************** * sched/group/group.h * - * Copyright (C) 2007-2013 Gregory Nutt. All rights reserved. + * Copyright (C) 2007-2013, 2015 Gregory Nutt. All rights reserved. * Author: Gregory Nutt * * Redistribution and use in source and binary forms, with or without @@ -102,6 +102,10 @@ int group_bind(FAR struct pthread_tcb_s *tcb); int group_join(FAR struct pthread_tcb_s *tcb); #endif void group_leave(FAR struct tcb_s *tcb); +#if defined(CONFIG_SCHED_WAITPID) && !defined(CONFIG_SCHED_HAVE_PARENT) +void group_addwaiter(FAR struct task_group_s *group); +void group_delwaiter(FAR struct task_group_s *group); +#endif #if defined(HAVE_GROUP_MEMBERS) || defined(CONFIG_ARCH_ADDRENV) FAR struct task_group_s *group_findbygid(gid_t gid); diff --git a/sched/group/group_free.c b/sched/group/group_free.c index 2a4f309739..c7b0342adb 100644 --- a/sched/group/group_free.c +++ b/sched/group/group_free.c @@ -50,26 +50,6 @@ #if (defined(CONFIG_BUILD_PROTECTED) || defined(CONFIG_BUILD_KERNEL)) && \ defined(CONFIG_MM_KERNEL_HEAP) -/**************************************************************************** - * Pre-processor Definitions - ****************************************************************************/ - -/**************************************************************************** - * Private Type Declarations - ****************************************************************************/ - -/**************************************************************************** - * Public Data - ****************************************************************************/ - -/**************************************************************************** - * Private Variables - ****************************************************************************/ - -/**************************************************************************** - * Private Function Prototypes - ****************************************************************************/ - /**************************************************************************** * Public Functions ****************************************************************************/ @@ -100,7 +80,7 @@ void group_free(FAR struct task_group_s *group, FAR void *mem) { /* It is a privileged group... use the kernel mode memory allocator */ - return kmm_free(mem); + kmm_free(mem); } else { @@ -108,7 +88,7 @@ void group_free(FAR struct task_group_s *group, FAR void *mem) * allocator. */ - return kumm_free(mem); + kumm_free(mem); } } diff --git a/sched/group/group_leave.c b/sched/group/group_leave.c index 276617c2bb..c22c0aee48 100644 --- a/sched/group/group_leave.c +++ b/sched/group/group_leave.c @@ -1,7 +1,7 @@ /**************************************************************************** * sched/group/group_leave.c * - * Copyright (C) 2013-2014 Gregory Nutt. All rights reserved. + * Copyright (C) 2013-2015 Gregory Nutt. All rights reserved. * Author: Gregory Nutt * * Redistribution and use in source and binary forms, with or without @@ -56,18 +56,6 @@ #ifdef HAVE_TASK_GROUP -/**************************************************************************** - * Pre-processor Definitions - ****************************************************************************/ - -/**************************************************************************** - * Private Types - ****************************************************************************/ - -/**************************************************************************** - * Private Data - ****************************************************************************/ - /**************************************************************************** * Private Functions ****************************************************************************/ @@ -271,9 +259,23 @@ static inline void group_release(FAR struct task_group_s *group) # endif #endif - /* Release the group container itself */ +#if defined(CONFIG_SCHED_WAITPID) && !defined(CONFIG_SCHED_HAVE_PARENT) + /* If there are threads waiting for this group to be freed, then we cannot + * yet free the memory resources. Instead just mark the group deleted + * and wait for those threads complete their waits. + */ - sched_kfree(group); + if (group->tg_nwaiters > 0) + { + group->tg_flags |= GROUP_FLAG_DELETED; + } + else +#endif + { + /* Release the group container itself */ + + sched_kfree(group); + } } /**************************************************************************** diff --git a/sched/sched/sched_waitpid.c b/sched/sched/sched_waitpid.c index 371df3f767..84e197a288 100644 --- a/sched/sched/sched_waitpid.c +++ b/sched/sched/sched_waitpid.c @@ -51,10 +51,6 @@ #ifdef CONFIG_SCHED_WAITPID -/**************************************************************************** - * Private Functions - ****************************************************************************/ - /**************************************************************************** * Public Functions ****************************************************************************/ @@ -217,6 +213,10 @@ pid_t waitpid(pid_t pid, int *stat_loc, int options) group = ctcb->group; DEBUGASSERT(group); + /* Lock this group so that it cannot be deleted until the wait completes */ + + group_addwaiter(group); + /* "If more than one thread is suspended in waitpid() awaiting termination of * the same process, exactly one thread will return the process status at the * time of the target process termination." Hmmm.. what do we return to the @@ -236,6 +236,8 @@ pid_t waitpid(pid_t pid, int *stat_loc, int options) /* Don't wait if status is not available */ ret = sem_trywait(&group->tg_exitsem); + group_delwaiter(group); + if (ret < 0) { pid = 0; @@ -246,6 +248,8 @@ pid_t waitpid(pid_t pid, int *stat_loc, int options) /* Wait if necessary for status to become available */ ret = sem_wait(&group->tg_exitsem); + group_delwaiter(group); + if (ret < 0) { /* Unlock pre-emption and return the ERROR (sem_wait has already set diff --git a/sched/semaphore/sem_holder.c b/sched/semaphore/sem_holder.c index 7a2db8d0b4..3b5b2b7286 100644 --- a/sched/semaphore/sem_holder.c +++ b/sched/semaphore/sem_holder.c @@ -68,11 +68,7 @@ typedef int (*holderhandler_t)(FAR struct semholder_s *pholder, FAR sem_t *sem, FAR void *arg); /**************************************************************************** - * Public Data - ****************************************************************************/ - -/**************************************************************************** - * Private Variables + * Private Data ****************************************************************************/ /* Preallocated holder structures */ @@ -82,10 +78,6 @@ static struct semholder_s g_holderalloc[CONFIG_SEM_PREALLOCHOLDERS]; static FAR struct semholder_s *g_freeholders; #endif -/**************************************************************************** - * Private Functions - ****************************************************************************/ - /**************************************************************************** * Name: sem_allocholder ****************************************************************************/ diff --git a/sched/semaphore/sem_wait.c b/sched/semaphore/sem_wait.c index 5804fac1c5..b7d5378295 100644 --- a/sched/semaphore/sem_wait.c +++ b/sched/semaphore/sem_wait.c @@ -48,26 +48,6 @@ #include "sched/sched.h" #include "semaphore/semaphore.h" -/**************************************************************************** - * Pre-processor Definitions - ****************************************************************************/ - -/**************************************************************************** - * Private Type Declarations - ****************************************************************************/ - -/**************************************************************************** - * Public Data - ****************************************************************************/ - -/**************************************************************************** - * Private Variables - ****************************************************************************/ - -/**************************************************************************** - * Private Functions - ****************************************************************************/ - /**************************************************************************** * Public Functions ****************************************************************************/