elf/dl-load.c: Remove local_strdup.

  This patch removes the apparently superfluous local_strdup.
I can find no reason for it to be here and the reason for
it being there is not documented.

It has been present since 1996-08-15 in a patch that simply
refactored strlen/malloc/memcpy code into local_strdup without
changing anything. Probably a conservative change. Why strdup
was not called is not clear.

I have verified that building dl-load.c uses the __strdup
alias so the loader is self-consistent.

Tested on x86_64 with no regressions.

OK to commit?

2014-10-23  Carlos O'Donell  <carlos@redhat.com>

	* dl-load.c (local_strdup): Remove.
	(expand_dynamic_string_token): Use strdup.
	(decompose_rpath): Likewise.
	(_dl_map_object): Likewise.

---

Cheers,
Carlos.
  

On 23 Oct 2014 23:07, Carlos O'Donell wrote:
> This patch removes the apparently superfluous local_strdup.
> I can find no reason for it to be here and the reason for
> it being there is not documented.
> 
> It has been present since 1996-08-15 in a patch that simply
> refactored strlen/malloc/memcpy code into local_strdup without
> changing anything. Probably a conservative change. Why strdup
> was not called is not clear.
> 
> I have verified that building dl-load.c uses the __strdup
> alias so the loader is self-consistent.

lgtm
-mike
  

Please use __strdup explicitly rather than relying on some magic rewriting.
  

On 10/24/2014 01:01 PM, Roland McGrath wrote:
> Please use __strdup explicitly rather than relying on some magic rewriting.

Just to be clear, the reason, from first principles, that we want
to use __strdup is that we want to make sure this symbol is never
interposed by the user (correctness) and never goes through the PLT
(perfromance)? I get that relying on string/string2.h alias to do
this for me is not the best idea.

I see that __strsep in dl-load.c is called with double underscore,
but why not strlen? Is it because for these routines, which are
always in ld.so, we rely on the ld.so copies of them?

I also thought for a second that a strcpy that went into the PLT
would trigger check-localplt failures, but then I saw ld.so is not
part of that test? Any reason why not?

Cheers,
Carlos.
  

> I see that __strsep in dl-load.c is called with double underscore,
> but why not strlen? Is it because for these routines, which are
> always in ld.so, we rely on the ld.so copies of them?

strlen is part of the most restricted name space (C89).  strsep and strdup
are not.  Remember that this code can also be linked statically (it's part
of libc.a).  In ld.so proper, it doesn't matter concretely because it does
not export these symbols and so will always use its own copies.

> I also thought for a second that a strcpy that went into the PLT
> would trigger check-localplt failures, but then I saw ld.so is not
> part of that test? Any reason why not?

It's a surprise to me too, and I had to check to convince myself it really
wasn't tested somehow.  I don't see any reason it shouldn't be per se.  Add
it to the list and verify nothing goes wrong.  But there are reasons it's
far less likely ever to be a problem.  ld.so has no dependencies and is
built with -z defs, so it can never have undefined references and can never
have PLT entries for anything but its own exported symbols (unless the
linker grows new bugs).  There is a very small set of exported symbols
(elf/Versions) that almost never changes.


Thanks,
Roland
  

On 10/24/2014 05:40 PM, Roland McGrath wrote:
>> I see that __strsep in dl-load.c is called with double underscore,
>> but why not strlen? Is it because for these routines, which are
>> always in ld.so, we rely on the ld.so copies of them?
> 
> strlen is part of the most restricted name space (C89).  strsep and strdup
> are not.  Remember that this code can also be linked statically (it's part
> of libc.a).  In ld.so proper, it doesn't matter concretely because it does
> not export these symbols and so will always use its own copies.

Thanks it's the static case I had forgotten to take into account.

>> I also thought for a second that a strcpy that went into the PLT
>> would trigger check-localplt failures, but then I saw ld.so is not
>> part of that test? Any reason why not?
> 
> It's a surprise to me too, and I had to check to convince myself it really
> wasn't tested somehow.  I don't see any reason it shouldn't be per se.  Add
> it to the list and verify nothing goes wrong.  But there are reasons it's
> far less likely ever to be a problem.  ld.so has no dependencies and is
> built with -z defs, so it can never have undefined references and can never
> have PLT entries for anything but its own exported symbols (unless the
> linker grows new bugs).  There is a very small set of exported symbols
> (elf/Versions) that almost never changes.

I'll do that now and commit as obvious. I can't see anyone objecting to
me touching arch localplt.data files.

Cheers,
Carlos.