From fbb73b8b234520b75fe29ec9c692f635cbe35ff1 Mon Sep 17 00:00:00 2001 From: Primiano Tucci Date: Fri, 24 Mar 2023 13:21:33 +0000 Subject: traced: bail out if an invalid trigger mode is specified This is a fix to a subtle binary back-compat issue. When introducing a new trigger mode, the service will ignore it and trace indefinitely (Because duration_ms is checked to be 0). The issue is not as bad as initially though because perfetto_cmd mitigates this, by enforcing a hard timeout to 60s + trigger_timeout regardless of the type of the trigger (See comments in b/274931668). Bug: 274931668 Bug: 260112703 Test: perfetto_unittests --gtest_filter=TracingServiceImplTest.FailOnUnknownTriggerMode (cherry picked from commit 7f432673ea568e8ab9b7f4ccfe0107a4c5e01e81) Change-Id: I7993ec7a3604b8ede00b3c29cf29059685af4848 Merged-In: I7993ec7a3604b8ede00b3c29cf29059685af4848 --- src/android_stats/perfetto_atoms.h | 1 + src/tracing/core/tracing_service_impl.cc | 12 +++++++++++ src/tracing/core/tracing_service_impl_unittest.cc | 25 +++++++++++++++++++++++ 3 files changed, 38 insertions(+) diff --git a/src/android_stats/perfetto_atoms.h b/src/android_stats/perfetto_atoms.h index 75cbbd837..14740ae80 100644 --- a/src/android_stats/perfetto_atoms.h +++ b/src/android_stats/perfetto_atoms.h @@ -70,6 +70,7 @@ enum class PerfettoStatsdAtom { kTracedStartTracingInvalidSessionState = 36, kTracedEnableTracingInvalidFilter = 47, kTracedEnableTracingOobTargetBuffer = 48, + kTracedEnableTracingInvalidTriggerMode = 52, // Checkpoints inside perfetto_cmd after tracing has finished. kOnTracingDisabled = 4, diff --git a/src/tracing/core/tracing_service_impl.cc b/src/tracing/core/tracing_service_impl.cc index b35345118..93419734e 100644 --- a/src/tracing/core/tracing_service_impl.cc +++ b/src/tracing/core/tracing_service_impl.cc @@ -613,6 +613,15 @@ base::Status TracingServiceImpl::EnableTracing(ConsumerEndpointImpl* consumer, cfg.trigger_config().trigger_timeout_ms()); } + // This check has been introduced in May 2023 after finding b/274931668. + if (static_cast(cfg.trigger_config().trigger_mode()) > + TraceConfig::TriggerConfig::TriggerMode_MAX) { + MaybeLogUploadEvent( + cfg, PerfettoStatsdAtom::kTracedEnableTracingInvalidTriggerMode); + return PERFETTO_SVC_ERR( + "The trace config specified an invalid trigger_mode"); + } + if (has_trigger_config && cfg.duration_ms() != 0) { MaybeLogUploadEvent( cfg, PerfettoStatsdAtom::kTracedEnableTracingDurationWithTrigger); @@ -944,6 +953,9 @@ base::Status TracingServiceImpl::EnableTracing(ConsumerEndpointImpl* consumer, tracing_session->config.set_duration_ms( cfg.trigger_config().trigger_timeout_ms()); break; + + // The case of unknown modes (coming from future versions of the service) + // is handled few lines above (search for TriggerMode_MAX). } tracing_session->state = TracingSession::CONFIGURED; diff --git a/src/tracing/core/tracing_service_impl_unittest.cc b/src/tracing/core/tracing_service_impl_unittest.cc index 00b7a3a16..bce99e7ff 100644 --- a/src/tracing/core/tracing_service_impl_unittest.cc +++ b/src/tracing/core/tracing_service_impl_unittest.cc @@ -425,6 +425,31 @@ TEST_F(TracingServiceImplTest, StartTracingTriggerTimeOut) { EXPECT_THAT(consumer->ReadBuffers(), IsEmpty()); } +// Regression test for b/274931668. An unkonwn trigger should not cause a trace +// that runs indefinitely. +TEST_F(TracingServiceImplTest, FailOnUnknownTrigger) { + std::unique_ptr consumer = CreateMockConsumer(); + consumer->Connect(svc.get()); + + std::unique_ptr producer = CreateMockProducer(); + producer->Connect(svc.get(), "mock_producer"); + producer->RegisterDataSource("ds_1"); + + TraceConfig trace_config; + trace_config.add_buffers()->set_size_kb(128); + trace_config.add_data_sources()->mutable_config()->set_name("ds_1"); + auto* trigger_config = trace_config.mutable_trigger_config(); + trigger_config->set_trigger_mode( + static_cast( + TraceConfig::TriggerConfig::TriggerMode_MAX + 1)); + auto* trigger = trigger_config->add_triggers(); + trigger->set_name("trigger_from_the_future"); + trigger_config->set_trigger_timeout_ms(1); + + consumer->EnableTracing(trace_config); + consumer->WaitForTracingDisabled(); +} + // Creates a tracing session with a START_TRACING trigger and checks that // the session is not started when the configured trigger producer is different // than the producer that sent the trigger. -- cgit v1.2.3 From 4b981b263d26b8733eae94d333fe0eb4d26406c8 Mon Sep 17 00:00:00 2001 From: Hector Dearman Date: Wed, 3 Aug 2022 15:13:08 +0100 Subject: perfetto_cmd: --ignore-guardrails effects service Allow --ignore-guardrails to override enable_extra_guardrails (and hence the traced side guardrails). (cherry-pick of 3c678cb3e4484c028bdb354bcbee872ebdc1ae91 to fix a CTS test failure on tm-qpr-dev caused by fixing b/268320325) Bug: 230096817 Merged-In: I60a280cfa8b4d5ad26f28cc023b408d87d3a5190 Change-Id: I60a280cfa8b4d5ad26f28cc023b408d87d3a5190 --- src/perfetto_cmd/perfetto_cmd.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/perfetto_cmd/perfetto_cmd.cc b/src/perfetto_cmd/perfetto_cmd.cc index 8c84802e4..aafc8445d 100644 --- a/src/perfetto_cmd/perfetto_cmd.cc +++ b/src/perfetto_cmd/perfetto_cmd.cc @@ -1002,8 +1002,9 @@ void PerfettoCmd::OnConnect() { } PERFETTO_DCHECK(trace_config_); - trace_config_->set_enable_extra_guardrails(save_to_incidentd_ || - report_to_android_framework_); + trace_config_->set_enable_extra_guardrails( + (save_to_incidentd_ || report_to_android_framework_) && + !ignore_guardrails_); // Set the statsd logging flag if we're uploading -- cgit v1.2.3