From 1798e3870bea9d56cbef9a5df032e5dcc2f113f9 Mon Sep 17 00:00:00 2001 From: borysp Date: Thu, 26 Nov 2020 15:35:16 +0100 Subject: [PATCH] [LibOS] Fix `shim_ipc_info` leak This PR has one caveat: it makes pids reusable i.e. they might not be growing (Graphene might reuse old, freed pids). --- LibOS/shim/src/bookkeep/shim_process.c | 3 +++ LibOS/shim/src/ipc/shim_ipc_ranges.c | 2 +- LibOS/shim/src/sys/shim_clone.c | 4 +++- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/LibOS/shim/src/bookkeep/shim_process.c b/LibOS/shim/src/bookkeep/shim_process.c index 32753543..bced8a12 100644 --- a/LibOS/shim/src/bookkeep/shim_process.c +++ b/LibOS/shim/src/bookkeep/shim_process.c @@ -77,6 +77,9 @@ struct shim_child_process* create_child_process(void) { void destroy_child_process(struct shim_child_process* child) { assert(LIST_EMPTY(child, list)); + /* This only removes the pid from internal IPC tracking in this process. */ + release_ipc_id(child->pid); + free(child); } diff --git a/LibOS/shim/src/ipc/shim_ipc_ranges.c b/LibOS/shim/src/ipc/shim_ipc_ranges.c index dd552146..cbc78fee 100644 --- a/LibOS/shim/src/ipc/shim_ipc_ranges.c +++ b/LibOS/shim/src/ipc/shim_ipc_ranges.c @@ -272,7 +272,7 @@ static int add_ipc_range(IDTYPE base, IDTYPE owner, const char* uri) { static void __del_ipc_subrange(struct subrange** ptr) { struct subrange* s = *ptr; *ptr = NULL; - put_ipc_info(s->owner); + put_ipc_info_in_list(s->owner); free(s); nsubed--; } diff --git a/LibOS/shim/src/sys/shim_clone.c b/LibOS/shim/src/sys/shim_clone.c index 7c3d2952..49407e42 100644 --- a/LibOS/shim/src/sys/shim_clone.c +++ b/LibOS/shim/src/sys/shim_clone.c @@ -369,7 +369,9 @@ long shim_do_clone(unsigned long flags, unsigned long user_stack_addr, int* pare /* We should not have saved any references to this thread anywhere and `put_thread` below * should free it. */ assert(__atomic_load_n(&thread->ref_count.counter, __ATOMIC_RELAXED) == 1); - /* We do not want to release `tid`, now it's owned by child process. */ + /* We do not want to release `tid` now, we will do this once the child process dies. This + * makes little sense (as in theory we've passed the ownership of this id to the child), but + * IPC code is a mess, so that's what we have to do. */ thread->tid = 0; put_thread(thread);