From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 67C9FC433E0 for ; Thu, 24 Dec 2020 13:52:40 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 197A522287 for ; Thu, 24 Dec 2020 13:52:39 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 197A522287 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=3DdI8p9A91W9lvYtR03eKThiAlu/OqY3g3PItypM748=; b=c1RN94SVanMlMhncArlXwAqx6 ftkYBBtonJ5cdt2e5bvcRbqWTX5NCKuT7MT3bgL9WExMFTTdX//67Bgc8pDZYDYYfssejXI88NOCf 3w5/QlXq6Q2auOEHRCVfTREb1PjAOaEvpKxTx2Rsz3pZ2Z1Pb6PGBjxLfn9X2TotMBQ0CM1TGg+mf 5Xv/d0pyDsE+kM63o8aiOTNDJY5h1z3TnBluYIbtwu0Nxhu3ROISXW2VSzFqMK5aGGecP5zlYBbVB 1TwQBBA8XXotvBFVrP6tRZe3DeS7EG3g4laGjnTUkV3/ig6Xcj1HpoFVaH8nGWWfrQqAxujVxZBfB Sz+SljOHg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1ksR1a-0001h5-Lv; Thu, 24 Dec 2020 13:51:26 +0000 Received: from [179.97.37.151] (helo=quaco.ghostprotocols.net) by merlin.infradead.org with esmtpsa (Exim 4.92.3 #3 (Red Hat Linux)) id 1ksR1Y-0001gx-AE; Thu, 24 Dec 2020 13:51:24 +0000 Received: by quaco.ghostprotocols.net (Postfix, from userid 1000) id 5E20C411E9; Thu, 24 Dec 2020 10:51:39 -0300 (-03) Date: Thu, 24 Dec 2020 10:51:39 -0300 From: Arnaldo Carvalho de Melo To: Leo Yan Subject: Re: [PATCH v1 1/2] perf probe: Fixup Arm64 SDT arguments Message-ID: <20201224135139.GF477817@kernel.org> References: <20201223063905.25784-1-leo.yan@linaro.org> <20201223063905.25784-2-leo.yan@linaro.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20201223063905.25784-2-leo.yan@linaro.org> X-Url: http://acmel.wordpress.com X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , Ian Rogers , He Zhe , Mathieu Poirier , Peter Zijlstra , Jiri Olsa , John Garry , linux-kernel@vger.kernel.org, Alexander Shishkin , Alexandre Truong , Ingo Molnar , Masami Hiramatsu , Namhyung Kim , Sumanth Korikkar , Will Deacon , linux-arm-kernel@lists.infradead.org, Thomas Richter Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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 > --- > 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: _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel