Skip to content

Commit d2e7289

Browse files
lalitblalitb
authored
[SDK] Use shared_ptr internally for AttributesProcessor to prevent use-after-free (#3457)
* fix * fix * add test * fix * iwuc
1 parent 05ac106 commit d2e7289

File tree

9 files changed

+94
-30
lines changed

9 files changed

+94
-30
lines changed

‎sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage
5858
public:
5959
SyncMetricStorage(InstrumentDescriptor instrument_descriptor,
6060
const AggregationType aggregation_type,
61-
const AttributesProcessor *attributes_processor,
61+
std::shared_ptr<const AttributesProcessor> attributes_processor,
6262
#ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW
6363
ExemplarFilterType exempler_filter_type,
6464
nostd::shared_ptr<ExemplarReservoir> &&exemplar_reservoir,
@@ -67,7 +67,7 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage
6767
size_t attributes_limit = kAggregationCardinalityLimit)
6868
: instrument_descriptor_(instrument_descriptor),
6969
attributes_hashmap_(new AttributesHashMap(attributes_limit)),
70-
attributes_processor_(attributes_processor),
70+
attributes_processor_(std::move(attributes_processor)),
7171
#ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW
7272
exemplar_filter_type_(exempler_filter_type),
7373
exemplar_reservoir_(exemplar_reservoir),
@@ -119,7 +119,7 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage
119119

120120
std::lock_guard<opentelemetry::common::SpinLockMutex> guard(attribute_hashmap_lock_);
121121
attributes_hashmap_
122-
->GetOrSetDefault(attributes, attributes_processor_, create_default_aggregation_)
122+
->GetOrSetDefault(attributes, attributes_processor_.get(), create_default_aggregation_)
123123
->Aggregate(value);
124124
}
125125

@@ -160,7 +160,7 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage
160160
#endif
161161
std::lock_guard<opentelemetry::common::SpinLockMutex> guard(attribute_hashmap_lock_);
162162
attributes_hashmap_
163-
->GetOrSetDefault(attributes, attributes_processor_, create_default_aggregation_)
163+
->GetOrSetDefault(attributes, attributes_processor_.get(), create_default_aggregation_)
164164
->Aggregate(value);
165165
}
166166

@@ -175,7 +175,7 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage
175175
// hashmap to maintain the metrics for delta collection (i.e, collection since last Collect call)
176176
std::unique_ptr<AttributesHashMap> attributes_hashmap_;
177177
std::function<std::unique_ptr<Aggregation>()> create_default_aggregation_;
178-
const AttributesProcessor *attributes_processor_;
178+
std::shared_ptr<const AttributesProcessor> attributes_processor_;
179179
#ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW
180180
ExemplarFilterType exemplar_filter_type_;
181181
nostd::shared_ptr<ExemplarReservoir> exemplar_reservoir_;

‎sdk/include/opentelemetry/sdk/metrics/view/view.h

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -53,10 +53,10 @@ class View
5353
return aggregation_config_.get();
5454
}
5555

56-
virtual const opentelemetry::sdk::metrics::AttributesProcessor &GetAttributesProcessor()
57-
const noexcept
56+
virtual std::shared_ptr<const opentelemetry::sdk::metrics::AttributesProcessor>
57+
GetAttributesProcessor() const noexcept
5858
{
59-
return *attributes_processor_.get();
59+
return attributes_processor_;
6060
}
6161

6262
private:
@@ -65,7 +65,7 @@ class View
6565
std::string unit_;
6666
AggregationType aggregation_type_;
6767
std::shared_ptr<AggregationConfig> aggregation_config_;
68-
std::unique_ptr<opentelemetry::sdk::metrics::AttributesProcessor> attributes_processor_;
68+
std::shared_ptr<AttributesProcessor> attributes_processor_;
6969
};
7070
} // namespace metrics
7171
} // namespace sdk

