Re: [PATCH v2 2/2] perf maps: Fix fixup_overlap_and_insert that can break sorted by name order

On Tue, Apr 7, 2026 at 5:51 PM <[email protected]> wrote:
>
> Sashiko AI review found 1 potential issue(s):
> - [High] The maps__copy_from function corrupts the maps_by_name array by populating it in address order but flagging it as sorted by name.
>
> --
>
> commit bd0adc21836e1932db4199fc14353b8a08f98a0a
> Author: Ian Rogers <[email protected]>
>
> perf maps: Fix fixup_overlap_and_insert that can break sorted by name order
>
> When an entry in the address array is replaced, the corresponding name
> entry is replaced. The entries names may sort differently and so it is
> important that the sorted by name property be cleared on the maps.
>
> > diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
> > index 7dd6da9d1e4f0..b44bc41f51f3b 100644
> > --- a/tools/perf/util/maps.c
> > +++ b/tools/perf/util/maps.c
> [ ... ]
> > @@ -955,6 +955,7 @@ static int __maps__fixup_overlap_and_insert(struct maps *maps, struct map *new)
> >                       if (maps_by_name) {
> >                               map__put(maps_by_name[ni]);
> >                               maps_by_name[ni] = map__get(new);
> > +                             maps__set_maps_by_name_sorted(maps, false);
> >                       }
> [ ... ]
>
> This isn't a bug in the newly added lines, but while reviewing the maps
> sorting logic, I noticed a regression in maps__copy_from().
>
> In maps__copy_from(), when copying maps from a parent to a destination
> (e.g., during thread fork), the dest_maps_by_name array is populated
> sequentially inside a loop iterating over parent_maps_by_address:
>
> tools/perf/util/maps.c:maps__copy_from() {
> [ ... ]
>                 for (unsigned int i = 0; !err && i < n; i++) {
>                         struct map *pos = parent_maps_by_address[i];
>                         struct map *new = map__clone(pos);
> [ ... ]
>                                 if (!err) {
>                                         dest_maps_by_address[i] = new;
>                                         map__set_kmap_maps(new, dest);
>                                         if (dest_maps_by_name)
>                                                 dest_maps_by_name[i] = map__get(new);
> [ ... ]
> }
>
> Because it clones and copies the elements in address order,
> dest_maps_by_name is strictly populated in address order.
>
> However, at the end of the fast-path copy block, the code executes:
>
> tools/perf/util/maps.c:maps__copy_from() {
> [ ... ]
>                 if (!err) {
>                         RC_CHK_ACCESS(dest)->last_search_by_name_idx =
>                                 RC_CHK_ACCESS(parent)->last_search_by_name_idx;
>                         maps__set_maps_by_name_sorted(dest,
>                                                 dest_maps_by_name &&
>                                                 maps__maps_by_name_sorted(parent));
>                 }
> [ ... ]
> }
>
> If the parent's name array was sorted, does this explicitly flag the new
> destination map's name array as sorted by name, even though it is physically
> sorted by address?
>
> Could this cause any subsequent lookup by name in the copied map
> (such as maps__find_by_name during child thread symbol resolution)
> to execute a binary search on an address-sorted array?
>
> Would this cause the binary search to take incorrect paths and fail to find
> legitimate mappings, leading to missing symbols or missing maps for
> child threads?

This is legitimate, I'll do a v3 with a fix for this issue as well.

Thanks,
Ian

> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2