Re: [PATCH perf/core v2 2/8] perf probe: Check the return value of strbuf APIs
From: Masami Hiramatsu
Date: Mon May 09 2016 - 22:37:54 EST
Hi Arnaldo,
Thank you for reviewing!
On Thu, 5 May 2016 20:46:11 -0300
Arnaldo Carvalho de Melo <acme@xxxxxxxxxx> wrote:
> Em Sat, Apr 30, 2016 at 12:10:00AM +0900, Masami Hiramatsu escreveu:
> > Check the return value of strbuf APIs in perf-probe
> > related code, so that it can handle errors in strbuf.
> >
> > Signed-off-by: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> > ---
> > Changes in V2:
> > - Rebased on the latest perf/core.
> > ---
> > tools/perf/util/dwarf-aux.c | 50 ++++++--------
> > tools/perf/util/probe-event.c | 146 ++++++++++++++++++++++++----------------
> > tools/perf/util/probe-finder.c | 30 +++++---
> > 3 files changed, 126 insertions(+), 100 deletions(-)
> >
> > diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> > index aea189b..8c8b4c1 100644
> > --- a/tools/perf/util/dwarf-aux.c
> > +++ b/tools/perf/util/dwarf-aux.c
> > @@ -915,8 +915,7 @@ int die_get_typename(Dwarf_Die *vr_die, struct strbuf *buf)
> > tmp = "*";
> > else if (tag == DW_TAG_subroutine_type) {
> > /* Function pointer */
> > - strbuf_add(buf, "(function_type)", 15);
> > - return 0;
> > + return strbuf_add(buf, "(function_type)", 15);
> > } else {
> > if (!dwarf_diename(&type))
> > return -ENOENT;
> > @@ -927,14 +926,10 @@ int die_get_typename(Dwarf_Die *vr_die, struct strbuf *buf)
> > else if (tag == DW_TAG_enumeration_type)
> > tmp = "enum ";
> > /* Write a base name */
> > - strbuf_addf(buf, "%s%s", tmp, dwarf_diename(&type));
> > - return 0;
> > + return strbuf_addf(buf, "%s%s", tmp, dwarf_diename(&type));
> > }
> > ret = die_get_typename(&type, buf);
> > - if (ret == 0)
> > - strbuf_addstr(buf, tmp);
> > -
> > - return ret;
> > + return ret ? ret : strbuf_addstr(buf, tmp);
> > }
> >
> > /**
> > @@ -951,12 +946,10 @@ int die_get_varname(Dwarf_Die *vr_die, struct strbuf *buf)
> > ret = die_get_typename(vr_die, buf);
> > if (ret < 0) {
> > pr_debug("Failed to get type, make it unknown.\n");
> > - strbuf_add(buf, " (unknown_type)", 14);
> > + ret = strbuf_add(buf, " (unknown_type)", 14);
> > }
> >
> > - strbuf_addf(buf, "\t%s", dwarf_diename(vr_die));
> > -
> > - return 0;
> > + return ret < 0 ? ret : strbuf_addf(buf, "\t%s", dwarf_diename(vr_die));
> > }
> >
> > #ifdef HAVE_DWARF_GETLOCATIONS
> > @@ -998,23 +991,23 @@ static int die_get_var_innermost_scope(Dwarf_Die *sp_die, Dwarf_Die *vr_die,
> > goto out;
> > }
> >
> > - while ((offset = dwarf_ranges(&scopes[1], offset, &base,
> > - &start, &end)) > 0) {
> > + while (!ret && (offset = dwarf_ranges(&scopes[1], offset, &base,
> > + &start, &end)) > 0) {
>
> Why not leave the above as is, i.e. no checking for ret that will always
> fail the first time, and...
>
> > start -= entry;
> > end -= entry;
> >
> > if (first) {
> > - strbuf_addf(buf, "@<%s+[%" PRIu64 "-%" PRIu64,
> > - name, start, end);
> > + ret = strbuf_addf(buf, "@<%s+[%" PRIu64 "-%" PRIu64,
> > + name, start, end);
> > first = false;
> > } else {
> > - strbuf_addf(buf, ",%" PRIu64 "-%" PRIu64,
> > - start, end);
> > + ret = strbuf_addf(buf, ",%" PRIu64 "-%" PRIu64,
> > + start, end);
> > }
>
> Add:
>
> if (ret)
> goto out;
>
> Here?
I just reused the while() condition :)
>
> > }
> >
> > - if (!first)
> > - strbuf_add(buf, "]>", 2);
> > + if (!ret && !first)
> > + ret = strbuf_add(buf, "]>", 2);
>
> No need for the above hunk? ditto for the following set of hunks.
Yeah, just for avoiding goto. But agreed, it seems not straightforward.
I'll fix that. (btw, anyway we still have to check the retval of the
last strbuf__add())
>
> > out:
> > free(scopes);
> > @@ -1054,9 +1047,8 @@ int die_get_var_range(Dwarf_Die *sp_die, Dwarf_Die *vr_die, struct strbuf *buf)
> > if (dwarf_attr(vr_die, DW_AT_location, &attr) == NULL)
> > return -EINVAL;
> >
> > - while ((offset = dwarf_getlocations(
> > - &attr, offset, &base,
> > - &start, &end, &op, &nops)) > 0) {
> > + while (!ret && (offset = dwarf_getlocations(&attr, offset, &base,
> > + &start, &end, &op, &nops)) > 0) {
> > if (start == 0) {
> > /* Single Location Descriptions */
> > ret = die_get_var_innermost_scope(sp_die, vr_die, buf);
> > @@ -1067,17 +1059,17 @@ int die_get_var_range(Dwarf_Die *sp_die, Dwarf_Die *vr_die, struct strbuf *buf)
> > start -= entry;
> > end -= entry;
> > if (first) {
> > - strbuf_addf(buf, "@<%s+[%" PRIu64 "-%" PRIu64,
> > - name, start, end);
> > + ret = strbuf_addf(buf, "@<%s+[%" PRIu64 "-%" PRIu64,
> > + name, start, end);
> > first = false;
> > } else {
> > - strbuf_addf(buf, ",%" PRIu64 "-%" PRIu64,
> > - start, end);
> > + ret = strbuf_addf(buf, ",%" PRIu64 "-%" PRIu64,
> > + start, end);
> > }
> > }
> >
> > - if (!first)
> > - strbuf_add(buf, "]>", 2);
> > + if (!ret && !first)
> > + ret = strbuf_add(buf, "]>", 2);
fix this too.
> >
> > return ret;
> > }
> > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > index 85d82f4..ff9c5c1 100644
> > --- a/tools/perf/util/probe-event.c
> > +++ b/tools/perf/util/probe-event.c
> > @@ -1677,28 +1677,36 @@ char *synthesize_perf_probe_arg(struct perf_probe_arg *pa)
> > {
> > struct perf_probe_arg_field *field = pa->field;
> > struct strbuf buf;
> > - char *ret;
> > + char *ret = NULL;
> > + int err;
> > +
> > + if (strbuf_init(&buf, 64) < 0)
> > + return NULL;
> >
> > - strbuf_init(&buf, 64);
> > if (pa->name && pa->var)
> > - strbuf_addf(&buf, "%s=%s", pa->name, pa->var);
> > + err = strbuf_addf(&buf, "%s=%s", pa->name, pa->var);
> > else
> > - strbuf_addstr(&buf, pa->name ?: pa->var);
> > -
> > - while (field) {
> > + err = strbuf_addstr(&buf, pa->name ?: pa->var);
> > + if (err)
> > + goto out;
>
> Why test err twice? Leave the 'while (field)' and...
>
> > + while (field && !err) {
> > if (field->name[0] == '[')
> > - strbuf_addstr(&buf, field->name);
> > + err = strbuf_addstr(&buf, field->name);
> > else
> > - strbuf_addf(&buf, "%s%s", field->ref ? "->" : ".",
> > - field->name);
> > + err = strbuf_addf(&buf, "%s%s", field->ref ? "->" : ".",
> > + field->name);
> > field = field->next;
>
> if (err)
> goto out;
> > }
>
> Move the following check + goto to inside the loop, as the last thing,
> otherwise you'll be testing it twice for no reason.
OK, I'll fix this.
>
> > + if (err)
> > + goto out;
> >
> > if (pa->type)
> > - strbuf_addf(&buf, ":%s", pa->type);
> > + if (strbuf_addf(&buf, ":%s", pa->type) < 0)
> > + goto out;
>
> The bove is wrong, it is not propagating the error, should be:
No need to propagate. This function returns allocated string.
If an error happens it just returns NULL.
>
>
> if (pa->type) {
> ret = strbuf_addf(&buf, ":%s", pa->type);
> if (ret < 0)
> goto out;
> }
>
> >
> > ret = strbuf_detach(&buf, NULL);
> > -
> > +out:
> > + strbuf_release(&buf);
> > return ret;
> > }
> >
> > @@ -1706,18 +1714,23 @@ char *synthesize_perf_probe_arg(struct perf_probe_arg *pa)
> > static char *synthesize_perf_probe_point(struct perf_probe_point *pp)
> > {
> > struct strbuf buf;
> > - char *tmp;
> > - int len;
> > + char *tmp, *ret = NULL;
> > + int len, err = 0;
> > +
> > + if (strbuf_init(&buf, 64) < 0)
> > + return NULL;
> >
> > - strbuf_init(&buf, 64);
> > if (pp->function) {
> > - strbuf_addstr(&buf, pp->function);
> > + if (strbuf_addstr(&buf, pp->function) < 0)
> > + goto out;
> > if (pp->offset)
> > - strbuf_addf(&buf, "+%lu", pp->offset);
> > + err = strbuf_addf(&buf, "+%lu", pp->offset);
> > else if (pp->line)
> > - strbuf_addf(&buf, ":%d", pp->line);
> > + err = strbuf_addf(&buf, ":%d", pp->line);
> > else if (pp->retprobe)
> > - strbuf_addstr(&buf, "%return");
> > + err = strbuf_addstr(&buf, "%return");
> > + if (err)
> > + goto out;
> > }
> > if (pp->file) {
> > tmp = pp->file;
> > @@ -1726,12 +1739,15 @@ static char *synthesize_perf_probe_point(struct perf_probe_point *pp)
> > tmp = strchr(pp->file + len - 30, '/');
> > tmp = tmp ? tmp + 1 : pp->file + len - 30;
> > }
> > - strbuf_addf(&buf, "@%s", tmp);
> > - if (!pp->function && pp->line)
> > - strbuf_addf(&buf, ":%d", pp->line);
> > + err = strbuf_addf(&buf, "@%s", tmp);
> > + if (!err && !pp->function && pp->line)
> > + err = strbuf_addf(&buf, ":%d", pp->line);
> > }
> > -
> > - return strbuf_detach(&buf, NULL);
> > + if (!err)
> > + ret = strbuf_detach(&buf, NULL);
> > +out:
> > + strbuf_release(&buf);
> > + return ret;
> > }
> >
> > #if 0
> > @@ -1762,28 +1778,30 @@ char *synthesize_perf_probe_command(struct perf_probe_event *pev)
> > static int __synthesize_probe_trace_arg_ref(struct probe_trace_arg_ref *ref,
> > struct strbuf *buf, int depth)
> > {
> > + int err;
> > if (ref->next) {
> > depth = __synthesize_probe_trace_arg_ref(ref->next, buf,
> > depth + 1);
> > if (depth < 0)
> > - goto out;
> > + return depth;
> > }
> > - strbuf_addf(buf, "%+ld(", ref->offset);
> > -out:
> > - return depth;
> > + err = strbuf_addf(buf, "%+ld(", ref->offset);
> > + return (err < 0) ? err : depth;
> > }
>
> The above would be more compact as:
>
> return err ?: depth;
>
> And would at a glance state that the return of addf() is 0 for Ok and -1
> for error, which I checked and is the case, minor detail tho. :-)
No, strbuf_addf returns the retval of strbuf_addv, which finally returns
strbuf_setlen() for OK. :(
>
> >
> > static int synthesize_probe_trace_arg(struct probe_trace_arg *arg,
> > struct strbuf *buf)
> > {
> > struct probe_trace_arg_ref *ref = arg->ref;
> > - int depth = 0;
> > + int depth = 0, err;
> >
> > /* Argument name or separator */
> > if (arg->name)
> > - strbuf_addf(buf, " %s=", arg->name);
> > + err = strbuf_addf(buf, " %s=", arg->name);
> > else
> > - strbuf_addch(buf, ' ');
> > + err = strbuf_addch(buf, ' ');
> > + if (err)
> > + return err;
> >
> > /* Special case: @XXX */
> > if (arg->value[0] == '@' && arg->ref)
> > @@ -1798,18 +1816,19 @@ static int synthesize_probe_trace_arg(struct probe_trace_arg *arg,
> >
> > /* Print argument value */
> > if (arg->value[0] == '@' && arg->ref)
> > - strbuf_addf(buf, "%s%+ld", arg->value, arg->ref->offset);
> > + err = strbuf_addf(buf, "%s%+ld", arg->value, arg->ref->offset);
> > else
> > - strbuf_addstr(buf, arg->value);
> > + err = strbuf_addstr(buf, arg->value);
>
> Missing check:
>
> if (err)
> return err;
>
> Ahh, ok, caught in the following loop, but then there the !err would be
> better as the first thing, as it checks what comes immediately before
> it, i.e. no sense in decrementing depth if there was an error before
> that loop. Confusing, but harmless.
Yeah, I see. let me fix...
>
> >
> > /* Closing */
> > - while (depth--)
> > - strbuf_addch(buf, ')');
> > + while (depth-- && !err)
> > + err = strbuf_addch(buf, ')');
> > +
> > /* Print argument type */
> > - if (arg->type)
> > - strbuf_addf(buf, ":%s", arg->type);
>
> One more, its more clear as:
>
> if (!err && arg->type)
>
> But harmless...
OK,
>
> > + if (arg->type && !err)
> > + err = strbuf_addf(buf, ":%s", arg->type);
> >
> > - return 0;
> > + return err;
> > }
> >
> > char *synthesize_probe_trace_command(struct probe_trace_event *tev)
> > @@ -1817,15 +1836,18 @@ char *synthesize_probe_trace_command(struct probe_trace_event *tev)
> > struct probe_trace_point *tp = &tev->point;
> > struct strbuf buf;
> > char *ret = NULL;
> > - int i;
> > + int i, err;
> >
> > /* Uprobes must have tp->module */
> > if (tev->uprobes && !tp->module)
> > return NULL;
> >
> > - strbuf_init(&buf, 32);
> > - strbuf_addf(&buf, "%c:%s/%s ", tp->retprobe ? 'r' : 'p',
> > - tev->group, tev->event);
> > + if (strbuf_init(&buf, 32) < 0)
> > + return NULL;
> > +
> > + if (strbuf_addf(&buf, "%c:%s/%s ", tp->retprobe ? 'r' : 'p',
> > + tev->group, tev->event) < 0)
> > + goto error;
> > /*
> > * If tp->address == 0, then this point must be a
> > * absolute address uprobe.
> > @@ -1839,14 +1861,16 @@ char *synthesize_probe_trace_command(struct probe_trace_event *tev)
> >
> > /* Use the tp->address for uprobes */
> > if (tev->uprobes)
> > - strbuf_addf(&buf, "%s:0x%lx", tp->module, tp->address);
> > + err = strbuf_addf(&buf, "%s:0x%lx", tp->module, tp->address);
> > else if (!strncmp(tp->symbol, "0x", 2))
> > /* Absolute address. See try_to_find_absolute_address() */
> > - strbuf_addf(&buf, "%s%s0x%lx", tp->module ?: "",
> > - tp->module ? ":" : "", tp->address);
> > + err = strbuf_addf(&buf, "%s%s0x%lx", tp->module ?: "",
> > + tp->module ? ":" : "", tp->address);
> > else
> > - strbuf_addf(&buf, "%s%s%s+%lu", tp->module ?: "",
> > - tp->module ? ":" : "", tp->symbol, tp->offset);
> > + err = strbuf_addf(&buf, "%s%s%s+%lu", tp->module ?: "",
> > + tp->module ? ":" : "", tp->symbol, tp->offset);
> > + if (err)
> > + goto error;
>
> Good! I thought you would do it as:
>
> for (i = 0; !err && i < tev->nargs; i++)
>
> ;-) :-)
I might finally have changed my mind :)
>
> >
> > for (i = 0; i < tev->nargs; i++)
> > if (synthesize_probe_trace_arg(&tev->args[i], &buf) < 0)
> > @@ -1960,14 +1984,15 @@ static int convert_to_perf_probe_event(struct probe_trace_event *tev,
> > if (tev->args[i].name)
> > pev->args[i].name = strdup(tev->args[i].name);
> > else {
> > - strbuf_init(&buf, 32);
> > + if ((ret = strbuf_init(&buf, 32)) < 0)
> > + goto error;
> > ret = synthesize_probe_trace_arg(&tev->args[i], &buf);
> > pev->args[i].name = strbuf_detach(&buf, NULL);
> > }
> > if (pev->args[i].name == NULL && ret >= 0)
> > ret = -ENOMEM;
> > }
> > -
> > +error:
> > if (ret < 0)
> > clear_perf_probe_event(pev);
> >
> > @@ -2140,37 +2165,40 @@ static int perf_probe_event__sprintf(const char *group, const char *event,
> > const char *module,
> > struct strbuf *result)
> > {
> > - int i;
> > + int i, ret;
> > char *buf;
> >
> > if (asprintf(&buf, "%s:%s", group, event) < 0)
> > return -errno;
> > - strbuf_addf(result, " %-20s (on ", buf);
> > + ret = strbuf_addf(result, " %-20s (on ", buf);
> > free(buf);
> > + if (ret)
> > + return ret;
> >
> > /* Synthesize only event probe point */
> > buf = synthesize_perf_probe_point(&pev->point);
> > if (!buf)
> > return -ENOMEM;
> > - strbuf_addstr(result, buf);
> > + ret = strbuf_addstr(result, buf);
> > free(buf);
> >
> > - if (module)
> > - strbuf_addf(result, " in %s", module);
>
> the next ones match what I suggested elsewhere: first check the result
> of the previous step before reusing the existing if to do something,
> as a way of saving source code lines, ok.
Agreed.
Thank you!
>
> > + if (!ret && module)
> > + ret = strbuf_addf(result, " in %s", module);
> >
> > - if (pev->nargs > 0) {
> > - strbuf_add(result, " with", 5);
> > - for (i = 0; i < pev->nargs; i++) {
> > + if (!ret && pev->nargs > 0) {
> > + ret = strbuf_add(result, " with", 5);
> > + for (i = 0; !ret && i < pev->nargs; i++) {
> > buf = synthesize_perf_probe_arg(&pev->args[i]);
> > if (!buf)
> > return -ENOMEM;
> > - strbuf_addf(result, " %s", buf);
> > + ret = strbuf_addf(result, " %s", buf);
> > free(buf);
> > }
> > }
> > - strbuf_addch(result, ')');
> > + if (!ret)
> > + ret = strbuf_addch(result, ')');
> >
> > - return 0;
> > + return ret;
> > }
> >
> > /* Show an event */
> > diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> > index 9f68875..1259839 100644
> > --- a/tools/perf/util/probe-finder.c
> > +++ b/tools/perf/util/probe-finder.c
> > @@ -1294,6 +1294,7 @@ static int collect_variables_cb(Dwarf_Die *die_mem, void *data)
> > {
> > struct available_var_finder *af = data;
> > struct variable_list *vl;
> > + struct strbuf buf = STRBUF_INIT;
> > int tag, ret;
> >
> > vl = &af->vls[af->nvls - 1];
> > @@ -1307,25 +1308,26 @@ static int collect_variables_cb(Dwarf_Die *die_mem, void *data)
> > if (ret == 0 || ret == -ERANGE) {
> > int ret2;
> > bool externs = !af->child;
> > - struct strbuf buf;
> >
> > - strbuf_init(&buf, 64);
> > + if (strbuf_init(&buf, 64) < 0)
> > + goto error;
> >
> > if (probe_conf.show_location_range) {
> > - if (!externs) {
> > - if (ret)
> > - strbuf_add(&buf, "[INV]\t", 6);
> > - else
> > - strbuf_add(&buf, "[VAL]\t", 6);
> > - } else
> > - strbuf_add(&buf, "[EXT]\t", 6);
> > + if (!externs)
> > + ret2 = strbuf_add(&buf,
> > + ret ? "[INV]\t" : "[VAL]\t", 6);
> > + else
> > + ret2 = strbuf_add(&buf, "[EXT]\t", 6);
> > + if (ret2)
> > + goto error;
> > }
> >
> > ret2 = die_get_varname(die_mem, &buf);
> >
> > if (!ret2 && probe_conf.show_location_range &&
> > !externs) {
> > - strbuf_addch(&buf, '\t');
> > + if (strbuf_addch(&buf, '\t') < 0)
> > + goto error;
> > ret2 = die_get_var_range(&af->pf.sp_die,
> > die_mem, &buf);
> > }
> > @@ -1334,8 +1336,8 @@ static int collect_variables_cb(Dwarf_Die *die_mem, void *data)
> > if (ret2 == 0) {
> > strlist__add(vl->vars,
> > strbuf_detach(&buf, NULL));
> > - } else
> > - strbuf_release(&buf);
> > + }
> > + strbuf_release(&buf);
> > }
> > }
> >
> > @@ -1343,6 +1345,10 @@ static int collect_variables_cb(Dwarf_Die *die_mem, void *data)
> > return DIE_FIND_CB_CONTINUE;
> > else
> > return DIE_FIND_CB_SIBLING;
> > +error:
> > + strbuf_release(&buf);
> > + pr_debug("Error in strbuf\n");
> > + return DIE_FIND_CB_END;
> > }
> >
> > /* Add a found vars into available variables list */
--
Masami Hiramatsu <mhiramat@xxxxxxxxxx>