Skip to content

Commit f3433e6

Browse files
committed
Fix user-after-move in TrackInternal.
1 parent 342550a commit f3433e6

File tree

2 files changed

+57
-5
lines changed

2 files changed

+57
-5
lines changed

libs/server-sdk/src/client_impl.cpp

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -239,17 +239,27 @@ void ClientImpl::TrackInternal(Context const& ctx,
239239
std::optional<Value> data,
240240
std::optional<double> metric_value,
241241
hooks::HookContext const& hook_context) {
242-
events_default_.Send([&](EventFactory const& factory) {
243-
return factory.Custom(ctx, std::move(event_name), std::move(data),
244-
metric_value);
245-
});
246242

247-
// Execute afterTrack hooks
243+
if (!ctx.Valid()) {
244+
LD_LOG(logger_, LogLevel::kWarn) << "Track method called with an invalid context";
245+
return;
246+
}
247+
// Execute afterTrack hooks before moving the data
248+
// Typically we would execute this after the data has been enqueued.
249+
// In this SDK doing so would introduce a performance penalty because we
250+
// would need to copy the event_name and data.
251+
// In this SDK the data is type-safe, and will be enqueued, so it makes
252+
// minimal functional difference.
248253
if (!config_.Hooks().empty()) {
249254
hooks::TrackSeriesContext series_context(ctx, event_name, metric_value,
250255
data, hook_context, std::nullopt);
251256
hooks::ExecuteAfterTrack(config_.Hooks(), series_context, logger_);
252257
}
258+
259+
events_default_.Send([&](EventFactory const& factory) {
260+
return factory.Custom(ctx, std::move(event_name), std::move(data),
261+
metric_value);
262+
});
253263
}
254264

255265
void ClientImpl::Track(Context const& ctx,

libs/server-sdk/tests/hooks_test.cpp

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ class RecordingHook : public Hook {
3131
struct TrackCall {
3232
std::string key;
3333
std::optional<double> metric_value;
34+
std::optional<Value> data;
3435
};
3536

3637
explicit RecordingHook(std::string name) : metadata_(std::move(name)) {}
@@ -77,6 +78,7 @@ class RecordingHook : public Hook {
7778
TrackCall call;
7879
call.key = std::string(series_context.Key());
7980
call.metric_value = series_context.MetricValue();
81+
call.data = series_context.Data();
8082
track_calls_.push_back(call);
8183
}
8284

@@ -443,6 +445,46 @@ TEST_F(HooksTest, AfterTrackWithMetricValue) {
443445
EXPECT_EQ(*calls[0].metric_value, 42.5);
444446
}
445447

448+
// Test that data is properly passed to afterTrack hooks
449+
// This test would catch use-after-move bugs with ASAN
450+
TEST_F(HooksTest, AfterTrackReceivesData) {
451+
auto hook = std::make_shared<RecordingHook>("TestHook");
452+
453+
auto config = ConfigBuilder("sdk-key")
454+
.Offline(true)
455+
.Hooks(hook)
456+
.Build()
457+
.value();
458+
459+
Client client(std::move(config));
460+
461+
// Test with string data
462+
client.Track(context_, "test-event", Value("test-data"));
463+
464+
auto const& calls = hook->GetTrackCalls();
465+
ASSERT_EQ(calls.size(), 1);
466+
EXPECT_EQ(calls[0].key, "test-event");
467+
ASSERT_TRUE(calls[0].data.has_value());
468+
EXPECT_EQ(calls[0].data->AsString(), "test-data");
469+
470+
hook->Reset();
471+
472+
// Test with complex data
473+
auto complex_data = Value::Object({{"field1", Value("value1")}, {"field2", Value(42)}});
474+
client.Track(context_, "test-event-2", complex_data, 99.5);
475+
476+
auto const& calls2 = hook->GetTrackCalls();
477+
ASSERT_EQ(calls2.size(), 1);
478+
EXPECT_EQ(calls2[0].key, "test-event-2");
479+
ASSERT_TRUE(calls2[0].data.has_value());
480+
EXPECT_EQ(calls2[0].data->Type(), Value::Type::kObject);
481+
auto const& obj = calls2[0].data->AsObject();
482+
EXPECT_EQ(obj["field1"].AsString(), "value1");
483+
EXPECT_EQ(obj["field2"].AsInt(), 42);
484+
ASSERT_TRUE(calls2[0].metric_value.has_value());
485+
EXPECT_EQ(*calls2[0].metric_value, 99.5);
486+
}
487+
446488
// Requirement 1.3.4: afterTrack handlers execute in order of registration
447489
TEST_F(HooksTest, AfterTrackExecutesInOrder) {
448490
auto execution_order = std::make_shared<std::vector<std::string>>();

0 commit comments

Comments
 (0)