augusto2112 added inline comments.
================ Comment at: lldb/source/Host/posix/PipePosix.cpp:69 PipeBase::operator=(std::move(pipe_posix)); - m_fds[READ] = pipe_posix.ReleaseReadFileDescriptor(); - m_fds[WRITE] = pipe_posix.ReleaseWriteFileDescriptor(); + std::lock_guard<std::mutex> guard(m_lock); + m_fds[READ] = pipe_posix.ReleaseReadFileDescriptorUnlocked(); ---------------- bulbazord wrote: > I think this lock needs to be at the beginning. Scenario: > > Thread 1 calls the move assign operator and performs the base move assign. > Gets unscheduled before acquiring lock. > Thread 2 calls the move assign operator and performs the base move assign. > Acquires the lock, fills in `m_fds`, and returns. > Thread 1 is scheduled again, grabs the lock, and fills in `m_fds`. > `this` will have its `PipeBase` members filled in by the Thread 2 call while > `m_fds` will be filled in by the Thread 1 call. Granted, one thread may be > surprised that suddenly the thing didn't get moved in as expected, but at > least `this` will be in a consistent state. > > I think you also need to lock the mutex of `pipe_posix` at the same time. > Imagine Thread 1 is trying to access something from `pipe_posix` while Thread > 2 is trying to move it into another `PipePosix` object. (Not sure how likely > this scenario is but I'd rather be safe than sorry). > I think you also need to lock the mutex of pipe_posix at the same time. > Imagine Thread 1 is trying to access something from pipe_posix while Thread 2 > is trying to move it into another PipePosix object. (Not sure how likely this > scenario is but I'd rather be safe than sorry). Yeah I thought about this too, since `pipe_posix` is an rvalue reference I was thinking this shouldn't be necessary, but you're right, I'll lock it anyway. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157654/new/ https://reviews.llvm.org/D157654 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
