heap profilers: reject data sources with unparsable cmdlines in cfgs
For logical parity with aosp/1254587. Same argument - fail fast on
invalid configs, instead of trying to degrade (and deal with associated
edge cases).
I don't think this does anything in practice at the time of writing.
NormalizeCmdLine() should always succeed unless there's a bug in the
caller (only error case is not being null-terminated afaict).
Test: profiling by cmdline not obviously broken, tried:
tools/heap_profile -i 4096 --name system_server -d 10000
Change-Id: I1658fe9bc7c07b5210e6c219a6976f6e8e40246f
diff --git a/src/profiling/common/proc_utils.cc b/src/profiling/common/proc_utils.cc
index b65a956..d69826e 100644
--- a/src/profiling/common/proc_utils.cc
+++ b/src/profiling/common/proc_utils.cc
@@ -21,6 +21,7 @@
#include <unistd.h>
#include "perfetto/ext/base/file_utils.h"
+#include "perfetto/ext/base/optional.h"
#include "perfetto/profiling/normalize.h"
namespace perfetto {
@@ -41,24 +42,25 @@
} // namespace
-std::vector<std::string> NormalizeCmdlines(
+base::Optional<std::vector<std::string>> NormalizeCmdlines(
const std::vector<std::string>& cmdlines) {
std::vector<std::string> normalized_cmdlines;
- for (std::string cmdline : cmdlines) {
+ normalized_cmdlines.reserve(cmdlines.size());
+
+ for (size_t i = 0; i < cmdlines.size(); i++) {
+ std::string cmdline = cmdlines[i]; // mutable copy
// Add nullbyte to make sure it's a C string.
cmdline.resize(cmdline.size() + 1, '\0');
- std::string normalized;
char* cmdline_cstr = &(cmdline[0]);
ssize_t size = NormalizeCmdLine(&cmdline_cstr, cmdline.size());
if (size == -1) {
- PERFETTO_PLOG("Failed to normalize cmdline %s. Skipping.",
- cmdline.c_str());
- continue;
+ PERFETTO_PLOG("Failed to normalize cmdline %s. Stopping the parse.",
+ cmdlines[i].c_str());
+ return base::nullopt;
}
- normalized_cmdlines.emplace_back(
- std::string(cmdline_cstr, static_cast<size_t>(size)));
+ normalized_cmdlines.emplace_back(cmdline_cstr, static_cast<size_t>(size));
}
- return normalized_cmdlines;
+ return base::make_optional(normalized_cmdlines);
}
// This is mostly the same as GetHeapprofdProgramProperty in
diff --git a/src/profiling/common/proc_utils.h b/src/profiling/common/proc_utils.h
index 11a0785..045bf69 100644
--- a/src/profiling/common/proc_utils.h
+++ b/src/profiling/common/proc_utils.h
@@ -21,6 +21,7 @@
#include <set>
#include <vector>
+#include "perfetto/ext/base/optional.h"
#include "perfetto/ext/base/scoped_file.h"
namespace perfetto {
@@ -43,7 +44,7 @@
}
}
-std::vector<std::string> NormalizeCmdlines(
+base::Optional<std::vector<std::string>> NormalizeCmdlines(
const std::vector<std::string>& cmdlines);
void FindAllProfilablePids(std::set<pid_t>* pids);
diff --git a/src/profiling/memory/heapprofd_producer.cc b/src/profiling/memory/heapprofd_producer.cc
index 98c11c7..be277e8 100644
--- a/src/profiling/memory/heapprofd_producer.cc
+++ b/src/profiling/memory/heapprofd_producer.cc
@@ -25,6 +25,7 @@
#include <unistd.h>
#include "perfetto/ext/base/file_utils.h"
+#include "perfetto/ext/base/optional.h"
#include "perfetto/ext/base/string_utils.h"
#include "perfetto/ext/base/thread_task_runner.h"
#include "perfetto/ext/tracing/core/trace_writer.h"
@@ -313,14 +314,18 @@
return;
}
- std::vector<std::string> normalized_cmdlines =
+ base::Optional<std::vector<std::string>> normalized_cmdlines =
NormalizeCmdlines(heapprofd_config.process_cmdline());
+ if (!normalized_cmdlines.has_value()) {
+ PERFETTO_ELOG("Rejecting data source due to invalid cmdline in config.");
+ return;
+ }
// Child mode is only interested in the first data source matching the
// already-connected process.
if (mode_ == HeapprofdMode::kChild) {
if (!ConfigTargetsProcess(heapprofd_config, target_process_,
- normalized_cmdlines)) {
+ normalized_cmdlines.value())) {
PERFETTO_DLOG("Child mode skipping setup of unrelated data source.");
return;
}
@@ -354,7 +359,7 @@
heapprofd_config.block_client_timeout_us();
cli_config.enable_extra_guardrails = ds_config.enable_extra_guardrails();
data_source.config = heapprofd_config;
- data_source.normalized_cmdlines = std::move(normalized_cmdlines);
+ data_source.normalized_cmdlines = std::move(normalized_cmdlines.value());
data_source.stop_timeout_ms = ds_config.stop_timeout_ms();
InterningOutputTracker::WriteFixedInterningsPacket(
diff --git a/src/profiling/memory/java_hprof_producer.cc b/src/profiling/memory/java_hprof_producer.cc
index cfaf633..6422238 100644
--- a/src/profiling/memory/java_hprof_producer.cc
+++ b/src/profiling/memory/java_hprof_producer.cc
@@ -18,6 +18,7 @@
#include <signal.h>
+#include "perfetto/ext/base/optional.h"
#include "perfetto/ext/tracing/core/trace_writer.h"
#include "src/profiling/common/proc_utils.h"
@@ -82,8 +83,13 @@
ds.id = id;
for (uint64_t pid : config.pid())
ds.pids.emplace(static_cast<pid_t>(pid));
- auto normalized_cmdlines = NormalizeCmdlines(config.process_cmdline());
- FindPidsForCmdlines(normalized_cmdlines, &ds.pids);
+ base::Optional<std::vector<std::string>> normalized_cmdlines =
+ NormalizeCmdlines(config.process_cmdline());
+ if (!normalized_cmdlines.has_value()) {
+ PERFETTO_ELOG("Rejecting data source due to invalid cmdline in config.");
+ return;
+ }
+ FindPidsForCmdlines(normalized_cmdlines.value(), &ds.pids);
ds.config = std::move(config);
data_sources_.emplace(id, std::move(ds));
}
