[DTrace-devel] [PATCH 06/17] dt_link: finish backing out varint, fixing dt_link-generated object files

[Eugene Loh]

I could almost convince myself I understand this patch, but I get lost 
in the commit message, presumably because I am not sufficiently familiar 
with the issues it discusses.

Simple question:  is there a test that fails due to the reported 
problem?  I would think there should be such a test as part of this patch.

Anyhow, s/becasue/because/.

Also, since varints are kind of history now, maybe the commit message 
should refer to one or more specific patches.  E.g., 29e3f422 "Remove 
the string length prefix".  Otherwise, a new developer might have no 
idea what varints were ever about.

On 8/10/22 18:06, Nick Alcock via DTrace-devel wrote:
> For a while now, linking dtrace -G-generated object files has been
> saying
>
> /usr/bin/ld: .../build/test-triggers--usdt-tst-special.o: error adding symbols: no error
>
> This is more than slightly confusing.  For starters, collect2 is
> corrupting the error message: the actual error is
>
> /usr/bin/ld: .../build/test-triggers--usdt-tst-special.o: error adding symbols: file format not recognized
>
> This is not much more helpful.  The actual cause is in
> elf_link_add_object_symbols:
>
>        name = bfd_elf_string_from_elf_section (abfd, hdr->sh_link,
> 					      isym->st_name);
>
> where the symbol name comes out as NULL.  This is because we are hitting
> a sanity check in bfd_elf_string_from_elf_section:
>
>        /* PR 24273: The string section's contents may have already
> 	 been loaded elsewhere, eg because a corrupt file has the
> 	 string section index in the ELF header pointing at a group
> 	 section.  So be paranoid, and test that the last byte of
> 	 the section is zero.  */
>        if (hdr->sh_size == 0 || hdr->contents[hdr->sh_size - 1] != 0)
> 	return NULL;
>
> This paranoia has won out! We are generating string tables with
> hdr->sh_size one too small, pointing at the last byte of the last
> string.
>
> The cause is somewhat gross.  dt_link.c:process_obj computes the length
> of the new strtab it's constructing by adding all the symbol names to a
> dt_strtab, asking it for its size, subtracting one for the \0 at the
> first byte, and then throwing the strtab away and directly editing
> the strtab in the object file, using the strtab it generated earlier
> only for its size.  Doing things in two stages like this is a bit risky:
> it's riskier yet becasue it's using a dt_strtab, which is not a model of
> an ELF strtab at all but rather of a DTrace string table.  No harm,
> they're the same: except that when varints landed the format of a string
> naturally changed to gain an extra length byte at the start. The size
> computation was changed to subtract two bytes rather than one (itself
> buggy: *every string in the strtab* had an extra byte on the front of
> it, so the wrong number of bytes was being subtracted and the resulting
> sh_size was too large).  But when the varint code was backed out, this
> size computation was not changed back, leading to a sh_size that is one
> byte too low, triggering this failure.
>
> Signed-off-by: Nick Alcock <nick.alcock at oracle.com>
> ---
>   libdtrace/dt_link.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libdtrace/dt_link.c b/libdtrace/dt_link.c
> index 766b97420db3..202dd642aba3 100644
> --- a/libdtrace/dt_link.c
> +++ b/libdtrace/dt_link.c
> @@ -1337,10 +1337,10 @@ process_obj(dtrace_hdl_t *dtp, const char *obj, int *eprobesp)
>   		 */
>   		if (nsym > 0) {
>   			/*
> -			 * The first two bytes of the string table are reserved
> +			 * The first byte of the string table is reserved
>   			 * for the \0 entry.
>   			 */
> -			len = dt_strtab_size(strtab) - 2;
> +			len = dt_strtab_size(strtab) - 1;
>   
>   			assert(len > 0);
>   			assert(dt_strtab_index(strtab, "") == 0);