Skip to content

Commit

Permalink
Fix TraceBuffer bound checks + various printf fmt
Browse files Browse the repository at this point in the history
I tried to update clang to the version in b/384391188 and
that highlighted
1. A bunch of printf formatters that we were holding plain-wrong
   until now, which clang now identifies correctly.
2. Clang not liking pointer arithmetic with possibly out-of-bounds
   pointers.

I'm holing off updating clang to a different CL. for now these
changes are good regardless of the compiler verison.

Bug: 384391188
Change-Id: I309463c4016187feed5eb33b5ba2e54e2227797a
  • Loading branch information
primiano committed Dec 16, 2024
1 parent 6dbcefe commit a472c95
Show file tree
Hide file tree
Showing 29 changed files with 93 additions and 90 deletions.
2 changes: 1 addition & 1 deletion src/base/subprocess_posix.cc
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ void Subprocess::TryReadExitStatus() {
} else if (WIFSIGNALED(pid_stat)) {
s_->returncode = 128 + WTERMSIG(pid_stat); // Follow bash convention.
} else {
PERFETTO_FATAL("waitpid() returned an unexpected value (0x%x)", pid_stat);
PERFETTO_FATAL("waitpid() returned an unexpected value (%d)", pid_stat);
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/base/unix_socket.cc
Original file line number Diff line number Diff line change
Expand Up @@ -745,7 +745,7 @@ std::string UnixSocketRaw::GetSockAddr() const {
PERFETTO_BUILDFLAG(PERFETTO_OS_ANDROID))
if (stg.ss_family == AF_VSOCK) {
auto* saddr = reinterpret_cast<struct sockaddr_vm*>(&stg);
base::StackString<255> addr_and_port("%s%d:%d", kVsockNamePrefix,
base::StackString<255> addr_and_port("%s%u:%u", kVsockNamePrefix,
saddr->svm_cid, saddr->svm_port);
return addr_and_port.ToStdString();
}
Expand Down
2 changes: 1 addition & 1 deletion src/perfetto_cmd/perfetto_cmd.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1043,7 +1043,7 @@ void PerfettoCmd::OnConnect() {
}

if (expected_duration_ms_) {
PERFETTO_LOG("Connected to the Perfetto traced service, TTL: %ds",
PERFETTO_LOG("Connected to the Perfetto traced service, TTL: %us",
(expected_duration_ms_ + 999) / 1000);
} else {
PERFETTO_LOG("Connected to the Perfetto traced service, starting tracing");
Expand Down
5 changes: 2 additions & 3 deletions src/profiling/common/proc_utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,8 @@ void RemoveUnderAnonThreshold(uint32_t min_size_kb, std::set<pid_t>* pids) {
rss_and_swap = GetRssAnonAndSwap(*status);

if (rss_and_swap && rss_and_swap < min_size_kb) {
PERFETTO_LOG("Removing pid %d from profiled set (anon: %d kB < %" PRIu32
")",
pid, *rss_and_swap, min_size_kb);
PERFETTO_LOG("Removing pid %d from profiled set (anon: %u kB < %u)", pid,
*rss_and_swap, min_size_kb);
it = pids->erase(it);
} else {
++it;
Expand Down
2 changes: 1 addition & 1 deletion src/protozero/field.cc
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ void Field::SerializeAndAppendToInternal(Container* dst) const {
break;
}
default:
PERFETTO_FATAL("Unknown field type %u", type_);
PERFETTO_FATAL("Unknown field type %d", type_);
}
size_t written_size = static_cast<size_t>(wptr - start);
PERFETTO_DCHECK(written_size > 0 && written_size < pu::kMaxMessageLength);
Expand Down
2 changes: 1 addition & 1 deletion src/tools/cpu_utilization/cpu_utilization.cc
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ int CpuUtilizationMain(int argc, char** argv) {
PERFETTO_LOG("--- setup: ---");
PERFETTO_LOG("target pid: %d", target_pid);
PERFETTO_LOG("intervals: %d x %uus", sleep_intervals, sleep_duration_us);
PERFETTO_LOG("utime/stime ticks per sec: %ld", ticks_per_s);
PERFETTO_LOG("utime/stime ticks per sec: %lu", ticks_per_s);
PERFETTO_LOG("wall clock resolution (ns): %ld", ts.tv_nsec);
PERFETTO_LOG("--- timings: ---");

Expand Down
2 changes: 1 addition & 1 deletion src/trace_processor/importers/common/clock_tracker.h
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ class ClockTracker {
PERFETTO_DCHECK(!IsSequenceClock(clock_id));
if (trace_time_clock_id_used_for_conversion_ &&
trace_time_clock_id_ != clock_id) {
PERFETTO_ELOG("Not updating trace time clock from %" PRIu64 " to %" PRIu64
PERFETTO_ELOG("Not updating trace time clock from %" PRId64 " to %" PRId64
" because the old clock was already used for timestamp "
"conversion - ClockSnapshot too late in trace?",
trace_time_clock_id_, clock_id);
Expand Down
2 changes: 1 addition & 1 deletion src/trace_processor/importers/ftrace/drm_tracker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ constexpr auto kVblankBlueprint = tracks::SliceBlueprint(
"drm_vblank",
tracks::DimensionBlueprints(tracks::UintDimensionBlueprint("drm_crtc")),
tracks::FnNameBlueprint([](uint32_t crtc) {
return base::StackString<256>("vblank-%d", crtc);
return base::StackString<256>("vblank-%u", crtc);
}));

constexpr auto kSchedRingBlueprint = tracks::SliceBlueprint(
Expand Down
6 changes: 3 additions & 3 deletions src/trace_processor/importers/ftrace/ftrace_parser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2188,7 +2188,7 @@ void FtraceParser::ParseSysEnterEvent(int64_t timestamp,
uint32_t count = 0;
for (auto it = evt.args(); it; ++it) {
if (syscall_arg_name_ids_.size() == count) {
base::StackString<32> string_arg("args[%" PRId32 "]", count);
base::StackString<32> string_arg("args[%u]", count);
auto string_id =
context_->storage->InternString(string_arg.string_view());
syscall_arg_name_ids_.emplace_back(string_id);
Expand Down Expand Up @@ -3428,7 +3428,7 @@ void FtraceParser::ParseUfshcdCommand(int64_t timestamp,
"ufs_command_tag",
tracks::DimensionBlueprints(tracks::UintDimensionBlueprint("ufs_tag")),
tracks::FnNameBlueprint([](uint32_t tag) {
return base::StackString<32>("io.ufs.command.tag[%03d]", tag);
return base::StackString<32>("io.ufs.command.tag[%03u]", tag);
}));

// Parse ufs command tag
Expand Down Expand Up @@ -3510,7 +3510,7 @@ void FtraceParser::ParseSuspendResume(int64_t timestamp,
// processor.
auto val = (action_name == "timekeeping_freeze") ? 0 : evt.val();

base::StackString<64> str("%s(%" PRIu32 ")", action_name.c_str(), val);
base::StackString<64> str("%s(%d)", action_name.c_str(), val);
std::string current_action = str.ToStdString();

StringId slice_name_id = context_->storage->InternString(str.string_view());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ void PkvmHypervisorCpuTracker::ParseHypEvent(uint32_t cpu,
break;
// TODO(b/249050813): add remaining hypervisor events
default:
PERFETTO_FATAL("Not a hypervisor event %d", event_id);
PERFETTO_FATAL("Not a hypervisor event %u", event_id);
}
}

Expand Down
12 changes: 6 additions & 6 deletions src/trace_processor/importers/ftrace/v4l2_tracker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ void V4l2Tracker::ParseV4l2Event(uint64_t fld_id,
evt.timecode_userbits3 = pb_evt.timecode_userbits3();

base::StackString<64> buf_name(
"VIDIOC_QBUF minor=%" PRIu32 " seq=%" PRIu32 " type=%" PRIu32
"VIDIOC_QBUF minor=%" PRId32 " seq=%" PRIu32 " type=%" PRIu32
" index=%" PRIu32,
evt.device_minor, evt.sequence, *evt.type, *evt.index);

Expand Down Expand Up @@ -123,7 +123,7 @@ void V4l2Tracker::ParseV4l2Event(uint64_t fld_id,
evt.timecode_userbits3 = pb_evt.timecode_userbits3();

base::StackString<64> buf_name(
"VIDIOC_DQBUF minor=%" PRIu32 " seq=%" PRIu32 " type=%" PRIu32
"VIDIOC_DQBUF minor=%" PRId32 " seq=%" PRIu32 " type=%" PRIu32
" index=%" PRIu32,
evt.device_minor, evt.sequence, *evt.type, *evt.index);

Expand Down Expand Up @@ -168,7 +168,7 @@ void V4l2Tracker::ParseV4l2Event(uint64_t fld_id,
evt.timecode_userbits2 = pb_evt.timecode_userbits2();
evt.timecode_userbits3 = pb_evt.timecode_userbits3();

base::StackString<64> buf_name("vb2_v4l2_buf_queue minor=%" PRIu32
base::StackString<64> buf_name("vb2_v4l2_buf_queue minor=%" PRId32
" seq=%" PRIu32 " type=0 index=0",
evt.device_minor, evt.sequence);

Expand Down Expand Up @@ -199,7 +199,7 @@ void V4l2Tracker::ParseV4l2Event(uint64_t fld_id,
evt.timecode_userbits2 = pb_evt.timecode_userbits2();
evt.timecode_userbits3 = pb_evt.timecode_userbits3();

base::StackString<64> buf_name("vb2_v4l2_buf_done minor=%" PRIu32
base::StackString<64> buf_name("vb2_v4l2_buf_done minor=%" PRId32
" seq=%" PRIu32 " type=0 index=0",
evt.device_minor, evt.sequence);

Expand Down Expand Up @@ -230,7 +230,7 @@ void V4l2Tracker::ParseV4l2Event(uint64_t fld_id,
evt.timecode_userbits2 = pb_evt.timecode_userbits2();
evt.timecode_userbits3 = pb_evt.timecode_userbits3();

base::StackString<64> buf_name("vb2_v4l2_qbuf minor=%" PRIu32
base::StackString<64> buf_name("vb2_v4l2_qbuf minor=%" PRId32
" seq=%" PRIu32 " type=0 index=0",
evt.device_minor, evt.sequence);

Expand Down Expand Up @@ -261,7 +261,7 @@ void V4l2Tracker::ParseV4l2Event(uint64_t fld_id,
evt.timecode_userbits2 = pb_evt.timecode_userbits2();
evt.timecode_userbits3 = pb_evt.timecode_userbits3();

base::StackString<64> buf_name("vb2_v4l2_qbuf minor=%" PRIu32
base::StackString<64> buf_name("vb2_v4l2_qbuf minor=%" PRId32
" seq=%" PRIu32 " type=0 index=0",
evt.device_minor, evt.sequence);

Expand Down
6 changes: 3 additions & 3 deletions src/trace_processor/importers/ftrace/virtio_video_tracker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ void VirtioVideoTracker::ParseVirtioVideoEvent(uint64_t fld_id,
uint64_t hash = base::Hasher::Combine(
pb_evt.stream_id(), pb_evt.resource_id(), pb_evt.queue_type());

base::StackString<64> name("Resource #%" PRIu32, pb_evt.resource_id());
base::StackString<64> name("Resource #%d", pb_evt.resource_id());
StringId name_id = context_->storage->InternString(name.string_view());

TrackSetId track_set_id =
Expand Down Expand Up @@ -178,8 +178,8 @@ void VirtioVideoTracker::AddCommandSlice(int64_t timestamp,

const char* suffix = response ? "Responses" : "Requests";

base::StackString<64> track_name("virtio_video stream #%" PRId32 " %s",
stream_id, suffix);
base::StackString<64> track_name("virtio_video stream #%u %s", stream_id,
suffix);
StringId track_name_id =
context_->storage->InternString(track_name.string_view());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ void AndroidCameraEventModule::InsertCameraFrameSlice(
const auto android_camera_frame_event =
protos::pbzero::AndroidCameraFrameEvent::Decoder(bytes);
StringId track_name = context_->storage->InternString(
base::StackString<32>("Camera %d Frames",
base::StackString<32>("Camera %u Frames",
android_camera_frame_event.camera_id())
.string_view());
StringId slice_name = context_->storage->InternString(
Expand Down
2 changes: 1 addition & 1 deletion src/trace_processor/importers/proto/gpu_event_parser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ void GpuEventParser::ParseGpuCounterEvent(int64_t ts, ConstBytes blob) {
}
} else {
// Either counter spec was repeated or it came after counter data.
PERFETTO_ELOG("Duplicated counter spec found. (counter_id=%d, name=%s)",
PERFETTO_ELOG("Duplicated counter spec found. (counter_id=%u, name=%s)",
counter_id, name.ToStdString().c_str());
context_->storage->IncrementStats(stats::gpu_counters_invalid_spec);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ StringId NetworkTraceModule::GetIpProto(NetworkPacketEvent::Decoder& evt) {
return net_ipproto_icmpv6_;
default:
return context_->storage->InternString(
base::StackString<32>("IPPROTO (%d)", evt.ip_proto()).string_view());
base::StackString<32>("IPPROTO (%u)", evt.ip_proto()).string_view());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ void ProtoTraceParserImpl::ParseMetatraceEvent(int64_t ts, ConstBytes blob) {
if (eid < metatrace::EVENTS_MAX) {
name_id = context_->storage->InternString(metatrace::kEventNames[eid]);
} else {
base::StackString<64> fallback("Event %d", eid);
base::StackString<64> fallback("Event %u", eid);
name_id = context_->storage->InternString(fallback.string_view());
}
} else if (event.has_event_name_iid()) {
Expand All @@ -356,7 +356,7 @@ void ProtoTraceParserImpl::ParseMetatraceEvent(int64_t ts, ConstBytes blob) {
name_id =
context_->storage->InternString(metatrace::kCounterNames[cid]);
} else {
base::StackString<64> fallback("Counter %d", cid);
base::StackString<64> fallback("Counter %u", cid);
name_id = context_->storage->InternString(fallback.string_view());
}
track = context_->track_tracker->InternTrack(
Expand Down
2 changes: 1 addition & 1 deletion src/trace_processor/importers/proto/proto_trace_reader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,7 @@ base::Status ProtoTraceReader::ParseClockSnapshot(ConstBytes blob,
if (!seq_id) {
return base::ErrStatus(
"ClockSnapshot packet is specifying a sequence-scoped clock id "
"(%" PRIu64 ") but the TracePacket sequence_id is zero",
"(%" PRId64 ") but the TracePacket sequence_id is zero",
clock_id);
}
clock_id = ClockTracker::SequenceToGlobalClock(seq_id, clk.clock_id());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,15 @@ std::optional<DecodedMessage> ProtoLogMessageDecoder::Decode(
break;
}
case 'o': {
base::StackString<32> param("%" PRIo64, *sint64_params_itr);
base::StackString<32> param(
"%" PRIo64, static_cast<uint64_t>(*sint64_params_itr));
formatted_message.append(param.c_str());
++sint64_params_itr;
break;
}
case 'x': {
base::StackString<32> param("%" PRIx64, *sint64_params_itr);
base::StackString<32> param(
"%" PRIx64, static_cast<uint64_t>(*sint64_params_itr));
formatted_message.append(param.c_str());
++sint64_params_itr;
break;
Expand Down Expand Up @@ -127,7 +129,7 @@ void ProtoLogMessageDecoder::TrackGroup(uint32_t id, const std::string& tag) {
auto tracked_group = tracked_groups_.Find(id);
if (tracked_group != nullptr && tracked_group->tag != tag) {
context_->storage->IncrementStats(
stats::winscope_protolog_view_config_collision);
stats::winscope_protolog_view_config_collision);
}
tracked_groups_.Insert(id, TrackedGroup{tag});
}
Expand All @@ -141,7 +143,7 @@ void ProtoLogMessageDecoder::TrackMessage(
auto tracked_message = tracked_messages_.Find(message_id);
if (tracked_message != nullptr && tracked_message->message != message) {
context_->storage->IncrementStats(
stats::winscope_protolog_view_config_collision);
stats::winscope_protolog_view_config_collision);
}
tracked_messages_.Insert(message_id,
TrackedMessage{level, group_id, message, location});
Expand Down
10 changes: 5 additions & 5 deletions src/trace_processor/metrics/metrics.cc
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ base::StatusOr<protozero::ConstBytes> ValidateSingleNonEmptyMessage(
single_field.size);

if (single.type() != schema_type) {
return base::ErrStatus("Message field has wrong wire type %d",
return base::ErrStatus("Message field has wrong wire type %u",
single.type());
}

Expand Down Expand Up @@ -207,7 +207,7 @@ base::Status ProtoBuilder::AppendSingleLong(const FieldDescriptor& field,
default: {
return base::ErrStatus(
"Tried to write value of type long into field %s (in proto type %s) "
"which has type %d",
"which has type %u",
field.name().c_str(), descriptor_->full_name().c_str(), field.type());
}
}
Expand All @@ -230,7 +230,7 @@ base::Status ProtoBuilder::AppendSingleDouble(const FieldDescriptor& field,
default: {
return base::ErrStatus(
"Tried to write value of type double into field %s (in proto type "
"%s) which has type %d",
"%s) which has type %u",
field.name().c_str(), descriptor_->full_name().c_str(), field.type());
}
}
Expand Down Expand Up @@ -272,7 +272,7 @@ base::Status ProtoBuilder::AppendSingleString(const FieldDescriptor& field,
default: {
return base::ErrStatus(
"Tried to write value of type string into field %s (in proto type "
"%s) which has type %d",
"%s) which has type %u",
field.name().c_str(), descriptor_->full_name().c_str(), field.type());
}
}
Expand Down Expand Up @@ -316,7 +316,7 @@ base::Status ProtoBuilder::AppendSingleBytes(const FieldDescriptor& field,

return base::ErrStatus(
"Tried to write value of type bytes into field %s (in proto type %s) "
"which has type %d",
"which has type %u",
field.name().c_str(), descriptor_->full_name().c_str(), field.type());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1015,16 +1015,16 @@ PerfettoSqlEngine::GetColumnNamesFromSelectStatement(
std::string col_name =
sqlite3_column_name(stmt.sqlite_stmt(), static_cast<int>(i));
if (col_name.empty()) {
return base::ErrStatus("%s: column %d: name must not be empty", tag, i);
return base::ErrStatus("%s: column %u: name must not be empty", tag, i);
}
if (!std::isalpha(col_name.front())) {
return base::ErrStatus(
"%s: Column %i: name '%s' has to start with a letter.", tag, i,
"%s: Column %u: name '%s' has to start with a letter.", tag, i,
col_name.c_str());
}
if (!sql_argument::IsValidName(base::StringView(col_name))) {
return base::ErrStatus(
"%s: Column %i: name '%s' has to contain only alphanumeric "
"%s: Column %u: name '%s' has to contain only alphanumeric "
"characters and underscores.",
tag, i, col_name.c_str());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ struct Struct : public SqliteFunction<Struct> {
}
if (argc / 2 > perfetto_sql::Struct::kMaxFields) {
return sqlite::utils::SetError(
ctx, base::ErrStatus("STRUCT: only at most %d fields are supported",
ctx, base::ErrStatus("STRUCT: only at most %u fields are supported",
perfetto_sql::Struct::kMaxFields));
}

Expand Down
2 changes: 1 addition & 1 deletion src/trace_processor/trace_processor_shell.cc
Original file line number Diff line number Diff line change
Expand Up @@ -514,7 +514,7 @@ base::StatusOr<QueryResult> ExtractQueryResult(Iterator* it, bool has_more) {
QueryResult result;

for (uint32_t c = 0; c < it->ColumnCount(); c++) {
fprintf(stderr, "column %d = %s\n", c, it->GetColumnName(c).c_str());
fprintf(stderr, "column %u = %s\n", c, it->GetColumnName(c).c_str());
result.column_names.push_back(it->GetColumnName(c));
}

Expand Down
4 changes: 2 additions & 2 deletions src/traceconv/trace_to_systrace.cc
Original file line number Diff line number Diff line change
Expand Up @@ -244,13 +244,13 @@ int ExtractRawEvents(TraceWriter* trace_writer,

// 2. Write the actual events.
if (truncate_keep == Keep::kEnd && raw_events > max_ftrace_events) {
base::StackString<150> end_truncate("%s limit %d offset %d",
base::StackString<150> end_truncate("%s limit %u offset %u",
kRawEventsQuery, max_ftrace_events,
raw_events - max_ftrace_events);
if (!q_writer.RunQuery(end_truncate.ToStdString(), raw_callback))
return 1;
} else if (truncate_keep == Keep::kStart) {
base::StackString<150> start_truncate("%s limit %d", kRawEventsQuery,
base::StackString<150> start_truncate("%s limit %u", kRawEventsQuery,
max_ftrace_events);
if (!q_writer.RunQuery(start_truncate.ToStdString(), raw_callback))
return 1;
Expand Down
2 changes: 1 addition & 1 deletion src/traced/probes/ftrace/ftrace_procfs.cc
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,7 @@ void FtraceProcfs::ClearTrace() {

void FtraceProcfs::ClearPerCpuTrace(size_t cpu) {
if (!ClearFile(root_ + "per_cpu/cpu" + std::to_string(cpu) + "/trace"))
PERFETTO_ELOG("Failed to clear buffer for CPU %zd", cpu);
PERFETTO_ELOG("Failed to clear buffer for CPU %zu", cpu);
}

bool FtraceProcfs::WriteTraceMarker(const std::string& str) {
Expand Down
2 changes: 1 addition & 1 deletion src/traced/probes/ftrace/proto_translation_table.cc
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ bool InferFtraceType(const std::string& type_and_name,
// TODO(fmayer): Handle u32[], u8[], __u8[] as well.
if (Contains(type_and_name, "__data_loc char[] ")) {
if (size != 4) {
PERFETTO_ELOG("__data_loc with incorrect size: %s (%zd)",
PERFETTO_ELOG("__data_loc with incorrect size: %s (%zu)",
type_and_name.c_str(), size);
return false;
}
Expand Down
Loading

0 comments on commit a472c95

Please sign in to comment.