Skip to content

Commit

Permalink
Fix data race in AutoRollLogger (facebook#12436)
Browse files Browse the repository at this point in the history
Summary:
`logger_` can be destructed in `ResetLogger()` so we should access them under `mutex_`. Similarly `status_` can be updated only under `mutex_` or in constructor.

Pull Request resolved: facebook#12436

Test Plan: I tried running tsan crash test with log_file_time_to_roll = 2, but not able to repro yet. Will monitor internal crash tests.

Reviewed By: hx235

Differential Revision: D54916371

Pulled By: cbi42

fbshipit-source-id: 4a3e3b40fbc2ae242598afdbd4bed5fb8ccf8d65
  • Loading branch information
cbi42 authored and facebook-github-bot committed Mar 14, 2024
1 parent dd24bda commit e91263e
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 13 deletions.
16 changes: 7 additions & 9 deletions logging/auto_roll_logger.cc
Original file line number Diff line number Diff line change
Expand Up @@ -192,14 +192,13 @@ void AutoRollLogger::LogInternal(const char* format, ...) {
}

void AutoRollLogger::Logv(const char* format, va_list ap) {
assert(GetStatus().ok());
if (!logger_) {
return;
}

std::shared_ptr<Logger> logger;
{
MutexLock l(&mutex_);
assert(GetStatus().ok());
if (!logger_) {
return;
}
if ((kLogFileTimeToRoll > 0 && LogExpired()) ||
(kMaxLogFileSize > 0 && logger_->GetLogFileSize() >= kMaxLogFileSize)) {
RollLogFile();
Expand Down Expand Up @@ -240,10 +239,6 @@ void AutoRollLogger::WriteHeaderInfo() {
}

void AutoRollLogger::LogHeader(const char* format, va_list args) {
if (!logger_) {
return;
}

// header message are to be retained in memory. Since we cannot make any
// assumptions about the data contained in va_list, we will retain them as
// strings
Expand All @@ -253,6 +248,9 @@ void AutoRollLogger::LogHeader(const char* format, va_list args) {
va_end(tmp);

MutexLock l(&mutex_);
if (!logger_) {
return;
}
headers_.push_back(data);

// Log the original message to the current log
Expand Down
7 changes: 3 additions & 4 deletions logging/auto_roll_logger.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,12 @@ class AutoRollLogger : public Logger {
Status GetStatus() { return status_; }

size_t GetLogFileSize() const override {
if (!logger_) {
return 0;
}

std::shared_ptr<Logger> logger;
{
MutexLock l(&mutex_);
if (!logger_) {
return 0;
}
// pin down the current logger_ instance before releasing the mutex.
logger = logger_;
}
Expand Down

0 comments on commit e91263e

Please sign in to comment.