linux-kernel - Re: [PATCH] perf symbols: Follow .gnu_debuglink section to find separate symbols

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Thu, 21 Jun 2012 14:28:59 -0300
From:	Arnaldo Carvalho de Melo <acme@...stprotocols.net>
To:	"Pierre-Loup A. Griffais" <pgriffais@...dia.com>
Cc:	linux-kernel@...r.kernel.org, a.p.zijlstra@...llo.nl,
	paulus@...ba.org, mingo@...hat.com, torvalds@...ux-foundation.org,
	Mike Sartain <mikesart@...vesoftware.com>
Subject: Re: [PATCH] perf symbols: Follow .gnu_debuglink section to find
 separate symbols

Em Wed, Jun 20, 2012 at 03:22:41PM -0700, Pierre-Loup A. Griffais escreveu:
> The .gnu_debuglink section is specified to contain the filename of the
> debug info file, as well as a CRC that can be used to validate it.

> This provides more context:
> http://sourceware.org/gdb/onlinedocs/gdb/Separate-Debug-Files.html
> 
> +++ b/tools/perf/util/symbol.c
> +static int filename__read_debuglink(const char *filename,
> +				    char *path, size_t size)
> +{

Isn't there any other function that opens an ELF file, looks for an
specific session to then read its contents?

Couldn't it be reused here?

> +		case SYMTAB__DEBUGLINK: {
> +			char *last_slash;
> +			strncpy(name, dso->long_name, size);
> +			last_slash = name + dso->long_name_len;
> +			while (last_slash && *last_slash != '/')
> +				last_slash--;

Why the test for last_slash to be != NULL? How could it ever be? This is
an optimization since we have the dso->long_name_len so that we avoid
using strrchr that in turn would do an strlen?

So the test should be:

			while (last_slash != name && *last_slash != '/')

To avoid underflowing, right?

> +			if (last_slash)
> +				last_slash++;
> +			filename__read_debuglink(dso->long_name, last_slash,

How last_slash can point to the path? It looks like it points to the
basename, no?

Yeah, it is, and then your algorithm will work because last_slash
doesn't point to the _path_, but to a string _preceded_ by the path, so
for /bin/ls, the debuglink content would be ls.debug and that is what
will be stashed there, ending up with:

name = "/bin/\0"
last_slash---^
name = "/bin/ls.debug\0"

I.e. its not really the last slash, but where the debuglink content has
to be stashed, concatenating with the same dirname as the associated
binary.

Can you please rename "last_slash" to "debuglink"? And "path" to
"debuglink" as well in the routine that reads the debuglink.

> +						 size - (last_slash - name));
> +			}

Other than that, thanks a lot for working on this, surely we have to
support this feature!

- ARnaldo
--
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/