elf/dl-load.c: Remove local_strdup.
diff mbox

Message ID 5449C26B.4070600@redhat.com
State New
Headers show

Commit Message

Carlos O'Donell Oct. 24, 2014, 3:07 a.m. UTC
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.

Comments

Mike Frysinger Oct. 24, 2014, 4:09 a.m. UTC | #1
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
Roland McGrath Oct. 24, 2014, 5:01 p.m. UTC | #2
Please use __strdup explicitly rather than relying on some magic rewriting.
Carlos O'Donell Oct. 24, 2014, 8:34 p.m. UTC | #3
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.
Roland McGrath Oct. 24, 2014, 9:40 p.m. UTC | #4
> 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
Carlos O'Donell Oct. 24, 2014, 11:17 p.m. UTC | #5
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.

Patch
diff mbox

diff --git a/elf/dl-load.c b/elf/dl-load.c
index 9dd40e3..538a00a 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -112,20 +112,6 @@  static const size_t system_dirs_len[] =
   (sizeof (system_dirs_len) / sizeof (system_dirs_len[0]))
 
 
-/* Local version of `strdup' function.  */
-static char *
-local_strdup (const char *s)
-{
-  size_t len = strlen (s) + 1;
-  void *new = malloc (len);
-
-  if (new == NULL)
-    return NULL;
-
-  return (char *) memcpy (new, s, len);
-}
-
-
 static bool
 is_trusted_path (const char *path, size_t len)
 {
@@ -384,7 +370,7 @@  expand_dynamic_string_token (struct link_map *l, const char *s, int is_path)
 
   /* If we do not have to replace anything simply copy the string.  */
   if (__glibc_likely (cnt == 0))
-    return local_strdup (s);
+    return strdup (s);
 
   /* Determine the length of the substituted string.  */
   total = DL_DST_REQUIRED (l, s, strlen (s), cnt);
@@ -593,7 +579,7 @@  decompose_rpath (struct r_search_path_struct *sps,
     }
 
   /* Make a writable copy.  */
-  copy = local_strdup (rpath);
+  copy = strdup (rpath);
   if (copy == NULL)
     {
       errstring = N_("cannot create RUNPATH/RPATH copy");
@@ -2101,7 +2087,7 @@  _dl_map_object (struct link_map *loader, const char *name,
 				    false);
 		  if (__glibc_likely (fd != -1))
 		    {
-		      realname = local_strdup (cached);
+		      realname = strdup (cached);
 		      if (realname == NULL)
 			{
 			  __close (fd);
@@ -2130,7 +2116,7 @@  _dl_map_object (struct link_map *loader, const char *name,
       /* The path may contain dynamic string tokens.  */
       realname = (loader
 		  ? expand_dynamic_string_token (loader, name, 0)
-		  : local_strdup (name));
+		  : strdup (name));
       if (realname == NULL)
 	fd = -1;
       else
@@ -2164,7 +2150,7 @@  _dl_map_object (struct link_map *loader, const char *name,
 	  static const Elf_Symndx dummy_bucket = STN_UNDEF;
 
 	  /* Allocate a new object map.  */
-	  if ((name_copy = local_strdup (name)) == NULL
+	  if ((name_copy = strdup (name)) == NULL
 	      || (l = _dl_new_object (name_copy, name, type, loader,
 				      mode, nsid)) == NULL)
 	    {