[libc-commits] [PATCH] D148294: [LIBC] Handle multiple calls to `detach` more gracefully

[Noah Goldstein via Phabricator via libc-commits]

goldstein.w.n added inline comments.


================
Comment at: libc/src/__support/threads/linux/thread.cpp:355
-
-  cleanup_thread_resources(attrib);
-
----------------
sivachandra wrote:
> goldstein.w.n wrote:
> > sivachandra wrote:
> > > goldstein.w.n wrote:
> > > > sivachandra wrote:
> > > > > A joinable but exiting thread will not cleanup its resources if we remove this. What problem is your patch trying to address?
> > > > Hmm? AFAICT this is only the handle for `pthread_detach ()`, how does it affect the cleanup routine? Its just handling the UB case of multiple calls to `pthread_detach ()` without almost certainty causing an infinite loop.
> > > > 
> > > > If thread is joinable then `Thread::join()` does cleanup.
> > > > If thread is not joinable then `thread_exist()` does cleanup.
> > > > 
> > > > 
> > > > Hmm? AFAICT this is only the handle for `pthread_detach ()`, how does it affect the cleanup routine? Its just handling the UB case of multiple calls to `pthread_detach ()` without almost certainty causing an infinite loop.
> > > > 
> > > > If thread is joinable then `Thread::join()` does cleanup.
> > > > If thread is not joinable then `thread_exist()` does cleanup.
> > > 
> > > Consider this:
> > > 
> > > A joinable thread has finished so its status has changed to `EXITING` but no other thread has called `join` or `detach` yet. In this case, `thread_exit` will not cleanup thread's resources as the call to one of `join` or `detach` will have to succeed. If `detach` is called, then cleanup becomes its responsibility. Likewise, if `join` is called, then cleanup becomes its responsibility.
> > > 
> > > So, this cleanup here is for the case when `detach` is called on an already finished thread.
> > > > Hmm? AFAICT this is only the handle for `pthread_detach ()`, how does it affect the cleanup routine? Its just handling the UB case of multiple calls to `pthread_detach ()` without almost certainty causing an infinite loop.
> > > > 
> > > > If thread is joinable then `Thread::join()` does cleanup.
> > > > If thread is not joinable then `thread_exist()` does cleanup.
> > > 
> > > Consider this:
> > > 
> > > A joinable thread has finished so its status has changed to `EXITING` but no other thread has called `join` or `detach` yet. In this case, `thread_exit` will not cleanup thread's resources as the call to one of `join` or `detach` will have to succeed. If `detach` is called, then cleanup becomes its responsibility. Likewise, if `join` is called, then cleanup becomes its responsibility.
> > > 
> > > So, this cleanup here is for the case when `detach` is called on an already finished thread.
> > 
> > Okay, thats fair. Was thinking of detached as only on `pthread_self()`. How about just return `EINVAL` if current state is `DETACHED`? The thing is this is a not super uncommon race and failing with infinite loop just seems unpleasant / harder to detect for the user.
> So, IIUC, what you are saying is that, before the call to `wait`, you want to add something like this:
> 
> ```
> if (joinable_state == uint32_t(DetachState::DETACHED))
>   return EINVAL;
> ```
> 
> I think it is OK but I am not sure it would be the cleanest thing to do. Consider a case where user code is calling `detach` twice on a thread. If the second `detach` happens after the thread has finished and cleaned up itself, then what you read into `joinable_state` is garbage. In fact, the program should crash because thread attrib's are on the thread stack which was likely already unmaped. So, that conditional is assuming that the thread was still running when the attribs were read. I am not sure making such assumptions is the correct thing to do.
> So, IIUC, what you are saying is that, before the call to `wait`, you want to add something like this:
> 
> ```
> if (joinable_state == uint32_t(DetachState::DETACHED))
>   return EINVAL;
> ```
> 
> I think it is OK but I am not sure it would be the cleanest thing to do. Consider a case where user code is calling `detach` twice on a thread. If the second `detach` happens after the thread has finished and cleaned up itself, then what you read into `joinable_state` is garbage. In fact, the program should crash because thread attrib's are on the thread stack which was likely already unmaped. So, that conditional is assuming that the thread was still running when the attribs were read. I am not sure making such assumptions is the correct thing to do.

Well its UB either way. We are still reading from attribs on the second detach when we try the cas from `JOINABLE`. In the UB case we aren't making string guarantees about whats going to happen, but we can at least try and handle it gracefully.

I don't have any strong opinion about whether we handle the case with `abort` or `EINVAL`, I just think `wait` which can cause an infinite loop (if the thread is calling detach on itself) is not really a great way to handle a not too uncommon bug. I tend towards `EINVAL` so its up to the user if they want to `abort` or handle however, but `abort` seems perfectly reasonable as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148294/new/

https://reviews.llvm.org/D148294