linux-kernel - Re: [PATCH] perf tools: fix bug in usage of the basename() function

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20131205085912.GB13377@gmail.com>
Date:	Thu, 5 Dec 2013 09:59:12 +0100
From:	Ingo Molnar <mingo@...nel.org>
To:	Stephane Eranian <eranian@...gle.com>
Cc:	linux-kernel@...r.kernel.org, acme@...hat.com, mingo@...e.hu,
	peterz@...radead.org, jolsa@...hat.com, bccheng@...gle.com,
	dsahern@...il.com
Subject: Re: [PATCH] perf tools: fix bug in usage of the basename() function


* Stephane Eranian <eranian@...gle.com> wrote:

> 
> The basename() implementation varies a lot between systems.
> The Linux man page says: "basename may modify the content of the path,
> so it may be desirable to pass a copy when calling the function".
> On some other systems, the returned address may come from an internal
> buffer which can be reused in subsequent calls, thus the results should
> also be copied.
> 
> The dso__set_basename() function was not doing this causing problems
> on some systems with wrong library names being shown by perf report,
> such as on Android systems.
> 
> This patch fixes the problem.
> Thanks to Ben Cheng for tracking down the problem.
> 
> Patch relative to tip.git at commit 631d5ea.
> 
> Reported-by: Ben Cheng <bccheng@...gle.com>
> Signed-off-by: Stephane Eranian <eranian@...gle.com>
> ---

Just three nits:

>  tools/perf/util/dso.c |   29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index af4c687c..d186ace 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -404,7 +404,34 @@ void dso__set_short_name(struct dso *dso, const char *name)
>  
>  static void dso__set_basename(struct dso *dso)
>  {
> -	dso__set_short_name(dso, basename(dso->long_name));
> +	char *lname, *base;
> +
> +	/*
> +	 * basename may modify path buffer, so we must pass
> +	 * a copy.

s/basename may modify path buffer
 /basename() may modify the path buffer

> +	 */
> +	lname = strdup(dso->long_name);
> +	if (!lname)
> +		return;
> +
> +	/*
> +	 * basename may return pointer to internal
> +	 * storage which is reused in subsequent calls
> +	 * so copy the result
> +	 */

s/basename may return pointer
 /basename() may return a pointer

Makes for easier reading.

(Also please use consistent periods - the first comment has a period, 
the second one doesn't. ':' works well too:)


> +	base = strdup(basename(lname));
> +
> +	free(lname);
> +
> +	if (!base)
> +		return;
> +
> +	if (dso->sname_alloc)
> +		free((char *)dso->short_name);

That cast is probably not needed.

> +	else
> +		dso->sname_alloc = 1;
> +
> +	dso__set_short_name(dso, base);
>  }
>  
>  int dso__name_len(const struct dso *dso)

Otherwise:

Acked-by: Ingo Molnar <mingo@...nel.org>

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/