‎sdk/src/metrics/meter.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -538,7 +538,7 @@ std::unique_ptr<SyncWritableMetricStorage> Meter::RegisterSyncMetricStorage(
538538
{
539539
WarnOnDuplicateInstrument(GetInstrumentationScope(), storage_registry_, view_instr_desc);
540540
sync_storage = std::shared_ptr<SyncMetricStorage>(new SyncMetricStorage(
541-
view_instr_desc, view.GetAggregationType(), &view.GetAttributesProcessor(),
541+
view_instr_desc, view.GetAggregationType(), view.GetAttributesProcessor(),
542542
#ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW
543543
exemplar_filter_type,
544544
GetExemplarReservoir(view.GetAggregationType(), view.GetAggregationConfig(),

‎sdk/test/metrics/cardinality_limit_test.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -109,9 +109,9 @@ TEST_P(WritableMetricStorageCardinalityLimitTestFixture, LongCounterSumAggregati
109109
const size_t attributes_limit = 10;
110110
InstrumentDescriptor instr_desc = {"name", "desc", "1unit", InstrumentType::kCounter,
111111
InstrumentValueType::kLong};
112-
std::unique_ptr<DefaultAttributesProcessor> default_attributes_processor{
112+
std::shared_ptr<DefaultAttributesProcessor> default_attributes_processor{
113113
new DefaultAttributesProcessor{}};
114-
SyncMetricStorage storage(instr_desc, AggregationType::kSum, default_attributes_processor.get(),
114+
SyncMetricStorage storage(instr_desc, AggregationType::kSum, default_attributes_processor,
115115
#ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW
116116
ExemplarFilterType::kAlwaysOff,
117117
ExemplarReservoir::GetNoExemplarReservoir(),

‎sdk/test/metrics/meter_test.cc

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,21 +6,25 @@
66
#include <stdint.h>
77
#include <algorithm>
88
#include <atomic>
9+
#include <chrono>
910
#include <iostream>
1011
#include <string>
1112
#include <thread>
13+
#include <type_traits>
1214
#include <utility>
1315
#include <vector>
1416
#include "common.h"
1517

1618
#include <functional>
19+
#include "opentelemetry/common/key_value_iterable.h"
1720
#include "opentelemetry/context/context.h"
1821
#include "opentelemetry/metrics/async_instruments.h"
1922
#include "opentelemetry/metrics/meter.h"
2023
#include "opentelemetry/sdk/instrumentationscope/instrumentation_scope.h"
2124
#include "opentelemetry/sdk/instrumentationscope/scope_configurator.h"
2225
#include "opentelemetry/sdk/metrics/instruments.h"
2326
#include "opentelemetry/sdk/metrics/meter_config.h"
27+
#include "opentelemetry/sdk/metrics/view/attributes_processor.h"
2428
#include "opentelemetry/sdk/metrics/view/view_registry.h"
2529
#include "opentelemetry/sdk/resource/resource.h"
2630

@@ -31,6 +35,7 @@
3135
#include "opentelemetry/metrics/sync_instruments.h" // IWYU pragma: keep
3236
#include "opentelemetry/nostd/function_ref.h"
3337
#include "opentelemetry/nostd/shared_ptr.h"
38+
#include "opentelemetry/nostd/string_view.h"
3439
#include "opentelemetry/nostd/variant.h"
3540
#include "opentelemetry/sdk/common/attribute_utils.h"
3641
#include "opentelemetry/sdk/common/global_log_handler.h"
@@ -184,6 +189,22 @@ class MeterCreateInstrumentTest : public ::testing::Test
184189
std::shared_ptr<MetricReader> metric_reader_ptr_{new MockMetricReader()};
185190
};
186191

192+
class TestProcessor : public sdk::metrics::AttributesProcessor
193+
{
194+
public:
195+
explicit TestProcessor() = default;
196+
~TestProcessor() override = default;
197+
198+
sdk::metrics::MetricAttributes process(
199+
const opentelemetry::common::KeyValueIterable &attributes) const noexcept override
200+
{
201+
// Just forward attributes
202+
return sdk::metrics::MetricAttributes(attributes);
203+
}
204+
205+
bool isPresent(nostd::string_view /*key*/) const noexcept override { return true; }
206+
};
207+
187208
} // namespace
188209

189210
TEST(MeterTest, BasicAsyncTests)
@@ -851,3 +872,46 @@ TEST_F(MeterCreateInstrumentTest, ViewCorrectedDuplicateAsyncInstrumentsByDescri
851872
return true;
852873
});
853874
}
875+
876+
TEST(MeterTest, RecordAfterProviderDestructionWithCustomProcessor_NoResetInMain)
877+
{
878+
std::unique_ptr<AttributesProcessor> processor(new TestProcessor());
879+
880+
// MeterProvider is owned by unique_ptr for explicit control
881+
std::unique_ptr<MeterProvider> provider(new MeterProvider());
882+
883+
// Register a View with custom processor
884+
std::unique_ptr<View> view(
885+
new View("my_counter", "", "", AggregationType::kSum, nullptr, std::move(processor)));
886+
std::unique_ptr<InstrumentSelector> instr_selector(
887+
new InstrumentSelector(InstrumentType::kCounter, "my_counter", ""));
888+
std::unique_ptr<MeterSelector> meter_selector(new MeterSelector("test_meter", "", ""));
889+
provider->AddView(std::move(instr_selector), std::move(meter_selector), std::move(view));
890+
891+
auto meter = provider->GetMeter("test_meter");
892+
auto counter = meter->CreateUInt64Counter("my_counter");
893+
894+
// Move the counter to the thread
895+
std::atomic<bool> thread_ready{false};
896+
std::atomic<bool> thread_done{false};
897+
898+
std::thread t([c = std::move(counter), &thread_ready, &thread_done]() mutable {
899+
thread_ready = true;
900+
std::this_thread::sleep_for(std::chrono::milliseconds(50));
901+
// Safe after provider destruction
902+
c->Add(12345, {{"thread", "after_provider_destruction"}});
903+
thread_done = true;
904+
});
905+
906+
// Wait for thread to be ready
907+
while (!thread_ready.load())
908+
std::this_thread::yield();
909+
910+
// Destroy the provider (and its storage etc)
911+
provider.reset();
912+
913+
// Wait for thread to finish
914+
while (!thread_done.load())
915+
std::this_thread::yield();
916+
t.join();
917+
}

‎sdk/test/metrics/sync_metric_storage_counter_test.cc

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -49,10 +49,10 @@ TEST_P(WritableMetricStorageTestFixture, LongCounterSumAggregation)
4949
std::map<std::string, std::string> attributes_get = {{"RequestType", "GET"}};
5050
std::map<std::string, std::string> attributes_put = {{"RequestType", "PUT"}};
5151

52-
std::unique_ptr<DefaultAttributesProcessor> default_attributes_processor{
52+
std::shared_ptr<DefaultAttributesProcessor> default_attributes_processor{
5353
new DefaultAttributesProcessor{}};
5454
opentelemetry::sdk::metrics::SyncMetricStorage storage(
55-
instr_desc, AggregationType::kSum, default_attributes_processor.get(),
55+
instr_desc, AggregationType::kSum, default_attributes_processor,
5656
#ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW
5757
ExemplarFilterType::kAlwaysOff, ExemplarReservoir::GetNoExemplarReservoir(),
5858
#endif
@@ -189,10 +189,10 @@ TEST_P(WritableMetricStorageTestFixture, DoubleCounterSumAggregation)
189189
std::map<std::string, std::string> attributes_get = {{"RequestType", "GET"}};
190190
std::map<std::string, std::string> attributes_put = {{"RequestType", "PUT"}};
191191

192-
std::unique_ptr<DefaultAttributesProcessor> default_attributes_processor{
192+
std::shared_ptr<DefaultAttributesProcessor> default_attributes_processor{
193193
new DefaultAttributesProcessor{}};
194194
opentelemetry::sdk::metrics::SyncMetricStorage storage(
195-
instr_desc, AggregationType::kSum, default_attributes_processor.get(),
195+
instr_desc, AggregationType::kSum, default_attributes_processor,
196196
#ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW
197197
ExemplarFilterType::kAlwaysOff, ExemplarReservoir::GetNoExemplarReservoir(),
198198
#endif

‎sdk/test/metrics/sync_metric_storage_gauge_test.cc

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -37,10 +37,10 @@ TEST_P(WritableMetricStorageTestFixture, LongGaugeLastValueAggregation)
3737
std::map<std::string, std::string> attributes_roomA = {{"Room.id", "Rack A"}};
3838
std::map<std::string, std::string> attributes_roomB = {{"Room.id", "Rack B"}};
3939

40-
std::unique_ptr<DefaultAttributesProcessor> default_attributes_processor{
40+
std::shared_ptr<DefaultAttributesProcessor> default_attributes_processor{
4141
new DefaultAttributesProcessor{}};
4242
opentelemetry::sdk::metrics::SyncMetricStorage storage(
43-
instr_desc, AggregationType::kLastValue, default_attributes_processor.get(),
43+
instr_desc, AggregationType::kLastValue, default_attributes_processor,
4444
# ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW
4545
ExemplarFilterType::kAlwaysOff, ExemplarReservoir::GetNoExemplarReservoir(),
4646
# endif
@@ -121,10 +121,10 @@ TEST_P(WritableMetricStorageTestFixture, DoubleGaugeLastValueAggregation)
121121
std::map<std::string, std::string> attributes_roomA = {{"Room.id", "Rack A"}};
122122
std::map<std::string, std::string> attributes_roomB = {{"Room.id", "Rack B"}};
123123

124-
std::unique_ptr<DefaultAttributesProcessor> default_attributes_processor{
124+
std::shared_ptr<DefaultAttributesProcessor> default_attributes_processor{
125125
new DefaultAttributesProcessor{}};
126126
opentelemetry::sdk::metrics::SyncMetricStorage storage(
127-
instr_desc, AggregationType::kLastValue, default_attributes_processor.get(),
127+
instr_desc, AggregationType::kLastValue, default_attributes_processor,
128128
# ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW
129129
ExemplarFilterType::kAlwaysOff, ExemplarReservoir::GetNoExemplarReservoir(),
130130
# endif

‎sdk/test/metrics/sync_metric_storage_histogram_test.cc

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -51,10 +51,10 @@ TEST_P(WritableMetricStorageHistogramTestFixture, LongHistogram)
5151
std::map<std::string, std::string> attributes_get = {{"RequestType", "GET"}};
5252
std::map<std::string, std::string> attributes_put = {{"RequestType", "PUT"}};
5353

54-
std::unique_ptr<DefaultAttributesProcessor> default_attributes_processor{
54+
std::shared_ptr<DefaultAttributesProcessor> default_attributes_processor{
5555
new DefaultAttributesProcessor{}};
5656
opentelemetry::sdk::metrics::SyncMetricStorage storage(
57-
instr_desc, AggregationType::kHistogram, default_attributes_processor.get(),
57+
instr_desc, AggregationType::kHistogram, default_attributes_processor,
5858
#ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW
5959
ExemplarFilterType::kAlwaysOff, ExemplarReservoir::GetNoExemplarReservoir(),
6060
#endif
@@ -192,10 +192,10 @@ TEST_P(WritableMetricStorageHistogramTestFixture, DoubleHistogram)
192192
std::map<std::string, std::string> attributes_get = {{"RequestType", "GET"}};
193193
std::map<std::string, std::string> attributes_put = {{"RequestType", "PUT"}};
194194

195-
std::unique_ptr<DefaultAttributesProcessor> default_attributes_processor{
195+
std::shared_ptr<DefaultAttributesProcessor> default_attributes_processor{
196196
new DefaultAttributesProcessor{}};
197197
opentelemetry::sdk::metrics::SyncMetricStorage storage(
198-
instr_desc, AggregationType::kHistogram, default_attributes_processor.get(),
198+
instr_desc, AggregationType::kHistogram, default_attributes_processor,
199199
#ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW
200200
ExemplarFilterType::kAlwaysOff, ExemplarReservoir::GetNoExemplarReservoir(),
201201
#endif
@@ -340,10 +340,10 @@ TEST_P(WritableMetricStorageHistogramTestFixture, Base2ExponentialDoubleHistogra
340340
std::map<std::string, std::string> attributes_get = {{"RequestType", "GET"}};
341341
std::map<std::string, std::string> attributes_put = {{"RequestType", "PUT"}};
342342

343-
std::unique_ptr<DefaultAttributesProcessor> default_attributes_processor{
343+
std::shared_ptr<DefaultAttributesProcessor> default_attributes_processor{
344344
new DefaultAttributesProcessor{}};
345345
opentelemetry::sdk::metrics::SyncMetricStorage storage(
346-
instr_desc, AggregationType::kBase2ExponentialHistogram, default_attributes_processor.get(),
346+
instr_desc, AggregationType::kBase2ExponentialHistogram, default_attributes_processor,
347347
#ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW
348348
ExemplarFilterType::kAlwaysOff, ExemplarReservoir::GetNoExemplarReservoir(),
349349
#endif

‎sdk/test/metrics/sync_metric_storage_up_down_counter_test.cc

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -48,10 +48,10 @@ TEST_P(WritableMetricStorageTestFixture, LongUpDownCounterSumAggregation)
4848
std::map<std::string, std::string> attributes_get = {{"RequestType", "GET"}};
4949
std::map<std::string, std::string> attributes_put = {{"RequestType", "PUT"}};
5050

51-
std::unique_ptr<DefaultAttributesProcessor> default_attributes_processor{
51+
std::shared_ptr<DefaultAttributesProcessor> default_attributes_processor{
5252
new DefaultAttributesProcessor{}};
5353
opentelemetry::sdk::metrics::SyncMetricStorage storage(
54-
instr_desc, AggregationType::kSum, default_attributes_processor.get(),
54+
instr_desc, AggregationType::kSum, default_attributes_processor,
5555
#ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW
5656
ExemplarFilterType::kAlwaysOff, ExemplarReservoir::GetNoExemplarReservoir(),
5757
#endif
@@ -198,10 +198,10 @@ TEST_P(WritableMetricStorageTestFixture, DoubleUpDownCounterSumAggregation)
198198
std::map<std::string, std::string> attributes_get = {{"RequestType", "GET"}};
199199
std::map<std::string, std::string> attributes_put = {{"RequestType", "PUT"}};
200200

201-
std::unique_ptr<DefaultAttributesProcessor> default_attributes_processor{
201+
std::shared_ptr<DefaultAttributesProcessor> default_attributes_processor{
202202
new DefaultAttributesProcessor{}};
203203
opentelemetry::sdk::metrics::SyncMetricStorage storage(
204-
instr_desc, AggregationType::kSum, default_attributes_processor.get(),
204+
instr_desc, AggregationType::kSum, default_attributes_processor,
205205
#ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW
206206
ExemplarFilterType::kAlwaysOff, ExemplarReservoir::GetNoExemplarReservoir(),
207207
#endif

0 commit comments

Comments
 (0)