Hi Sayed, BTW. Did you see my review/question about your previous contribution? https://inbox.sourceware.org/elfutils-devel/[email protected]/T/#t
On Fri, 2026-06-19 at 14:19 +0530, Sayed Kaif wrote: > x86_sample_perf_regs_mapping initialized perf_to_regs[] only up to the > highest set bit of perf_regs_mask, but indexed it with dwarf_to_perf[i] > values up to PERF_REG_X86_64_MAX-1. For a sample carrying only the base > GP registers the upper slots stay uninitialized, and the resulting index > j was used unchecked to write cached_regs_mapping[j], a count-element > calloc buffer. Initialize the whole perf_to_regs[] array to -1, bounds- > check j before the write, and check calloc for failure. > > * backends/x86_initreg_sample.c (x86_sample_perf_regs_mapping): > Initialize full perf_to_regs array, bounds-check index, check > calloc result. > > Signed-off-by: Sayed Kaif <[email protected]> > > Signed-off-by: Sayed Kaif <[email protected]> Double signed off :) I removed one before pushing. > --- > backends/x86_initreg_sample.c | 18 ++++++++++++------ > 1 file changed, 12 insertions(+), 6 deletions(-) > > diff --git a/backends/x86_initreg_sample.c b/backends/x86_initreg_sample.c > index fe870e61..07f0f563 100644 > --- a/backends/x86_initreg_sample.c > +++ b/backends/x86_initreg_sample.c > @@ -65,25 +65,31 @@ x86_sample_perf_regs_mapping (Ebl *ebl, > const int *dwarf_to_perf = is_abi32 ? regs_i386 : regs_x86_64; > > /* Count bits and allocate regs_mapping: */ > - int j, k, kmax, count; uint64_t bit; > - for (k = 0, kmax = -1, count = 0, bit = 1; > + int j, k, count; uint64_t bit; > + for (k = 0, count = 0, bit = 1; > k < PERF_REG_X86_64_MAX; k++, bit <<= 1) > { > if ((bit & perf_regs_mask)) { > count++; > - kmax = k; > } > } OK, don't track kmax anymore. > ebl->cached_perf_regs_mask = perf_regs_mask; > ebl->cached_regs_mapping = (int *)calloc (count, sizeof(int)); > + if (count != 0 && ebl->cached_regs_mapping == NULL) > + return false; > ebl->cached_n_regs_mapping = count; checking ebl->cached_regs_mapping == NULL obviously OK. The count == zero case is kind of funny. It might or might not return an unique pointer (I believe the glibc calloc does). But might return NULL. Either way this check is correct. > /* Locations of perf_regs in the regs[] array, according to > - perf_regs_mask: */ > + perf_regs_mask. Initialize the whole array (not just up to the > + highest set bit): the loop below indexes perf_to_regs[] by > + dwarf_to_perf[i], which can be larger than the highest set bit in > + perf_regs_mask. Leaving the tail uninitialized would read stack > + garbage and could turn into an out-of-bounds write into the > + cached_regs_mapping[] heap buffer. */ > int perf_to_regs[PERF_REG_X86_64_MAX]; > uint64_t expected_mask = is_abi32 ? > PERF_FRAME_REGISTERS_I386 : PERF_FRAME_REGISTERS_X86_64; > - for (j = 0, k = 0, bit = 1; k <= kmax; k++, bit <<= 1) > + for (j = 0, k = 0, bit = 1; k < PERF_REG_X86_64_MAX; k++, bit <<= 1) > { > if ((bit & expected_mask) && (bit & perf_regs_mask)) > { Yes, this makes sense. An alternative might have been to memset the whole array to -1 at the start. But not clear if that would be more efficient. > @@ -104,7 +110,7 @@ x86_sample_perf_regs_mapping (Ebl *ebl, > { > k = dwarf_to_perf[i]; > j = perf_to_regs[k]; > - if (j < 0) continue; > + if (j < 0 || j >= (int)ebl->cached_n_regs_mapping) continue; > ebl->cached_regs_mapping[j] = i; > } Yes, good bounds/sanity check. All looks good. Pushed. Thanks, Mark
