Skip to contentSkip to conversation

perf synthetic events: Avoid write of uninitialized memory.

PATCH Applied Ian Rogers 3 participants 3 messages Mar 9, 2021 – Mar 10, 2021 perf events

lore
Share & tools
lore

Ian Rogers

author reviewer
#0 lore raw

PATCH 1/1 perf synthetic events: Avoid write of uninitialized memory.

Account for alignment bytes in the zero-ing memset.

Signed-off-by Ian Rogers <[email protected]>
---
 tools/perf/util/synthetic-events.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
tools/perf/util/synthetic-events.c +5 -4
index b698046ec2db..31bf3dd6a1e0 100644
@@ -424,7 +424,7 @@ int perf_event__synthesize_mmap_events(struct perf_tool *tool,
 
 	while (!io.eof) {
 		static const char anonstr[] = "//anon";
-		size_t size;
+		size_t size, aligned_size;
 
 		/* ensure null termination since stack will be reused. */
 		event->mmap2.filename[0] = '\0';

@@ -484,11 +484,12 @@ int perf_event__synthesize_mmap_events(struct perf_tool *tool,
 		}
 
 		size = strlen(event->mmap2.filename) + 1;
-		size = PERF_ALIGN(size, sizeof(u64));
+		aligned_size = PERF_ALIGN(size, sizeof(u64));
 		event->mmap2.len -= event->mmap.start;
 		event->mmap2.header.size = (sizeof(event->mmap2) -
-					(sizeof(event->mmap2.filename) - size));
-		memset(event->mmap2.filename + size, 0, machine->id_hdr_size);
+					(sizeof(event->mmap2.filename) - aligned_size));
+		memset(event->mmap2.filename + size, 0, machine->id_hdr_size +
+			(aligned_size - size));
 		event->mmap2.header.size += machine->id_hdr_size;
 		event->mmap2.pid = tgid;
 		event->mmap2.tid = pid;
Signature · 1 line
2.30.1.766.gb4fecdf3b7-goog
Acked-by on v1 1/1
#1 lore raw
Quoted 23 lines · Ian Rogers

Account for alignment bytes in the zero-ing memset.

Signed-off-by Ian Rogers <[email protected]>
---
 tools/perf/util/synthetic-events.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
tools/perf/util/synthetic-events.c +1 -1
index b698046ec2db..31bf3dd6a1e0 100644
@@ -424,7 +424,7 @@ int perf_event__synthesize_mmap_events(struct perf_tool *tool,
 
 	while (!io.eof) {
 		static const char anonstr[] = "//anon";
-		size_t size;
+		size_t size, aligned_size;
 
 		/* ensure null termination since stack will be reused. */
 		event->mmap2.filename[0] = '\0';

@@ -484,11 +484,12 @@ int perf_event__synthesize_mmap_events(struct perf_tool *tool,
 		}

tools/perf/util/synthetic-events.c int perf_event__synthesize_mmap_events(struct perf_tool *tool, · line 492

Quoted 10 lines
tools/perf/util/synthetic-events.c +4 -3
 		size = strlen(event->mmap2.filename) + 1;
-		size = PERF_ALIGN(size, sizeof(u64));
+		aligned_size = PERF_ALIGN(size, sizeof(u64));
 		event->mmap2.len -= event->mmap.start;
 		event->mmap2.header.size = (sizeof(event->mmap2) -
-					(sizeof(event->mmap2.filename) - size));
-		memset(event->mmap2.filename + size, 0, machine->id_hdr_size);
+					(sizeof(event->mmap2.filename) - aligned_size));
+		memset(event->mmap2.filename + size, 0, machine->id_hdr_size +
+			(aligned_size - size));

so we did not zero the extra alignment bytes, nice ;-) looks good

Acked-by Jiri Olsa <[email protected]>

thanks,
jirka

Quoted 6 lines
tools/perf/util/synthetic-events.c +0 -0
 		event->mmap2.header.size += machine->id_hdr_size;
 		event->mmap2.pid = tgid;
 		event->mmap2.tid = pid;
Signature · 1 line
2.30.1.766.gb4fecdf3b7-goog
maintainer Applied
#2 lore raw

Em Wed, Mar 10, 2021 at 12:48:36PM +0100, Jiri Olsa escreveu:

Quoted 37 lines · Ian Rogers
Quoted 23 lines

Account for alignment bytes in the zero-ing memset.

Signed-off-by Ian Rogers <[email protected]>
---
 tools/perf/util/synthetic-events.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
tools/perf/util/synthetic-events.c +1 -1
index b698046ec2db..31bf3dd6a1e0 100644
@@ -424,7 +424,7 @@ int perf_event__synthesize_mmap_events(struct perf_tool *tool,
 
 	while (!io.eof) {
 		static const char anonstr[] = "//anon";
-		size_t size;
+		size_t size, aligned_size;
 
 		/* ensure null termination since stack will be reused. */
 		event->mmap2.filename[0] = '\0';

@@ -484,11 +484,12 @@ int perf_event__synthesize_mmap_events(struct perf_tool *tool,
 		}

tools/perf/util/synthetic-events.c int perf_event__synthesize_mmap_events(struct perf_tool *tool, · line 492

Quoted 10 lines
tools/perf/util/synthetic-events.c +4 -3
 		size = strlen(event->mmap2.filename) + 1;
-		size = PERF_ALIGN(size, sizeof(u64));
+		aligned_size = PERF_ALIGN(size, sizeof(u64));
 		event->mmap2.len -= event->mmap.start;
 		event->mmap2.header.size = (sizeof(event->mmap2) -
-					(sizeof(event->mmap2.filename) - size));
-		memset(event->mmap2.filename + size, 0, machine->id_hdr_size);
+					(sizeof(event->mmap2.filename) - aligned_size));
+		memset(event->mmap2.filename + size, 0, machine->id_hdr_size +
+			(aligned_size - size));

so we did not zero the extra alignment bytes, nice ;-) looks good

Quoted 1 line
Acked-by Jiri Olsa <[email protected]>

That is really old:

Fixes 1a853e36871b533c ("perf record: Allow specifying a pid to record")

Circa 2009, the PERF_RECORD_COMM is ok as TASK_COMM_LEN is 16.

But I think there are other places synthesizing PERF_RECORD_MMAP, jitdump maybe:

tools/perf/bench/inject-buildid.c, but it uses memset to zero the whole union, no problem.

tools/perf/util/jitdump.c
jit_repipe_code_load() but it uses calloc to allocate the union perf_event, so no problem as well.

Thanks, applied.

- Arnaldo