Skip to content

Commit af11ba0

Browse files
rkennkerkennke
authored
[PROF-12990] Improve locking and prevent use-after-free in FlightRecorder
1 parent 8070f3c commit af11ba0

3 files changed

Lines changed: 132 additions & 92 deletions

File tree

‎ddprof-lib/src/main/cpp/flightRecorder.cpp‎

Lines changed: 103 additions & 87 deletions
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,6 @@
4040
#include <vector>
4141
#include <unistd.h>
4242

43-
static SpinLock _rec_lock(0);
44-
4543
static const char *const SETTING_RING[] = {NULL, "kernel", "user", "any"};
4644
static const char *const SETTING_CSTACK[] = {NULL, "no", "fp", "dwarf", "lbr"};
4745

@@ -1484,159 +1482,177 @@ Error FlightRecorder::newRecording(bool reset) {
14841482
return Error("Could not open Flight Recorder output file");
14851483
}
14861484

1487-
// Given some of reads are not protected by _rec_lock,
1488-
// we want to publish _rec with full fence, so that read
1489-
// side cannot see partially initialized recording.
1490-
Recording* tmp = new Recording(fd, _args);
1491-
__atomic_store_n(&_rec, tmp, __ATOMIC_SEQ_CST);
1485+
_rec = new Recording(fd, _args);
14921486
return Error::OK;
14931487
}
14941488

