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 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=-16.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham 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 999C6C433DB for ; Thu, 24 Dec 2020 13:52:23 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 63CE522287 for ; Thu, 24 Dec 2020 13:52:23 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728735AbgLXNwH (ORCPT ); Thu, 24 Dec 2020 08:52:07 -0500 Received: from mail.kernel.org ([198.145.29.99]:50228 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726611AbgLXNwH (ORCPT ); Thu, 24 Dec 2020 08:52:07 -0500 Received: by mail.kernel.org (Postfix) with ESMTPSA id AAFEF22287; Thu, 24 Dec 2020 13:51:25 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1608817886; bh=wLns9vObxxx0pxYvhSFlfHflWryqLD1ESlAgSO2vQzs=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=R0rk2V/EOWGfxa2SCoYMVf3Or9GON8qDDMmsEYC50MuqFekPdhhubnt/l7KbLITuh Wz0S+lgMlY6x4sE8etFCZDmncYRQDpjf9TBHFYtJ38J4sHl665TPVyGKgu3jrm9leh x6P9vNtWfiBXPxZJi3Y6c/5zQhnGxbPW1pYbtocoRjk7QyWBwc9pyfnt868qu7afM/ ZZrRVFRbxYb8hkRyscgI7/pQOhgFfj35cvsReXYj7qSefUkUxns3LDE7/kjVVud8q0 rgqWsFMNXGw7h6kH5IYtZbSbq+AERbOpUB05zx8ZmRJUlXsiu3UCsY0grv57e32HDM ErNBZQHCs8EdQ== 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 Cc: Will Deacon , John Garry , Mathieu Poirier , Peter Zijlstra , Ingo Molnar , Mark Rutland , Alexander Shishkin , Jiri Olsa , Namhyung Kim , Ian Rogers , Alexandre Truong , Masami Hiramatsu , He Zhe , Thomas Richter , Sumanth Korikkar , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org 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-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201223063905.25784-2-leo.yan@linaro.org> X-Url: http://acmel.wordpress.com Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.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: