| Message ID | 20201223063905.25784-2-leo.yan@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf arm64: Support SDT | expand |
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? 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(). 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. 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? 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:
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:
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); + arg_idx++; + + if (err < 0) goto error; } }
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(-)