Skip to content

Commit 9c135f1

Browse files
JDevlieghereJDevlieghere
committed
[lldb] Fix data race in NativeFile
TSan reports the following data race: Write of size 4 at 0x000109e0b160 by thread T2 (...): #0 lldb_private::NativeFile::Close() File.cpp:329 #1 lldb_private::ConnectionFileDescriptor::Disconnect(...) ConnectionFileDescriptorPosix.cpp:232 #2 lldb_private::Communication::Disconnect(...) Communication.cpp:61 #3 lldb_private::process_gdb_remote::ProcessGDBRemote::DidExit() ProcessGDBRemote.cpp:1164 #4 lldb_private::Process::SetExitStatus(...) Process.cpp:1097 #5 lldb_private::process_gdb_remote::ProcessGDBRemote::MonitorDebugserverProcess(...) ProcessGDBRemote.cpp:3387 Previous read of size 4 at 0x000109e0b160 by main thread (...): #0 lldb_private::NativeFile::IsValid() const File.h:393 #1 lldb_private::ConnectionFileDescriptor::IsConnected() const ConnectionFileDescriptorPosix.cpp:121 #2 lldb_private::Communication::IsConnected() const Communication.cpp:79 #3 lldb_private::process_gdb_remote::GDBRemoteCommunication::WaitForPacketNoLock(...) GDBRemoteCommunication.cpp:256 #4 lldb_private::process_gdb_remote::GDBRemoteCommunication::WaitForPacketNoLock(...) GDBRemoteCommunication.cpp:244 #5 lldb_private::process_gdb_remote::GDBRemoteClientBase::SendPacketAndWaitForResponseNoLock(...) GDBRemoteClientBase.cpp:246 I originally tried fixing the problem at the ConnectionFileDescriptor level, but that operates on an IOObject which can have different thread safety guarantees depending on its implementation. For this particular issue, the problem is specific to NativeFile. NativeFile can hold a file descriptor and/or a file stream. Throughout its implementation, it checks if the descriptor or stream is valid and do some operation on it if it is. While that works in a single threaded environment, nothing prevents another thread from modifying the descriptor or stream between the IsValid check and when it's actually being used. This patch prevents such issues by returning a ValueGuard RAII object. As long as the object is in scope, the value is guaranteed by a lock. Differential revision: https://reviews.llvm.org/D157347
1 parent 13629b1 commit 9c135f1

File tree

2 files changed

+103
-45
lines changed

2 files changed

+103
-45
lines changed

