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.

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

Conversation

yamt
Copy link
Contributor

@yamt yamt commented Jun 12, 2023

the issue was pointed out by @alexcrichton in
#405

@yamt
Copy link
Contributor Author

yamt commented Jun 12, 2023

tested with this patch: yamt@f4024a1

Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

lgtm % nits

@@ -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

// 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

*
* 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

Also, align the comment style with musl.
@abrown abrown merged commit 7018e24 into WebAssembly:main Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants