Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a use-after-free bug for detached threads #420

Merged
merged 2 commits into from
Jun 21, 2023
Merged

Fix a use-after-free bug for detached threads #420

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6342344
Fix a use-after-free bug for detached threads
yamt Jun 12, 2023
6df86ea
Rename map_base_lazy_free_queue as it only keeps a single item
yamt Jun 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 33 additions & 1 deletion libc-top-half/musl/src/thread/pthread_create.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,18 @@ void __tl_sync(pthread_t td)
if (tl_lock_waiters) __wake(&__thread_list_lock, 1, 0);
}

#ifndef __wasilibc_unmodified_upstream
/* this queue has at most one entry */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Give that, I'm not sure queue is the right name for this.

I'm not sure I have any better suggestions though. Maybe deferred_deallocation and process_deferred_deallocation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed

static void *map_base_lazy_free_queue;

static void process_map_base_lazy_free_queue()
{
/* called with __tl_lock held */
free(map_base_lazy_free_queue);
map_base_lazy_free_queue = NULL;
}
#endif

#ifdef __wasilibc_unmodified_upstream
_Noreturn void __pthread_exit(void *result)
#else
Expand Down Expand Up @@ -182,7 +194,17 @@ static void __pthread_exit(void *result)
}
#else
if (state==DT_DETACHED && self->map_base) {
free(self->map_base);
// As we use malloc/free which is considerably more complex
// than mmap/munmap to call and can even require a valid
// thread context, it's difficult to implement __unmapself.
//
// Here we take an alternative approach which simply defers
// the deallocation. An obvious downside of this approach is
// that it keeps the stack longer. (possibly forever.)
// To avoid wasting too much memory, we keep the free queue
// length at most one.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we be consistent about C vs C++ comments? Below you've used C multi-line comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i followed nearby code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

process_map_base_lazy_free_queue();
map_base_lazy_free_queue = self->map_base;
// Can't use `exit()` here, because it is too high level
return;
}
Expand Down Expand Up @@ -424,6 +446,16 @@ int __pthread_create(pthread_t *restrict res, const pthread_attr_t *restrict att
if (map == MAP_FAILED) goto fail;
}
#else
/*
* Process the free queue before allocationg a new one.
* Hopefully it enables a reuse of the memory.
*
* Note: We can't perform a simple "handoff" becasue allocation
* sizes might be different. (eg. the stack size might differ)
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment style within this file seems to be:

/* first line
  * second line
  * last line */

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

__tl_lock();
process_map_base_lazy_free_queue();
__tl_unlock();
map = malloc(size);
if (!map) goto fail;
#endif
Expand Down