Skip to content

Commit e2df6e3

Browse files
sfc-gh-jshimsfc-gh-jshim
authored
Wipe packet buffers that held serialized WipedString (#10018)
* Extend WipedString guarantees to serialized packets * Apply review suggestions
1 parent a099d37 commit e2df6e3

File tree

11 files changed

+505
-101
lines changed

11 files changed

+505
-101
lines changed

‎fdbserver/workloads/UnitTests.actor.cpp‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ void forceLinkArenaStringTests();
4949
void forceLinkActorCollectionTests();
5050
void forceLinkDDSketchTests();
5151
void forceLinkCommitProxyTests();
52+
void forceLinkWipedStringTests();
5253

5354
struct UnitTestWorkload : TestWorkload {
5455
static constexpr auto NAME = "UnitTests";
@@ -113,6 +114,7 @@ struct UnitTestWorkload : TestWorkload {
113114
forceLinkArenaStringTests();
114115
forceLinkActorCollectionTests();
115116
forceLinkDDSketchTests();
117+
forceLinkWipedStringTests();
116118
}
117119

118120
Future<Void> setup(Database const& cx) override {

‎flow/Arena.cpp‎

Lines changed: 12 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -344,22 +344,6 @@ void* ArenaBlock::allocate(Reference<ArenaBlock>& self, int bytes, IsSecureMem i
344344
return result;
345345
}
346346

347-
force_inline ArenaBlock* newBigArenaBlock(int size) {
348-
if (keepalive_allocator::isActive()) [[unlikely]] {
349-
return (ArenaBlock*)keepalive_allocator::allocate(size);
350-
} else {
351-
return (ArenaBlock*)new uint8_t[size];
352-
}
353-
}
354-
355-
force_inline void deleteBigArenaBlock(ArenaBlock* block) {
356-
if (keepalive_allocator::isActive()) [[unlikely]] {
357-
return keepalive_allocator::invalidate(block);
358-
} else {
359-
return delete[] reinterpret_cast<uint8_t*>(block);
360-
}
361-
}
362-
363347
// Return an appropriately-sized ArenaBlock to store the given data
364348
ArenaBlock* ArenaBlock::create(int dataSize, Reference<ArenaBlock>& next) {
365349
ArenaBlock* b;
@@ -402,23 +386,23 @@ ArenaBlock* ArenaBlock::create(int dataSize, Reference<ArenaBlock>& next) {
402386
b->bigSize = 256;
403387
INSTRUMENT_ALLOCATE("Arena256");
404388
} else if (reqSize <= 512) {
405-
b = newBigArenaBlock(512);
389+
b = (ArenaBlock*)allocateAndMaybeKeepalive(512);
406390
b->bigSize = 512;
407391
INSTRUMENT_ALLOCATE("Arena512");
408392
} else if (reqSize <= 1024) {
409-
b = newBigArenaBlock(1024);
393+
b = (ArenaBlock*)allocateAndMaybeKeepalive(1024);
410394
b->bigSize = 1024;
411395
INSTRUMENT_ALLOCATE("Arena1024");
412396
} else if (reqSize <= 2048) {
413-
b = newBigArenaBlock(2048);
397+
b = (ArenaBlock*)allocateAndMaybeKeepalive(2048);
414398
b->bigSize = 2048;
415399
INSTRUMENT_ALLOCATE("Arena2048");
416400
} else if (reqSize <= 4096) {
417-
b = newBigArenaBlock(4096);
401+
b = (ArenaBlock*)allocateAndMaybeKeepalive(4096);
418402
b->bigSize = 4096;
419403
INSTRUMENT_ALLOCATE("Arena4096");
420404
} else {
421-
b = newBigArenaBlock(8192);
405+
b = (ArenaBlock*)allocateAndMaybeKeepalive(8192);
422406
b->bigSize = 8192;
423407
INSTRUMENT_ALLOCATE("Arena8192");
424408
}
@@ -430,7 +414,7 @@ ArenaBlock* ArenaBlock::create(int dataSize, Reference<ArenaBlock>& next) {
430414
#ifdef ALLOC_INSTRUMENTATION
431415
allocInstr["ArenaHugeKB"].alloc((reqSize + 1023) >> 10);
432416
#endif
433-
b = newBigArenaBlock(reqSize);
417+
b = (ArenaBlock*)allocateAndMaybeKeepalive(reqSize);
434418
b->tinySize = b->tinyUsed = NOT_TINY;
435419
b->bigSize = reqSize;
436420
b->totalSizeEstimate = b->bigSize;
@@ -522,26 +506,26 @@ void ArenaBlock::destroyLeaf() {
522506
FastAllocator<256>::release(this);
523507
INSTRUMENT_RELEASE("Arena256");
524508
} else if (bigSize <= 512) {
525-
deleteBigArenaBlock(this);
509+
freeOrMaybeKeepalive(this);
526510
INSTRUMENT_RELEASE("Arena512");
527511
} else if (bigSize <= 1024) {
528-
deleteBigArenaBlock(this);
512+
freeOrMaybeKeepalive(this);
529513
INSTRUMENT_RELEASE("Arena1024");
530514
} else if (bigSize <= 2048) {
531-
deleteBigArenaBlock(this);
515+
freeOrMaybeKeepalive(this);
532516
INSTRUMENT_RELEASE("Arena2048");
533517
} else if (bigSize <= 4096) {
534-
deleteBigArenaBlock(this);
518+
freeOrMaybeKeepalive(this);
535519
INSTRUMENT_RELEASE("Arena4096");
536520
} else if (bigSize <= 8192) {
537-
deleteBigArenaBlock(this);
521+
freeOrMaybeKeepalive(this);
538522
INSTRUMENT_RELEASE("Arena8192");
539523
} else {
540524
#ifdef ALLOC_INSTRUMENTATION
541525
allocInstr["ArenaHugeKB"].dealloc((bigSize + 1023) >> 10);
542526
#endif
543527
g_hugeArenaMemory.fetch_sub(bigSize);
544-
deleteBigArenaBlock(this);
528+
freeOrMaybeKeepalive(this);
545529
}
546530
}
547531
}

‎flow/FastAlloc.cpp‎

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -310,6 +310,7 @@ namespace detail {
310310

311311
std::set<void*> g_allocatedSet;
312312
std::set<void*> g_freedSet;
313+
std::vector<std::pair<const uint8_t*, int>> g_wipedSet;
313314
bool g_active = false;
314315

315316
} // namespace detail
@@ -319,33 +320,52 @@ ActiveScope::ActiveScope() {
319320
ASSERT(!detail::g_active);
320321
ASSERT(detail::g_allocatedSet.empty());
321322
ASSERT(detail::g_freedSet.empty());
323+
ASSERT(detail::g_wipedSet.empty());
322324
detail::g_active = true;
325+
// As of writing, TraceEvent uses eventname-based throttling keyed by Standalone<StringRef>,
326+
// which uses Arena and stays allocated after scope.
327+
// Therefore, we disable allocation tracing (e.g. hugeArenaSample()) while this scope is active.
328+
g_allocation_tracing_disabled++;
323329
}
324330

325331
ActiveScope::~ActiveScope() {
326332
ASSERT_ABORT(detail::g_active);
327333
ASSERT_ABORT(detail::g_allocatedSet == detail::g_freedSet);
334+
g_allocation_tracing_disabled--;
328335
for (auto memory : detail::g_allocatedSet) {
329-
delete[] reinterpret_cast<uint8_t*>(memory);
336+
delete[] static_cast<uint8_t*>(memory);
330337
}
331338
detail::g_allocatedSet.clear();
332339
detail::g_freedSet.clear();
340+
detail::g_wipedSet.clear();
333341
detail::g_active = false;
334342
}
335343

336344
void* allocate(size_t size) {
345+
ASSERT_ABORT(detail::g_active);
337346
auto ptr = new uint8_t[size];
338347
auto [_, inserted] = detail::g_allocatedSet.insert(ptr);
339-
ASSERT(inserted); // no duplicates
348+
ASSERT_ABORT(inserted); // no duplicates
340349
return ptr;
341350
}
342351

343352
void invalidate(void* ptr) {
344-
ASSERT(detail::g_allocatedSet.contains(ptr));
345-
ASSERT(!detail::g_freedSet.contains(ptr));
353+
ASSERT_ABORT(detail::g_active);
354+
ASSERT_ABORT(detail::g_allocatedSet.contains(ptr));
355+
ASSERT_ABORT(!detail::g_freedSet.contains(ptr));
346356
detail::g_freedSet.insert(ptr);
347357
}
348358

359+
void trackWipedArea(const uint8_t* begin, int size) {
360+
ASSERT_ABORT(detail::g_active);
361+
detail::g_wipedSet.emplace_back(begin, size);
362+
}
363+
364+
std::vector<std::pair<const uint8_t*, int>> const& getWipedAreaSet() {
365+
ASSERT_ABORT(detail::g_active);
366+
return detail::g_wipedSet;
367+
}
368+
349369
} // namespace keepalive_allocator
350370

351371
template <int Size>

‎flow/Knobs.cpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,7 @@ void FlowKnobs::initialize(Randomize randomize, IsSimulated isSimulated) {
145145
init( PUBLIC_KEY_FILE_REFRESH_INTERVAL_SECONDS, 300 );
146146
init( AUDIT_TIME_WINDOW, 5.0 );
147147
init( TOKEN_CACHE_SIZE, 2000 );
148+
init( WIPE_SENSITIVE_DATA_FROM_PACKET_BUFFER, true );
148149

149150
//AsyncFileCached
150151
init( PAGE_CACHE_4K, 2LL<<30 );

0 commit comments

Comments
 (0)