Skip to content

Commit 2241364

Browse files
augusto2112augusto2112
committed
[lldb] Fix data race in PipePosix
Thread sanitizer reports the following data race: ``` WARNING: ThreadSanitizer: data race (pid=43201) Write of size 4 at 0x00010520c474 by thread T1 (mutexes: write M0, write M1): #0 lldb_private::PipePosix::CloseWriteFileDescriptor() PipePosix.cpp:242 (liblldb.18.0.0git.dylib:arm64+0x414700) (BuildId: 2983976beb2637b5943bff32fd12eb8932000000200000000100000000000e00) #1 lldb_private::PipePosix::Close() PipePosix.cpp:217 (liblldb.18.0.0git.dylib:arm64+0x4144e8) (BuildId: 2983976beb2637b5943bff32fd12eb8932000000200000000100000000000e00) #2 lldb_private::ConnectionFileDescriptor::Disconnect(lldb_private::Status*) ConnectionFileDescriptorPosix.cpp:239 (liblldb.18.0.0git.dylib:arm64+0x40a620) (BuildId: 2983976beb2637b5943bff32fd12eb8932000000200000000100000000000e00) #3 lldb_private::Communication::Disconnect(lldb_private::Status*) Communication.cpp:61 (liblldb.18.0.0git.dylib:arm64+0x2a9318) (BuildId: 2983976beb2637b5943bff32fd12eb8932000000200000000100000000000e00) #4 lldb_private::process_gdb_remote::ProcessGDBRemote::DidExit() ProcessGDBRemote.cpp:1167 (liblldb.18.0.0git.dylib:arm64+0x8ed984) (BuildId: 2983976beb2637b5943bff32fd12eb8932000000200000000100000000000e00) Previous read of size 4 at 0x00010520c474 by main thread (mutexes: write M2, write M3): #0 lldb_private::PipePosix::CanWrite() const PipePosix.cpp:229 (liblldb.18.0.0git.dylib:arm64+0x4145e4) (BuildId: 2983976beb2637b5943bff32fd12eb8932000000200000000100000000000e00) #1 lldb_private::ConnectionFileDescriptor::Disconnect(lldb_private::Status*) ConnectionFileDescriptorPosix.cpp:212 (liblldb.18.0.0git.dylib:arm64+0x40a4a8) (BuildId: 2983976beb2637b5943bff32fd12eb8932000000200000000100000000000e00) #2 lldb_private::Communication::Disconnect(lldb_private::Status*) Communication.cpp:61 (liblldb.18.0.0git.dylib:arm64+0x2a9318) (BuildId: 2983976beb2637b5943bff32fd12eb8932000000200000000100000000000e00) #3 lldb_private::process_gdb_remote::GDBRemoteCommunication::WaitForPacketNoLock(StringExtractorGDBRemote&, lldb_private::Timeout<std::__1::ratio<1l, 1000000l>>, bool) GDBRemoteCommunication.cpp:373 (liblldb.18.0.0git.dylib:arm64+0x8b9c48) (BuildId: 2983976beb2637b5943bff32fd12eb8932000000200000000100000000000e00) #4 lldb_private::process_gdb_remote::GDBRemoteCommunication::WaitForPacketNoLock(StringExtractorGDBRemote&, lldb_private::Timeout<std::__1::ratio<1l, 1000000l>>, bool) GDBRemoteCommunication.cpp:243 (liblldb.18.0.0git.dylib:arm64+0x8b9904) (BuildId: 2983976beb2637b5943bff32fd12eb8932000000200000000100000000000e00) ``` Fix this by adding a mutex to PipePosix. Differential Revision: https://reviews.llvm.org/D157654
1 parent 7897a94 commit 2241364

File tree

2 files changed

+92
-19
lines changed

2 files changed

+92
-19
lines changed

‎lldb/include/lldb/Host/posix/PipePosix.h‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,8 @@
88

99
#ifndef LLDB_HOST_POSIX_PIPEPOSIX_H
1010
#define LLDB_HOST_POSIX_PIPEPOSIX_H
11-
1211
#include "lldb/Host/PipeBase.h"
12+
#include <mutex>
1313

1414
namespace lldb_private {
1515

@@ -70,7 +70,22 @@ class PipePosix : public PipeBase {
7070
size_t &bytes_read) override;
7171

7272
private:
73+
bool CanReadUnlocked() const;
74+
bool CanWriteUnlocked() const;
75+
76+
int GetReadFileDescriptorUnlocked() const;
77+
int GetWriteFileDescriptorUnlocked() const;
78+
int ReleaseReadFileDescriptorUnlocked();
79+
int ReleaseWriteFileDescriptorUnlocked();
80+
void CloseReadFileDescriptorUnlocked();
81+
void CloseWriteFileDescriptorUnlocked();
82+
void CloseUnlocked();
83+
7384
int m_fds[2];
85+
86+
/// Mutexes for m_fds;
87+
mutable std::mutex m_read_mutex;
88+
mutable std::mutex m_write_mutex;
7489
};
7590

