Messages in this thread | | | Date | Fri, 25 Dec 2020 10:27:00 +0800 | From | Leo Yan <> | Subject | Re: [PATCH v1 1/2] perf probe: Fixup Arm64 SDT arguments |
| |
On Thu, Dec 24, 2020 at 10:51:39AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Dec 23, 2020 at 02:39:04PM +0800, Leo Yan escreveu: > > Arm64 ELF section '.note.stapsdt' uses string format "-4@[sp, NUM]" if > > the probe is to access data in stack, e.g. below is an example for > > dumping Arm64 ELF file and shows the argument format: > > > > Arguments: -4@[sp, 12] -4@[sp, 8] -4@[sp, 4] > > > > Comparing against other archs' argument format, Arm64's argument > > introduces an extra space character in the middle of square brackets, > > due to argv_split() uses space as splitter, the argument is wrongly > > divided into two items. > > > > To support Arm64 SDT, this patch fixes up for this case, if any item > > contains sub string "[sp", concatenates the two continuous items. And > > adds the detailed explaination in comment. > > > > Signed-off-by: Leo Yan <leo.yan@linaro.org> > > --- > > tools/perf/util/probe-file.c | 32 ++++++++++++++++++++++++++++++-- > > 1 file changed, 30 insertions(+), 2 deletions(-) > > > > diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c > > index 064b63a6a3f3..60878c859e60 100644 > > --- a/tools/perf/util/probe-file.c > > +++ b/tools/perf/util/probe-file.c > > @@ -794,6 +794,8 @@ static char *synthesize_sdt_probe_command(struct sdt_note *note, > > char *ret = NULL, **args; > > int i, args_count, err; > > unsigned long long ref_ctr_offset; > > + char *arg; > > + int arg_idx = 0; > > > > if (strbuf_init(&buf, 32) < 0) > > return NULL; > > @@ -815,8 +817,34 @@ static char *synthesize_sdt_probe_command(struct sdt_note *note, > > if (note->args) { > > args = argv_split(note->args, &args_count); > > > > - for (i = 0; i < args_count; ++i) { > > - if (synthesize_sdt_probe_arg(&buf, i, args[i]) < 0) > > + for (i = 0; i < args_count; ) { > > + /* > > + * FIXUP: Arm64 ELF section '.note.stapsdt' uses string > > + * format "-4@[sp, NUM]" if a probe is to access data in > > + * the stack, e.g. below is an example for the SDT > > + * Arguments: > > + * > > + * Arguments: -4@[sp, 12] -4@[sp, 8] -4@[sp, 4] > > + * > > + * Since the string introduces an extra space character > > + * in the middle of square brackets, the argument is > > + * divided into two items. Fixup for this case, if an > > + * item contains sub string "[sp,", need to concatenate > > + * the two items. > > + */ > > + if (strstr(args[i], "[sp,") && (i+1) < args_count) { > > + arg = strcat(args[i], args[i+1]); > > + i += 2; > > + } else { > > + arg = strdup(args[i]); > > + i += 1; > > + } > > + > > + err = synthesize_sdt_probe_arg(&buf, arg_idx, arg); > > + free(arg); > > So you free here unconditionally because either you used something you > got from argv_split() that strdup'ed all the entries in the array it > returns, or that you strdup'ed in the else branch.
> But then you may not free all the things argv_split() returned, right?
Yes.
> Also, that strcat(args[i], args[i+1]), are you sure that is safe? strcat > expects dest to have enough space for the concatenation, I don't see > argv_split[] adding extra bytes, just a strdup().
Correct, will change to use asprintf().
> So probably you need asprintf() where you use strcat() and then, at the > end of the loop, you need to free what argv_split() returned, using > argv_free(), no? > > Also please check strdup() (and then asprintf) managed to allocate, else > synthesize_sdt_probe_arg() will receive its 'desc' argument as NULL, do > _another_ strdup on it and boom.
Will add checking for the pointer from strdup()/asprintf().
> Or am I missing something? :) > > I just looked ant synthesize_sdt_probe_command() is leaking the args it > gets from argv_split() > > So this patch is needed, ack?
Below change is good for me. In the next respin, I will add this new patch with your author name and send out.
Thanks a lot for the review, Masami & Arnaldo!
> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c > index 064b63a6a3f311cd..bbecb449ea944395 100644 > --- a/tools/perf/util/probe-file.c > +++ b/tools/perf/util/probe-file.c > @@ -791,7 +791,7 @@ static char *synthesize_sdt_probe_command(struct sdt_note *note, > const char *sdtgrp) > { > struct strbuf buf; > - char *ret = NULL, **args; > + char *ret = NULL; > int i, args_count, err; > unsigned long long ref_ctr_offset; > > @@ -813,12 +813,19 @@ static char *synthesize_sdt_probe_command(struct sdt_note *note, > goto out; > > if (note->args) { > - args = argv_split(note->args, &args_count); > + char **args = argv_split(note->args, &args_count); > + > + if (args == NULL) > + goto error; > > for (i = 0; i < args_count; ++i) { > - if (synthesize_sdt_probe_arg(&buf, i, args[i]) < 0) > + if (synthesize_sdt_probe_arg(&buf, i, args[i]) < 0) { > + argv_free(args); > goto error; > + } > } > + > + argv_free(args); > } > > out:
| |