Re: [PATCH] perf tools: Avoid unaligned pointer operations

[Ian Rogers]

On Thu, Dec 12, 2024 at 1:01 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>
> On Thu, Dec 12, 2024 at 03:41:02PM -0300, Arnaldo Carvalho de Melo wrote:
> > On Wed, Nov 27, 2024 at 04:51:15PM -0800, Ian Rogers wrote:
> > > On Wed, Nov 27, 2024 at 1:26 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> > > > The sample data is 64-bit aligned basically but raw data starts with
> > > > 32-bit length field and data follows.  In perf_event__synthesize_sample
> > > > it treats the sample data as a 64-bit array.  And it needs some trick
> > > > to update the raw data properly.
> >
> > > > But it seems some compilers are not happy with this and the program dies
> > > > siliently.  I found the sample parsing test failed without any messages
> > > > on affected systems.
> >
> > > > Let's update the code to use a 32-bit pointer directly and make sure the
> > > > result is 64-bit aligned again.  No functional changes intended.
> >
> > > > Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> >
> > > Reviewed-by: Ian Rogers <irogers@xxxxxxxxxx>
> >
> > Looks good, applied to perf-tools-next since this is something that is
> > not new nor looks urgent.
> >
> > I think that since we have multiple maintainers, one for not urgent
> > stuff/development and the other for the current window/urgent stuff,
> > that we should express the expectation about where a patch should be
> > processed, by having on the subject the tree the submitter thinks should
> > take the patch, i.e. for this one:
> >
> > [PATCH next] perf tools: Avoid unaligned pointer operations
> >
> > While for urgent stuff we could do:
> >
> > [PATCH urgent] perf tools: Avoid unaligned pointer operations
> >
> > wdyt?
>
> Looks good.  It'd be really great if contributors can do this.
>
> But I also think 'next' should be the default so only 'urgent' would be
> specified if needed.

Fwiw, I needed this fix, forgot about this change, and wrote my own by
just sinking the unaligned array computation (that causes undefined
behavior) into where it was used:
```
--- a/tools/perf/util/synthetic-events.c
+++ b/tools/perf/util/synthetic-events.c
@@ -1690,10 +1690,9 @@ int perf_event__synthesize_sample(union
perf_event *event, u64 type, u64 read_fo
        if (type & PERF_SAMPLE_RAW) {
                u.val32[0] = sample->raw_size;
                *array = u.val64;
-               array = (void *)array + sizeof(u32);

-               memcpy(array, sample->raw_data, sample->raw_size);
-               array = (void *)array + sample->raw_size;
+               memcpy((void *)array + sizeof(u32), sample->raw_data,
sample->raw_size);
+               array = (void *)array + sizeof(u32) + sample->raw_size;
        }

        if (type & PERF_SAMPLE_BRANCH_STACK) {
````
Namhyung's change is better because of the BUG_ON. Perhaps that BUG_ON
should appear after all the void* math that can create unaligned u64
pointers in this function.

Thanks,
Ian