7691
} // namespace lldb_private

‎lldb/source/Host/posix/PipePosix.cpp‎

Lines changed: 76 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -65,16 +65,20 @@ PipePosix::PipePosix(PipePosix &&pipe_posix)
6565
pipe_posix.ReleaseWriteFileDescriptor()} {}
6666

6767
PipePosix &PipePosix::operator=(PipePosix &&pipe_posix) {
68+
std::scoped_lock guard(m_read_mutex, m_write_mutex, pipe_posix.m_read_mutex,
69+
pipe_posix.m_write_mutex);
70+
6871
PipeBase::operator=(std::move(pipe_posix));
69-
m_fds[READ] = pipe_posix.ReleaseReadFileDescriptor();
70-
m_fds[WRITE] = pipe_posix.ReleaseWriteFileDescriptor();
72+
m_fds[READ] = pipe_posix.ReleaseReadFileDescriptorUnlocked();
73+
m_fds[WRITE] = pipe_posix.ReleaseWriteFileDescriptorUnlocked();
7174
return *this;
7275
}
7376

7477
PipePosix::~PipePosix() { Close(); }
7578

7679
Status PipePosix::CreateNew(bool child_processes_inherit) {
77-
if (CanRead() || CanWrite())
80+
std::scoped_lock guard(m_read_mutex, m_write_mutex);
81+
if (CanReadUnlocked() || CanWriteUnlocked())
7882
return Status(EINVAL, eErrorTypePOSIX);
7983

8084
Status error;
@@ -87,7 +91,7 @@ Status PipePosix::CreateNew(bool child_processes_inherit) {
8791
if (!child_processes_inherit) {
8892
if (!SetCloexecFlag(m_fds[0]) || !SetCloexecFlag(m_fds[1])) {
8993
error.SetErrorToErrno();
90-
Close();
94+
CloseUnlocked();
9195
return error;
9296
}
9397
}
@@ -103,7 +107,8 @@ Status PipePosix::CreateNew(bool child_processes_inherit) {
103107
}
104108

105109
Status PipePosix::CreateNew(llvm::StringRef name, bool child_process_inherit) {
106-
if (CanRead() || CanWrite())
110+
std::scoped_lock (m_read_mutex, m_write_mutex);
111+
if (CanReadUnlocked() || CanWriteUnlocked())
107112
return Status("Pipe is already opened");
108113

109114
Status error;
@@ -140,7 +145,9 @@ Status PipePosix::CreateWithUniqueName(llvm::StringRef prefix,
140145

141146
Status PipePosix::OpenAsReader(llvm::StringRef name,
142147
bool child_process_inherit) {
143-
if (CanRead() || CanWrite())
148+
std::scoped_lock (m_read_mutex, m_write_mutex);
149+
150+
if (CanReadUnlocked() || CanWriteUnlocked())
144151
return Status("Pipe is already opened");
145152

146153
int flags = O_RDONLY | O_NONBLOCK;
@@ -161,7 +168,8 @@ Status
161168
PipePosix::OpenAsWriterWithTimeout(llvm::StringRef name,
162169
bool child_process_inherit,
163170
const std::chrono::microseconds &timeout) {
164-
if (CanRead() || CanWrite())
171+
std::lock_guard guard(m_write_mutex);
172+
if (CanReadUnlocked() || CanWriteUnlocked())
165173
return Status("Pipe is already opened");
166174

167175
int flags = O_WRONLY | O_NONBLOCK;
@@ -171,7 +179,7 @@ PipePosix::OpenAsWriterWithTimeout(llvm::StringRef name,
171179
using namespace std::chrono;
172180
const auto finish_time = Now() + timeout;
173181

174-
while (!CanWrite()) {
182+
while (!CanWriteUnlocked()) {
175183
if (timeout != microseconds::zero()) {
176184
const auto dur = duration_cast<microseconds>(finish_time - Now()).count();
177185
if (dur <= 0)
@@ -196,48 +204,96 @@ PipePosix::OpenAsWriterWithTimeout(llvm::StringRef name,
196204
return Status();
197205
}
198206

199-
int PipePosix::GetReadFileDescriptor() const { return m_fds[READ]; }
207+
int PipePosix::GetReadFileDescriptor() const {
208+
std::lock_guard guard(m_read_mutex);
209+
return GetReadFileDescriptorUnlocked();
210+
}
211+
212+
int PipePosix::GetReadFileDescriptorUnlocked() const {
213+
return m_fds[READ];
214+
}
200215

201-
int PipePosix::GetWriteFileDescriptor() const { return m_fds[WRITE]; }
216+
int PipePosix::GetWriteFileDescriptor() const {
217+
std::lock_guard guard(m_write_mutex);
218+
return GetWriteFileDescriptorUnlocked();
219+
}
220+
221+
int PipePosix::GetWriteFileDescriptorUnlocked() const {
222+
return m_fds[WRITE];
223+
}
202224

203225
int PipePosix::ReleaseReadFileDescriptor() {
226+
std::lock_guard guard(m_read_mutex);
227+
return ReleaseReadFileDescriptorUnlocked();
228+
}
229+
230+
int PipePosix::ReleaseReadFileDescriptorUnlocked() {
204231
const int fd = m_fds[READ];
205232
m_fds[READ] = PipePosix::kInvalidDescriptor;
206233
return fd;
207234
}
208235

209236
int PipePosix::ReleaseWriteFileDescriptor() {
237+
std::lock_guard guard(m_write_mutex);
238+
return ReleaseWriteFileDescriptorUnlocked();
239+
}
240+
241+
int PipePosix::ReleaseWriteFileDescriptorUnlocked() {
210242
const int fd = m_fds[WRITE];
211243
m_fds[WRITE] = PipePosix::kInvalidDescriptor;
212244
return fd;
213245
}
214246

215247
void PipePosix::Close() {
216-
CloseReadFileDescriptor();
217-
CloseWriteFileDescriptor();
248+
std::scoped_lock guard(m_read_mutex, m_write_mutex);
249+
CloseUnlocked();
250+
}
251+
252+
void PipePosix::CloseUnlocked() {
253+
CloseReadFileDescriptorUnlocked();
254+
CloseWriteFileDescriptorUnlocked();
218255
}
219256

220257
Status PipePosix::Delete(llvm::StringRef name) {
221258
return llvm::sys::fs::remove(name);
222259
}
223260

224261
bool PipePosix::CanRead() const {
262+
std::lock_guard guard(m_read_mutex);
263+
return CanReadUnlocked();
264+
}
265+
266+
bool PipePosix::CanReadUnlocked() const {
225267
return m_fds[READ] != PipePosix::kInvalidDescriptor;
226268
}
227269

228270
bool PipePosix::CanWrite() const {
271+
std::lock_guard guard(m_write_mutex);
272+
return CanWriteUnlocked();
273+
}
274+
275+
bool PipePosix::CanWriteUnlocked() const {
229276
return m_fds[WRITE] != PipePosix::kInvalidDescriptor;
230277
}
231278

232279
void PipePosix::CloseReadFileDescriptor() {
233-
if (CanRead()) {
280+
std::lock_guard guard(m_read_mutex);
281+
CloseReadFileDescriptorUnlocked();
282+
}
283+
void PipePosix::CloseReadFileDescriptorUnlocked() {
284+
if (CanReadUnlocked()) {
234285
close(m_fds[READ]);
235286
m_fds[READ] = PipePosix::kInvalidDescriptor;
236287
}
237288
}
238289

239290
void PipePosix::CloseWriteFileDescriptor() {
240-
if (CanWrite()) {
291+
std::lock_guard guard(m_write_mutex);
292+
CloseWriteFileDescriptorUnlocked();
293+
}
294+
295+
void PipePosix::CloseWriteFileDescriptorUnlocked() {
296+
if (CanWriteUnlocked()) {
241297
close(m_fds[WRITE]);
242298
m_fds[WRITE] = PipePosix::kInvalidDescriptor;
243299
}
@@ -246,11 +302,12 @@ void PipePosix::CloseWriteFileDescriptor() {
246302
Status PipePosix::ReadWithTimeout(void *buf, size_t size,
247303
const std::chrono::microseconds &timeout,
248304
size_t &bytes_read) {
305+
std::lock_guard guard(m_read_mutex);
249306
bytes_read = 0;
250-
if (!CanRead())
307+
if (!CanReadUnlocked())
251308
return Status(EINVAL, eErrorTypePOSIX);
252309

253-
const int fd = GetReadFileDescriptor();
310+
const int fd = GetReadFileDescriptorUnlocked();
254311

255312
SelectHelper select_helper;
256313
select_helper.SetTimeout(timeout);
@@ -278,11 +335,12 @@ Status PipePosix::ReadWithTimeout(void *buf, size_t size,
278335
}
279336

280337
Status PipePosix::Write(const void *buf, size_t size, size_t &bytes_written) {
338+
std::lock_guard guard(m_write_mutex);
281339
bytes_written = 0;
282-
if (!CanWrite())
340+
if (!CanWriteUnlocked())
283341
return Status(EINVAL, eErrorTypePOSIX);
284342

285-
const int fd = GetWriteFileDescriptor();
343+
const int fd = GetWriteFileDescriptorUnlocked();
286344
SelectHelper select_helper;
287345
select_helper.SetTimeout(std::chrono::seconds(0));
288346
select_helper.FDSetWrite(fd);

0 commit comments

Comments
 (0)