diff options
-rw-r--r-- | src/android_stats/perfetto_atoms.h | 1 | ||||
-rw-r--r-- | src/perfetto_cmd/perfetto_cmd.cc | 5 | ||||
-rw-r--r-- | src/tracing/core/tracing_service_impl.cc | 12 | ||||
-rw-r--r-- | src/tracing/core/tracing_service_impl_unittest.cc | 25 |
4 files changed, 41 insertions, 2 deletions
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/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 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<int>(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<MockConsumer> consumer = CreateMockConsumer(); + consumer->Connect(svc.get()); + + std::unique_ptr<MockProducer> 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>( + 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. |