14951489
void FlightRecorder::stop() {
14961490
ExclusiveLockGuard locker(&_rec_lock);
1497-
if (_rec != NULL) {
1498-
volatile Recording *tmp = _rec;
1491+
Recording* rec = _rec;
1492+
if (rec != nullptr) {
14991493
// NULL first, deallocate later
1500-
__atomic_store_n(&_rec, nullptr, __ATOMIC_RELAXED);
1501-
delete tmp;
1494+
_rec = nullptr;
1495+
delete rec;
15021496
}
15031497
}
15041498

15051499
Error FlightRecorder::dump(const char *filename, const int length) {
15061500
ExclusiveLockGuard locker(&_rec_lock);
1507-
if (_rec != NULL) {
1501+
Recording* rec = _rec;
1502+
if (rec != nullptr) {
15081503
if (_filename.length() != length ||
15091504
strncmp(filename, _filename.c_str(), length) != 0) {
15101505
// if the filename to dump the recording to is specified move the current
15111506
// working file there
15121507
int copy_fd = open(filename, O_CREAT | O_RDWR | O_TRUNC, 0644);
1513-
_rec->switchChunk(copy_fd);
1508+
rec->switchChunk(copy_fd);
15141509
close(copy_fd);
1515-
} else {
1516-
return Error(
1517-
"Can not dump recording to itself. Provide a different file name!");
1510+
return Error::OK;
15181511
}
1519-
1520-
return Error::OK;
1521-
} else {
1522-
return Error("No active recording");
1512+
return Error(
1513+
"Can not dump recording to itself. Provide a different file name!");
15231514
}
1515+
return Error("No active recording");
15241516
}
15251517

15261518
void FlightRecorder::flush() {
15271519
ExclusiveLockGuard locker(&_rec_lock);
1528-
if (_rec != NULL) {
1529-
jvmtiEnv *jvmti = VM::jvmti();
1530-
JNIEnv *env = VM::jni();
1520+
Recording* rec = _rec;
1521+
if (rec != nullptr) {
1522+
jvmtiEnv* jvmti = VM::jvmti();
1523+
JNIEnv* env = VM::jni();
15311524

1532-
jclass **classes = NULL;
1525+
jclass** classes = NULL;
15331526
jint count = 0;
15341527
// obtaining the class list will create local refs to all loaded classes,
15351528
// effectively preventing them from being unloaded while flushing
15361529
jvmtiError err = jvmti->GetLoadedClasses(&count, classes);
1537-
_rec->switchChunk(-1);
1530+
rec->switchChunk(-1);
15381531
if (!err) {
15391532
// deallocate all loaded classes
15401533
for (int i = 0; i < count; i++) {
1541-
env->DeleteLocalRef((jobject)classes[i]);
1542-
jvmti->Deallocate((unsigned char *)classes[i]);
1534+
env->DeleteLocalRef((jobject) classes[i]);
1535+
jvmti->Deallocate((unsigned char*) classes[i]);
15431536
}
15441537
}
15451538
}
15461539
}
15471540

15481541
void FlightRecorder::wallClockEpoch(int lock_index,
15491542
WallClockEpochEvent *event) {
1550-
if (_rec != NULL) {
1551-
Buffer *buf = _rec->buffer(lock_index);
1552-
_rec->recordWallClockEpoch(buf, event);
1543+
OptionalSharedLockGuard locker(&_rec_lock);
1544+
if (locker.ownsLock()) {
1545+
Recording* rec = _rec;
1546+
if (rec != nullptr) {
1547+
Buffer *buf = rec->buffer(lock_index);
1548+
rec->recordWallClockEpoch(buf, event);
1549+
}
15531550
}
15541551
}
15551552

15561553
void FlightRecorder::recordTraceRoot(int lock_index, int tid,
15571554
TraceRootEvent *event) {
1558-
if (_rec != NULL) {
1559-
Buffer *buf = _rec->buffer(lock_index);
1560-
_rec->recordTraceRoot(buf, tid, event);
1555+
OptionalSharedLockGuard locker(&_rec_lock);
1556+
if (locker.ownsLock()) {
1557+
Recording* rec = _rec;
1558+
if (rec != nullptr) {
1559+
Buffer *buf = rec->buffer(lock_index);
1560+
rec->recordTraceRoot(buf, tid, event);
1561+
}
15611562
}
15621563
}
15631564

15641565
void FlightRecorder::recordQueueTime(int lock_index, int tid,
15651566
QueueTimeEvent *event) {
1566-
if (_rec != NULL) {
1567-
Buffer *buf = _rec->buffer(lock_index);
1568-
_rec->recordQueueTime(buf, tid, event);
1567+
OptionalSharedLockGuard locker(&_rec_lock);
1568+
if (locker.ownsLock()) {
1569+
Recording* rec = _rec;
1570+
if (rec != nullptr) {
1571+
Buffer *buf = rec->buffer(lock_index);
1572+
rec->recordQueueTime(buf, tid, event);
1573+
}
15691574
}
15701575
}
15711576

15721577
void FlightRecorder::recordDatadogSetting(int lock_index, int length,
15731578
const char *name, const char *value,
15741579
const char *unit) {
1575-
if (_rec != NULL) {
1576-
Buffer *buf = _rec->buffer(lock_index);
1577-
_rec->writeDatadogSetting(buf, length, name, value, unit);
1580+
OptionalSharedLockGuard locker(&_rec_lock);
1581+
if (locker.ownsLock()) {
1582+
Recording* rec = _rec;
1583+
if (rec != nullptr) {
1584+
Buffer *buf = rec->buffer(lock_index);
1585+
rec->writeDatadogSetting(buf, length, name, value, unit);
1586+
}
15781587
}
15791588
}
15801589

15811590
void FlightRecorder::recordHeapUsage(int lock_index, long value, bool live) {
1582-
if (_rec != NULL) {
1583-
Buffer *buf = _rec->buffer(lock_index);
1584-
_rec->writeHeapUsage(buf, value, live);
1591+
OptionalSharedLockGuard locker(&_rec_lock);
1592+
if (locker.ownsLock()) {
1593+
Recording* rec = _rec;
1594+
if (rec != nullptr) {
1595+
Buffer *buf = rec->buffer(lock_index);
1596+
rec->writeHeapUsage(buf, value, live);
1597+
}
15851598
}
15861599
}
15871600

15881601
void FlightRecorder::recordEvent(int lock_index, int tid, u64 call_trace_id,
15891602
int event_type, Event *event) {
1590-
if (_rec != NULL) {
1591-
RecordingBuffer *buf = _rec->buffer(lock_index);
1592-
switch (event_type) {
1593-
case 0:
1594-
_rec->recordExecutionSample(buf, tid, call_trace_id,
1603+
OptionalSharedLockGuard locker(&_rec_lock);
1604+
if (locker.ownsLock()) {
1605+
Recording* rec = _rec;
1606+
if (rec != nullptr) {
1607+
RecordingBuffer *buf = rec->buffer(lock_index);
1608+
switch (event_type) {
1609+
case 0:
1610+
rec->recordExecutionSample(buf, tid, call_trace_id,
1611+
(ExecutionEvent *)event);
1612+
break;
1613+
case BCI_WALL:
1614+
rec->recordMethodSample(buf, tid, call_trace_id,
15951615
(ExecutionEvent *)event);
1596-
break;
1597-
case BCI_WALL:
1598-
_rec->recordMethodSample(buf, tid, call_trace_id,
1599-
(ExecutionEvent *)event);
1600-
break;
1601-
case BCI_ALLOC:
1602-
_rec->recordAllocation(buf, tid, call_trace_id, (AllocEvent *)event);
1603-
break;
1604-
case BCI_LIVENESS:
1605-
_rec->recordHeapLiveObject(buf, tid, call_trace_id,
1606-
(ObjectLivenessEvent *)event);
1607-
break;
1608-
case BCI_LOCK:
1609-
_rec->recordMonitorBlocked(buf, tid, call_trace_id, (LockEvent *)event);
1610-
break;
1611-
case BCI_PARK:
1612-
_rec->recordThreadPark(buf, tid, call_trace_id, (LockEvent *)event);
1613-
break;
1614-
}
1615-
_rec->flushIfNeeded(buf);
1616-
_rec->addThread(lock_index, tid);
1616+
break;
1617+
case BCI_ALLOC:
1618+
rec->recordAllocation(buf, tid, call_trace_id, (AllocEvent *)event);
1619+
break;
1620+
case BCI_LIVENESS:
1621+
rec->recordHeapLiveObject(buf, tid, call_trace_id,
1622+
(ObjectLivenessEvent *)event);
1623+
break;
1624+
case BCI_LOCK:
1625+
rec->recordMonitorBlocked(buf, tid, call_trace_id, (LockEvent *)event);
1626+
break;
1627+
case BCI_PARK:
1628+
rec->recordThreadPark(buf, tid, call_trace_id, (LockEvent *)event);
1629+
break;
1630+
}
1631+
rec->flushIfNeeded(buf);
1632+
rec->addThread(lock_index, tid);
1633+
}
16171634
}
16181635
}
16191636

16201637
void FlightRecorder::recordLog(LogLevel level, const char *message,
16211638
size_t len) {
1622-
if (!_rec_lock.tryLockShared()) {
1623-
// No active recording
1624-
return;
1625-
}
1626-
1627-
if (len > MAX_STRING_LENGTH)
1628-
len = MAX_STRING_LENGTH;
1629-
// cppcheck-suppress obsoleteFunctions
1630-
Buffer *buf = (Buffer *)alloca(len + 40);
1631-
buf->reset();
1639+
OptionalSharedLockGuard locker(&_rec_lock);
1640+
if (locker.ownsLock()) {
1641+
Recording* rec = _rec;
1642+
if (rec != nullptr) {
1643+
if (len > MAX_STRING_LENGTH)
1644+
len = MAX_STRING_LENGTH;
1645+
// cppcheck-suppress obsoleteFunctions
1646+
Buffer *buf = (Buffer *)alloca(len + 40);
1647+
buf->reset();
16321648

1633-
int start = buf->skip(5);
1634-
buf->putVar64(T_LOG);
1635-
buf->putVar64(TSC::ticks());
1636-
buf->putVar64(level);
1637-
buf->putUtf8(message, len);
1638-
buf->putVar32(start, buf->offset() - start);
1639-
_rec->flush(buf);
1640-
1641-
_rec_lock.unlockShared();
1649+
int start = buf->skip(5);
1650+
buf->putVar64(T_LOG);
1651+
buf->putVar64(TSC::ticks());
1652+
buf->putVar64(level);
1653+
buf->putUtf8(message, len);
1654+
buf->putVar32(start, buf->offset() - start);
1655+
_rec->flush(buf);
1656+
}
1657+
}
16421658
}

‎ddprof-lib/src/main/cpp/flightRecorder.h‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -291,7 +291,9 @@ class FlightRecorder {
291291
private:
292292
std::string _filename;
293293
Arguments _args;
294-
Recording* volatile _rec;
294+
295+
SpinLock _rec_lock;
296+
Recording* _rec;
295297

296298
Error newRecording(bool reset);
297299

‎ddprof-lib/src/main/cpp/spinLock.h‎

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ class alignas(DEFAULT_CACHE_LINE_SIZE) SpinLock {
3030
volatile int _lock;
3131
char _padding[DEFAULT_CACHE_LINE_SIZE - sizeof(_lock)];
3232
public:
33-
constexpr SpinLock(int initial_state = 0) : _lock(initial_state), _padding() {
33+
explicit constexpr SpinLock(int initial_state = 0) : _lock(initial_state), _padding() {
3434
static_assert(sizeof(SpinLock) == DEFAULT_CACHE_LINE_SIZE);
3535
}
3636

@@ -48,8 +48,7 @@ class alignas(DEFAULT_CACHE_LINE_SIZE) SpinLock {
4848

4949
bool tryLockShared() {
5050
int value;
51-
// we use relaxed as the compare already offers the guarantees we need
52-
while ((value = __atomic_load_n(&_lock, __ATOMIC_RELAXED)) <= 0) {
51+
while ((value = __atomic_load_n(&_lock, __ATOMIC_ACQUIRE)) <= 0) {
5352
if (__sync_bool_compare_and_swap(&_lock, value, value - 1)) {
5453
return true;
5554
}
@@ -59,7 +58,7 @@ class alignas(DEFAULT_CACHE_LINE_SIZE) SpinLock {
5958

6059
void lockShared() {
6160
int value;
62-
while ((value = __atomic_load_n(&_lock, __ATOMIC_RELAXED)) > 0 ||
61+
while ((value = __atomic_load_n(&_lock, __ATOMIC_ACQUIRE)) > 0 ||
6362
!__sync_bool_compare_and_swap(&_lock, value, value - 1)) {
6463
spinPause();
6564
}
@@ -86,6 +85,29 @@ class SharedLockGuard {
8685
SharedLockGuard& operator=(SharedLockGuard&&) = delete;
8786
};
8887

88+
class OptionalSharedLockGuard {
89+
SpinLock* _lock;
90+
public:
91+
OptionalSharedLockGuard(SpinLock* lock) : _lock(lock) {
92+
if (!_lock->tryLockShared()) {
93+
// Locking failed, no need to unlock.
94+
_lock = nullptr;
95+
}
96+
}
97+
~OptionalSharedLockGuard() {
98+
if (_lock != nullptr) {
99+
_lock->unlockShared();
100+
}
101+
}
102+
bool ownsLock() { return _lock != nullptr; }
103+
104+
// Non-copyable and non-movable
105+
OptionalSharedLockGuard(const OptionalSharedLockGuard&) = delete;
106+
OptionalSharedLockGuard& operator=(const OptionalSharedLockGuard&) = delete;
107+
OptionalSharedLockGuard(OptionalSharedLockGuard&&) = delete;
108+
OptionalSharedLockGuard& operator=(OptionalSharedLockGuard&&) = delete;
109+
};
110+
89111
class ExclusiveLockGuard {
90112
private:
91113
SpinLock* _lock;

0 commit comments

Comments
 (0)