‎lldb/include/lldb/Host/File.h

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -389,9 +389,7 @@ class NativeFile : public File {
389389

390390
~NativeFile() override { Close(); }
391391

392-
bool IsValid() const override {
393-
return DescriptorIsValid() || StreamIsValid();
394-
}
392+
bool IsValid() const override;
395393

396394
Status Read(void *buf, size_t &num_bytes) override;
397395
Status Write(const void *buf, size_t &num_bytes) override;
@@ -417,15 +415,37 @@ class NativeFile : public File {
417415
static bool classof(const File *file) { return file->isA(&ID); }
418416

419417
protected:
420-
bool DescriptorIsValid() const {
418+
struct ValueGuard {
419+
ValueGuard(std::mutex &m, bool b) : guard(m, std::adopt_lock), value(b) {}
420+
std::lock_guard<std::mutex> guard;
421+
bool value;
422+
operator bool() { return value; }
423+
};
424+
425+
bool DescriptorIsValidUnlocked() const {
426+
421427
return File::DescriptorIsValid(m_descriptor);
422428
}
423-
bool StreamIsValid() const { return m_stream != kInvalidStream; }
424429

425-
// Member variables
430+
bool StreamIsValidUnlocked() const { return m_stream != kInvalidStream; }
431+
432+
ValueGuard DescriptorIsValid() const {
433+
m_descriptor_mutex.lock();
434+
return ValueGuard(m_descriptor_mutex, DescriptorIsValidUnlocked());
435+
}
436+
437+
ValueGuard StreamIsValid() const {
438+
m_stream_mutex.lock();
439+
return ValueGuard(m_stream_mutex, StreamIsValidUnlocked());
440+
}
441+
426442
int m_descriptor;
427443
bool m_own_descriptor = false;
444+
mutable std::mutex m_descriptor_mutex;
445+
428446
FILE *m_stream;
447+
mutable std::mutex m_stream_mutex;
448+
429449
OpenOptions m_options{};
430450
bool m_own_stream = false;
431451
std::mutex offset_access_mutex;

‎lldb/source/Host/common/File.cpp

Lines changed: 77 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,7 @@ size_t File::Printf(const char *format, ...) {
219219
size_t File::PrintfVarArg(const char *format, va_list args) {
220220
llvm::SmallString<0> s;
221221
if (VASprintf(s, format, args)) {
222-
size_t written = s.size();;
222+
size_t written = s.size();
223223
Write(s.data(), written);
224224
return written;
225225
}
@@ -247,15 +247,21 @@ uint32_t File::GetPermissions(Status &error) const {
247247
return file_stats.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO);
248248
}
249249

250+
bool NativeFile::IsValid() const {
251+
std::scoped_lock lock(m_descriptor_mutex, m_stream_mutex);
252+
return DescriptorIsValidUnlocked() || StreamIsValidUnlocked();
253+
}
254+
250255
Expected<File::OpenOptions> NativeFile::GetOptions() const { return m_options; }
251256

252257
int NativeFile::GetDescriptor() const {
253-
if (DescriptorIsValid())
258+
if (ValueGuard descriptor_guard = DescriptorIsValid()) {
254259
return m_descriptor;
260+
}
255261

256262
// Don't open the file descriptor if we don't need to, just get it from the
257263
// stream if we have one.
258-
if (StreamIsValid()) {
264+
if (ValueGuard stream_guard = StreamIsValid()) {
259265
#if defined(_WIN32)
260266
return _fileno(m_stream);
261267
#else
@@ -272,8 +278,9 @@ IOObject::WaitableHandle NativeFile::GetWaitableHandle() {
272278
}
273279

274280
FILE *NativeFile::GetStream() {
275-
if (!StreamIsValid()) {
276-
if (DescriptorIsValid()) {
281+
ValueGuard stream_guard = StreamIsValid();
282+
if (!stream_guard) {
283+
if (ValueGuard descriptor_guard = DescriptorIsValid()) {
277284
auto mode = GetStreamOpenModeFromOptions(m_options);
278285
if (!mode)
279286
llvm::consumeError(mode.takeError());
@@ -282,9 +289,9 @@ FILE *NativeFile::GetStream() {
282289
// We must duplicate the file descriptor if we don't own it because when you
283290
// call fdopen, the stream will own the fd
284291
#ifdef _WIN32
285-
m_descriptor = ::_dup(GetDescriptor());
292+
m_descriptor = ::_dup(m_descriptor);
286293
#else
287-
m_descriptor = dup(GetDescriptor());
294+
m_descriptor = dup(m_descriptor);
288295
#endif
289296
m_own_descriptor = true;
290297
}
@@ -306,8 +313,11 @@ FILE *NativeFile::GetStream() {
306313
}
307314

308315
Status NativeFile::Close() {
316+
std::scoped_lock lock(m_descriptor_mutex, m_stream_mutex);
317+
309318
Status error;
310-
if (StreamIsValid()) {
319+
320+
if (StreamIsValidUnlocked()) {
311321
if (m_own_stream) {
312322
if (::fclose(m_stream) == EOF)
313323
error.SetErrorToErrno();
@@ -322,15 +332,17 @@ Status NativeFile::Close() {
322332
}
323333
}
324334
}
325-
if (DescriptorIsValid() && m_own_descriptor) {
335+
336+
if (DescriptorIsValidUnlocked() && m_own_descriptor) {
326337
if (::close(m_descriptor) != 0)
327338
error.SetErrorToErrno();
328339
}
329-
m_descriptor = kInvalidDescriptor;
340+
330341
m_stream = kInvalidStream;
331-
m_options = OpenOptions(0);
332342
m_own_stream = false;
343+
m_descriptor = kInvalidDescriptor;
333344
m_own_descriptor = false;
345+
m_options = OpenOptions(0);
334346
m_is_interactive = eLazyBoolCalculate;
335347
m_is_real_terminal = eLazyBoolCalculate;
336348
return error;
@@ -374,7 +386,7 @@ Status NativeFile::GetFileSpec(FileSpec &file_spec) const {
374386

375387
off_t NativeFile::SeekFromStart(off_t offset, Status *error_ptr) {
376388
off_t result = 0;
377-
if (DescriptorIsValid()) {
389+
if (ValueGuard descriptor_guard = DescriptorIsValid()) {
378390
result = ::lseek(m_descriptor, offset, SEEK_SET);
379391

380392
if (error_ptr) {
@@ -383,7 +395,10 @@ off_t NativeFile::SeekFromStart(off_t offset, Status *error_ptr) {
383395
else
384396
error_ptr->Clear();
385397
}
386-
} else if (StreamIsValid()) {
398+
return result;
399+
}
400+
401+
if (ValueGuard stream_guard = StreamIsValid()) {
387402
result = ::fseek(m_stream, offset, SEEK_SET);
388403

389404
if (error_ptr) {
@@ -392,15 +407,17 @@ off_t NativeFile::SeekFromStart(off_t offset, Status *error_ptr) {
392407
else
393408
error_ptr->Clear();
394409
}
395-
} else if (error_ptr) {
396-
error_ptr->SetErrorString("invalid file handle");
410+
return result;
397411
}
412+
413+
if (error_ptr)
414+
error_ptr->SetErrorString("invalid file handle");
398415
return result;
399416
}
400417

401418
off_t NativeFile::SeekFromCurrent(off_t offset, Status *error_ptr) {
402419
off_t result = -1;
403-
if (DescriptorIsValid()) {
420+
if (ValueGuard descriptor_guard = DescriptorIsValid()) {
404421
result = ::lseek(m_descriptor, offset, SEEK_CUR);
405422

406423
if (error_ptr) {
@@ -409,7 +426,10 @@ off_t NativeFile::SeekFromCurrent(off_t offset, Status *error_ptr) {
409426
else
410427
error_ptr->Clear();
411428
}
412-
} else if (StreamIsValid()) {
429+
return result;
430+
}
431+
432+
if (ValueGuard stream_guard = StreamIsValid()) {
413433
result = ::fseek(m_stream, offset, SEEK_CUR);
414434

415435
if (error_ptr) {
@@ -418,15 +438,17 @@ off_t NativeFile::SeekFromCurrent(off_t offset, Status *error_ptr) {
418438
else
419439
error_ptr->Clear();
420440
}
421-
} else if (error_ptr) {
422-
error_ptr->SetErrorString("invalid file handle");
441+
return result;
423442
}
443+
444+
if (error_ptr)
445+
error_ptr->SetErrorString("invalid file handle");
424446
return result;
425447
}
426448

427449
off_t NativeFile::SeekFromEnd(off_t offset, Status *error_ptr) {
428450
off_t result = -1;
429-
if (DescriptorIsValid()) {
451+
if (ValueGuard descriptor_guard = DescriptorIsValid()) {
430452
result = ::lseek(m_descriptor, offset, SEEK_END);
431453

432454
if (error_ptr) {
@@ -435,7 +457,10 @@ off_t NativeFile::SeekFromEnd(off_t offset, Status *error_ptr) {
435457
else
436458
error_ptr->Clear();
437459
}
438-
} else if (StreamIsValid()) {
460+
return result;
461+
}
462+
463+
if (ValueGuard stream_guard = StreamIsValid()) {
439464
result = ::fseek(m_stream, offset, SEEK_END);
440465

441466
if (error_ptr) {
@@ -444,26 +469,32 @@ off_t NativeFile::SeekFromEnd(off_t offset, Status *error_ptr) {
444469
else
445470
error_ptr->Clear();
446471
}
447-
} else if (error_ptr) {
448-
error_ptr->SetErrorString("invalid file handle");
449472
}
473+
474+
if (error_ptr)
475+
error_ptr->SetErrorString("invalid file handle");
450476
return result;
451477
}
452478

453479
Status NativeFile::Flush() {
454480
Status error;
455-
if (StreamIsValid()) {
481+
if (ValueGuard stream_guard = StreamIsValid()) {
456482
if (llvm::sys::RetryAfterSignal(EOF, ::fflush, m_stream) == EOF)
457483
error.SetErrorToErrno();
458-
} else if (!DescriptorIsValid()) {
459-
error.SetErrorString("invalid file handle");
484+
return error;
485+
}
486+
487+
{
488+
ValueGuard descriptor_guard = DescriptorIsValid();
489+
if (!descriptor_guard)
490+
error.SetErrorString("invalid file handle");
460491
}
461492
return error;
462493
}
463494

464495
Status NativeFile::Sync() {
465496
Status error;
466-
if (DescriptorIsValid()) {
497+
if (ValueGuard descriptor_guard = DescriptorIsValid()) {
467498
#ifdef _WIN32
468499
int err = FlushFileBuffers((HANDLE)_get_osfhandle(m_descriptor));
469500
if (err == 0)
@@ -518,14 +549,18 @@ Status NativeFile::Read(void *buf, size_t &num_bytes) {
518549
#endif
519550

520551
ssize_t bytes_read = -1;
521-
if (DescriptorIsValid()) {
522-
bytes_read = llvm::sys::RetryAfterSignal(-1, ::read, m_descriptor, buf, num_bytes);
552+
if (ValueGuard descriptor_guard = DescriptorIsValid()) {
553+
bytes_read =
554+
llvm::sys::RetryAfterSignal(-1, ::read, m_descriptor, buf, num_bytes);
523555
if (bytes_read == -1) {
524556
error.SetErrorToErrno();
525557
num_bytes = 0;
526558
} else
527559
num_bytes = bytes_read;
528-
} else if (StreamIsValid()) {
560+
return error;
561+
}
562+
563+
if (ValueGuard file_lock = StreamIsValid()) {
529564
bytes_read = ::fread(buf, 1, num_bytes, m_stream);
530565

531566
if (bytes_read == 0) {
@@ -536,10 +571,11 @@ Status NativeFile::Read(void *buf, size_t &num_bytes) {
536571
num_bytes = 0;
537572
} else
538573
num_bytes = bytes_read;
539-
} else {
540-
num_bytes = 0;
541-
error.SetErrorString("invalid file handle");
574+
return error;
542575
}
576+
577+
num_bytes = 0;
578+
error.SetErrorString("invalid file handle");
543579
return error;
544580
}
545581

@@ -577,15 +613,18 @@ Status NativeFile::Write(const void *buf, size_t &num_bytes) {
577613
#endif
578614

579615
ssize_t bytes_written = -1;
580-
if (DescriptorIsValid()) {
616+
if (ValueGuard descriptor_guard = DescriptorIsValid()) {
581617
bytes_written =
582618
llvm::sys::RetryAfterSignal(-1, ::write, m_descriptor, buf, num_bytes);
583619
if (bytes_written == -1) {
584620
error.SetErrorToErrno();
585621
num_bytes = 0;
586622
} else
587623
num_bytes = bytes_written;
588-
} else if (StreamIsValid()) {
624+
return error;
625+
}
626+
627+
if (ValueGuard stream_guard = StreamIsValid()) {
589628
bytes_written = ::fwrite(buf, 1, num_bytes, m_stream);
590629

591630
if (bytes_written == 0) {
@@ -596,12 +635,11 @@ Status NativeFile::Write(const void *buf, size_t &num_bytes) {
596635
num_bytes = 0;
597636
} else
598637
num_bytes = bytes_written;
599-
600-
} else {
601-
num_bytes = 0;
602-
error.SetErrorString("invalid file handle");
638+
return error;
603639
}
604640

641+
num_bytes = 0;
642+
error.SetErrorString("invalid file handle");
605643
return error;
606644
}
607645

0 commit comments

Comments
 (0)