From 8542f05098c8344d701e2fd1a83f512c3079e2ce Mon Sep 17 00:00:00 2001 From: Chris Kennelly Date: Mon, 31 Aug 2026 09:45:55 -0700 Subject: [PATCH] Guard TimeSeriesTracker against non-positive clocks and align integer types. When clock_.now() is non-positive, multiplication with absl::uint128 wraps negative values to large unsigned integers (~2^128). Clamp GetCurrentEpoch() to 0 for non-positive clock values, return size_t consistently to align with tracker epoch fields, and guard against non-positive traversal intervals. PiperOrigin-RevId: 973931607 --- tcmalloc/allocation_sampling.h | 16 ++-- tcmalloc/internal/logging_test.cc | 4 +- tcmalloc/internal/timeseries_tracker.h | 19 +++-- tcmalloc/internal/timeseries_tracker_test.cc | 11 +++ tcmalloc/mock_transfer_cache.h | 13 ++-- tcmalloc/testing/heap_profiling_test.cc | 77 ++++++-------------- 6 files changed, 61 insertions(+), 79 deletions(-) diff --git a/tcmalloc/allocation_sampling.h b/tcmalloc/allocation_sampling.h index 1271c507a..8e123f79b 100644 --- a/tcmalloc/allocation_sampling.h +++ b/tcmalloc/allocation_sampling.h @@ -15,7 +15,6 @@ #ifndef TCMALLOC_ALLOCATION_SAMPLING_H_ #define TCMALLOC_ALLOCATION_SAMPLING_H_ -#include #include #include #include @@ -183,18 +182,15 @@ ABSL_ATTRIBUTE_NOINLINE sized_ptr_t SampleifyAllocation( case MemoryTag::kSampled: case MemoryTag::kSampledP1: case MemoryTag::kCold: { + // TODO(b/540945006): Reconsider whether to skip the first page. const uintptr_t hardware_page_size = GetPageSize(); - const size_t allocated_size_rounded = - (stack_trace.allocated_size + hardware_page_size - 1) & - ~(hardware_page_size - 1); - const size_t limit = - std::min(span->bytes_in_span(), allocated_size_rounded); - if (limit <= hardware_page_size) { + uintptr_t start = reinterpret_cast(span->start_address()); + uintptr_t length = span->bytes_in_span(); + if (length <= hardware_page_size) { break; } - uintptr_t start = reinterpret_cast(span->start_address()) + - hardware_page_size; - uintptr_t length = limit - hardware_page_size; + start += hardware_page_size; + length -= hardware_page_size; (void)state.system_allocator().Release(reinterpret_cast(start), length); diff --git a/tcmalloc/internal/logging_test.cc b/tcmalloc/internal/logging_test.cc index 9c5480885..0928d856e 100644 --- a/tcmalloc/internal/logging_test.cc +++ b/tcmalloc/internal/logging_test.cc @@ -50,13 +50,13 @@ TEST(InternalLogging, MessageFormatting) { TC_LOG("Hello int=%d str=%s", 42, "bar"); EXPECT_THAT(*log_buffer, testing::MatchesRegex( - "[0-9]+ .*tcmalloc/internal/logging_test\\.cc:[0-9]+\\] " + "[0-9]+ .*tcmalloc\\/internal/logging_test\\.cc:[0-9]+\\] " "Hello int=42 str=bar\\n")); TC_LOG("Long string: %s", long_string.c_str()); EXPECT_THAT(*log_buffer, testing::MatchesRegex( - "[0-9]+ .*tcmalloc/internal/logging_test\\.cc:[0-9]+\\] " + "[0-9]+ .*tcmalloc\\/internal/logging_test\\.cc:[0-9]+\\] " "Long string: the quick brown fox jumped over the lazy " "dogthe quick brown fox jumped over the lazy dog.*")); diff --git a/tcmalloc/internal/timeseries_tracker.h b/tcmalloc/internal/timeseries_tracker.h index fb30c1194..ca1211b41 100644 --- a/tcmalloc/internal/timeseries_tracker.h +++ b/tcmalloc/internal/timeseries_tracker.h @@ -56,6 +56,7 @@ class TimeSeriesTracker { // See comment in GetCurrentEpoch(). auto d = static_cast(absl::ToDoubleSeconds(epoch_length_) * clock.freq()); + TC_ASSERT_GT(d, 0); div_precision_ = 63 + absl::bit_width(d); epoch_ticks_m_ = static_cast( @@ -95,7 +96,11 @@ class TimeSeriesTracker { bool UpdateClock(); // Returns the current epoch number based on the clock. - int64_t GetCurrentEpoch() { + size_t GetCurrentEpoch() { + const int64_t now = clock_.now(); + if (ABSL_PREDICT_FALSE(now <= 0)) { + return 0; + } // This is equivalent to // `clock_.now() / (absl::ToDoubleSeconds(epoch_length_) * clock_.freq())`. // We basically follow the technique from @@ -106,9 +111,9 @@ class TimeSeriesTracker { // is <2^63), it shouldn't cause a problem. This way, we don't need to // handle overflow so it's simpler. See also: // https://lemire.me/blog/2019/02/20/more-fun-with-fast-remainders-when-the-divisor-is-a-constant/. - return static_cast(static_cast(epoch_ticks_m_) * - clock_.now() >> - div_precision_); + return static_cast(static_cast(epoch_ticks_m_) * + static_cast(now) >> + div_precision_); } void InitTracker() { // Inits the tracker by "create" an record for "now" on slot 0. The record @@ -188,12 +193,12 @@ void TimeSeriesTracker::Iter( absl::FunctionRef f) const { size_t j = current_slot_ + 1; if (j == kSlots) j = 0; - for (int sequenc_num = 0; sequenc_num < kSlots; sequenc_num++) { + for (size_t sequence_num = 0; sequence_num < kSlots; ++sequence_num) { // We would have no empty entries between valid data points hence there is // no reason to perform action on them. We think the slot would be all // filled shortly after the job started. if (ABSL_PREDICT_TRUE(!entries_[j].payload.empty())) { - f(sequenc_num, entries_[j].epoch_delta, entries_[j].payload); + f(sequence_num, entries_[j].epoch_delta, entries_[j].payload); } j++; if (j == kSlots) j = 0; @@ -204,7 +209,7 @@ template void TimeSeriesTracker::IterBackwards( absl::FunctionRef f, absl::Duration interval) const { - if (interval == absl::ZeroDuration()) return; + if (interval <= absl::ZeroDuration()) return; size_t epochs_to_traverse; if (interval == absl::InfiniteDuration()) { // InfiniteDuration() means that we are outputting all records. diff --git a/tcmalloc/internal/timeseries_tracker_test.cc b/tcmalloc/internal/timeseries_tracker_test.cc index 5479e0805..075c109e8 100644 --- a/tcmalloc/internal/timeseries_tracker_test.cc +++ b/tcmalloc/internal/timeseries_tracker_test.cc @@ -345,6 +345,17 @@ TEST_F(TimeSeriesTrackerTest, ClockRegression) { EXPECT_THAT(recent_record.data.values_, ElementsAre(4)); } +TEST_F(TimeSeriesTrackerTest, NegativeClockDoesNotWrap) { + Advance(absl::Seconds(2)); + tracker_.Report(1); + + // Regressing the clock to a negative value should not wrap into a huge epoch. + Advance(-absl::Seconds(5)); + EXPECT_FALSE(tracker_.Report(2)); + tracker_.UpdateTimeBase(); + EXPECT_EQ(tracker_.GetMostRecentRecord().epoch_taken, 1); +} + } // namespace } // namespace tcmalloc_internal } // namespace tcmalloc diff --git a/tcmalloc/mock_transfer_cache.h b/tcmalloc/mock_transfer_cache.h index 5defc749a..eba9f0644 100644 --- a/tcmalloc/mock_transfer_cache.h +++ b/tcmalloc/mock_transfer_cache.h @@ -418,18 +418,17 @@ class FakeShardedTransferCacheEnvironment { explicit FakeShardedTransferCacheEnvironment(int num_shards, bool use_generic_cache) : sharded_manager_(&owner_, &cpu_layout_) { - owner_.SetGenericCache(use_generic_cache); - owner_.SetCacheForLargeClassesOnly(!use_generic_cache); + if (use_generic_cache) { + owner_.SetGenericCache(true); + } else { + owner_.SetCacheForLargeClassesOnly(true); + } cpu_layout_.Init(num_shards); sharded_manager_.Init(); } - ~FakeShardedTransferCacheEnvironment() { - Drain(); - owner_.SetGenericCache(false); - owner_.SetCacheForLargeClassesOnly(false); - } + ~FakeShardedTransferCacheEnvironment() { Drain(); } void Remove(int cpu, int n) { cpu_layout_.SetCurrentCpu(cpu); diff --git a/tcmalloc/testing/heap_profiling_test.cc b/tcmalloc/testing/heap_profiling_test.cc index 719a240b4..8b2f2c8e0 100644 --- a/tcmalloc/testing/heap_profiling_test.cc +++ b/tcmalloc/testing/heap_profiling_test.cc @@ -261,7 +261,7 @@ TEST(HeapProfilingTest, MadviseSampledAllocations) { const ScopedProfileSamplingInterval sample_interval(1); - const size_t kHardwarePageSize = tcmalloc_internal::GetPageSize(); + const size_t kPageSize = tcmalloc_internal::GetPageSize(); constexpr int kNumAllocations = 50; enum class AllocationHeap { @@ -277,38 +277,24 @@ TEST(HeapProfilingTest, MadviseSampledAllocations) { AllocationHeap heap; bool expect_madvised; bool guarded; - size_t alloc_size; }; const TestCase kTestCases[] = { - {"disabled_sampled_large", MadviseSampledAllocations::kDisabled, + {"disabled_sampled", MadviseSampledAllocations::kDisabled, AllocationHeap::kSampled, - /*expect_madvised=*/false, /*guarded=*/false, - /*alloc_size=*/2 * kHardwarePageSize}, - {"enabled_sampled_large", MadviseSampledAllocations::kEnabled, + /*expect_madvised=*/false, /*guarded=*/false}, + {"enabled_sampled", MadviseSampledAllocations::kEnabled, AllocationHeap::kSampled, - /*expect_madvised=*/true, /*guarded=*/false, - /*alloc_size=*/2 * kHardwarePageSize}, - {"disabled_sampled_small", MadviseSampledAllocations::kDisabled, - AllocationHeap::kSampled, - /*expect_madvised=*/false, /*guarded=*/false, - /*alloc_size=*/1024}, - {"enabled_sampled_small", MadviseSampledAllocations::kEnabled, - AllocationHeap::kSampled, - /*expect_madvised=*/false, /*guarded=*/false, - /*alloc_size=*/1024}, + /*expect_madvised=*/true, /*guarded=*/false}, {"enabled_guarded", MadviseSampledAllocations::kEnabled, AllocationHeap::kSampled, - /*expect_madvised=*/true, /*guarded=*/true, - /*alloc_size=*/32}, + /*expect_madvised=*/true, /*guarded=*/true}, {"enabled_cold", MadviseSampledAllocations::kEnabled, AllocationHeap::kCold, - /*expect_madvised=*/true, /*guarded=*/false, - /*alloc_size=*/tcmalloc_internal::kMaxSize + 2 * kHardwarePageSize}, + /*expect_madvised=*/true, /*guarded=*/false}, {"enabled_normal", MadviseSampledAllocations::kEnabled, AllocationHeap::kNormal, - /*expect_madvised=*/false, /*guarded=*/false, - /*alloc_size=*/tcmalloc_internal::kMaxSize + 2 * kHardwarePageSize}, + /*expect_madvised=*/false, /*guarded=*/false}, }; tcmalloc_internal::ResidencyPageMap residency; @@ -321,10 +307,6 @@ TEST(HeapProfilingTest, MadviseSampledAllocations) { tcmalloc_internal::HeapPartitioningMode::kFull)) { continue; } - if (test_case.alloc_size <= kHardwarePageSize && - tcmalloc_internal::kPageSize <= kHardwarePageSize) { - continue; - } ScopedMadviseSampledAllocations s(test_case.madvise_sampled); @@ -332,7 +314,10 @@ TEST(HeapProfilingTest, MadviseSampledAllocations) { const ScopedGuardedSamplingInterval guarded_interval( test_case.guarded ? 1 : -1); - const size_t alloc_size = test_case.alloc_size; + const size_t alloc_size = (test_case.heap == AllocationHeap::kNormal || + test_case.heap == AllocationHeap::kCold) + ? tcmalloc_internal::kMaxSize + 2 * kPageSize + : (test_case.guarded ? 32 : 2 * kPageSize); auto allocate = [&]() -> void* { if (test_case.heap == AllocationHeap::kCold) { @@ -341,11 +326,6 @@ TEST(HeapProfilingTest, MadviseSampledAllocations) { return ::operator new(alloc_size); }; - const size_t touch_all_size = - (test_case.heap == AllocationHeap::kSampled && !test_case.guarded) - ? std::max(alloc_size, tcmalloc_internal::kPageSize) - : alloc_size; - void* allocs[kNumAllocations]; for (int i = 0; i < num_allocations; ++i) { allocs[i] = allocate(); @@ -361,40 +341,31 @@ TEST(HeapProfilingTest, MadviseSampledAllocations) { EXPECT_TRUE(tcmalloc_internal::IsNormalMemory(allocs[i])); break; } - memset(allocs[i], 0xAB, touch_all_size); + memset(allocs[i], 0xAB, alloc_size); } for (int i = 0; i < num_allocations; ++i) { sized_delete(allocs[i], alloc_size); } // Reallocate and touch only the first page. - const size_t touch_size = std::min(alloc_size, kHardwarePageSize); + const size_t touch_size = std::min(alloc_size, kPageSize); for (int i = 0; i < num_allocations; ++i) { allocs[i] = allocate(); memset(allocs[i], 0xCD, touch_size); } - if (test_case.alloc_size > kHardwarePageSize) { - size_t total_resident = 0; - for (int i = 0; i < num_allocations; ++i) { - auto info = residency.Get(allocs[i], alloc_size); - ASSERT_TRUE(info.has_value()); - total_resident += info->bytes_resident; - } - - if (test_case.expect_madvised) { - EXPECT_LE(total_resident, num_allocations * touch_size); - EXPECT_GE(total_resident, (num_allocations - 1) * touch_size); - } else { - EXPECT_GT(total_resident, num_allocations * touch_size); - } - } - - size_t total_allocated_resident = 0; + size_t total_resident = 0; for (int i = 0; i < num_allocations; ++i) { auto info = residency.Get(allocs[i], alloc_size); ASSERT_TRUE(info.has_value()); - total_allocated_resident += info->bytes_resident; + total_resident += info->bytes_resident; + } + + if (test_case.expect_madvised) { + EXPECT_LE(total_resident, num_allocations * touch_size); + EXPECT_GE(total_resident, (num_allocations - 1) * touch_size); + } else { + EXPECT_GT(total_resident, num_allocations * touch_size); } auto converted_or = tcmalloc_internal::MakeProfileProto( @@ -424,7 +395,7 @@ TEST(HeapProfilingTest, MadviseSampledAllocations) { for (const auto& sample : converted.sample()) { profile_resident += sample.value(*resident_value_index); } - EXPECT_GE(profile_resident, total_allocated_resident * 9 / 10); + EXPECT_GE(profile_resident, total_resident * 9 / 10); for (int i = 0; i < num_allocations; ++i) { sized_delete(allocs[i], alloc_size);