[Lldb-commits] [PATCH] D157654: [lldb] Fix data race in PipePosix
Augusto Noronha via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Fri Aug 11 10:34:59 PDT 2023
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
More information about the lldb-commits
mailing list