Re: [PATCH 4/4] io: Reorganize the getcwd implementation

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 26/08/2020 18:02, Adhemerval Zanella wrote:
> The generic implementation uses two internal symbols: __getcwd_system
> (which might be overriden by the system) and __getcwd_generic (the
> generic implementation shared with gnulib).  The Linux implementation
> is moved to __getcwd_system and generic POSIX implementation is moved
> to __getcwd_generic.
> 
> This change aims to make the code sync with gnulib easier and simplify
> the Linux override implementation.
> 
> The dl-fxstatat64 is not required anymore and adding it explicit issue
> a duplicate symbol in libc.so linking.
> 
> Hurd still overrides the getcwd altogether and one possibility would
> to be move its implementation to __getcwd_system and reimplement the
> __getcwd_generic to be a empty one.
> 
> Checked on x86_64-linux-gnu and i686-linux-gnu.

Hi Florian,

With this patch above applied over this set I remove the generic
implementation from rtld.  It allowed some code simplification and the
resulting ld.so size change from:

$ size elf/ld.so
   text    data     bss     dec     hex filename
 164592    7304     392  172288   2a100 elf/ld.so

To

$ size elf/ld.so
   text    data     bss     dec     hex filename
 162222    7304     392  169918   297be elf/ld.so

It also has de advantage of not pulling the generic implementation on
hurd build (which does not use it anyway).

---

diff --git a/elf/dl-object.c b/elf/dl-object.c
index d2cdf135cc..aab7265717 100644
--- a/elf/dl-object.c
+++ b/elf/dl-object.c
@@ -202,30 +202,12 @@ _dl_new_object (char *realname, const char *libname, int type,
 	}
       else
 	{
-	  size_t len = realname_len;
-	  char *result = NULL;
-
-	  /* Get the current directory name.  */
-	  origin = NULL;
-	  do
-	    {
-	      char *new_origin;
-
-	      len += 128;
-	      new_origin = (char *) realloc (origin, len);
-	      if (new_origin == NULL)
-		/* We exit the loop.  Note that result == NULL.  */
-		break;
-	      origin = new_origin;
-	    }
-	  while ((result = __getcwd (origin, len - realname_len)) == NULL
-		 && errno == ERANGE);
-
-	  if (result == NULL)
+	  /* The rtld __getcwd implementation does not handle paths larger
+	     than PATH_MAX (which would be invalid to be used on subsequent
+	     open calls).  */
+	  origin = __getcwd (NULL, 0);
+	  if (origin == NULL)
 	    {
-	      /* We were not able to determine the current directory.
-		 Note that free(origin) is OK if origin == NULL.  */
-	      free (origin);
 	      origin = (char *) -1;
 	      goto out;
 	    }
diff --git a/io/Makefile b/io/Makefile
index 26dfe047c0..57cb778790 100644
--- a/io/Makefile
+++ b/io/Makefile
@@ -46,7 +46,7 @@ routines :=								\
 	close dup dup2 dup3 pipe pipe2					\
 	creat creat64							\
 	chdir fchdir							\
-	getcwd getwd getcwd-system getcwd-generic getdirname		\
+	getcwd getwd getcwd-system getdirname				\
 	chown fchown lchown fchownat					\
 	ttyname ttyname_r isatty					\
 	link linkat symlink symlinkat readlink readlinkat		\
diff --git a/io/getcwd.c b/io/getcwd.c
index cf7a8e1a30..574f51085b 100644
--- a/io/getcwd.c
+++ b/io/getcwd.c
@@ -19,6 +19,10 @@
 #include <unistd.h>
 #include <stddef.h>
 
+#if !IS_IN(rtld)
+#include <getcwd-generic.c>
+#endif
+
 /* Get the pathname of the current working directory,
    and put it in SIZE bytes of BUF.  Returns NULL if the
    directory couldn't be determined or SIZE was too small.
@@ -30,8 +34,10 @@ char *
 __getcwd (char *buf, size_t size)
 {
   char *r = __getcwd_system (buf, size);
+#if !IS_IN(rtld)
   if (r == NULL)
     r = __getcwd_generic (buf, size);
+#endif
   return r;
 }
 libc_hidden_def (__getcwd)
diff --git a/sysdeps/unix/sysv/linux/getcwd-system.c b/sysdeps/unix/sysv/linux/getcwd-system.c
index a7e8535b72..8526b1465b 100644
--- a/sysdeps/unix/sysv/linux/getcwd-system.c
+++ b/sysdeps/unix/sysv/linux/getcwd-system.c
@@ -20,18 +20,11 @@
 #include <unistd.h>
 #include <sysdep.h>
 
-/* If we compile the file for use in ld.so we don't need the feature
-   that getcwd() allocates the buffers itself.  */
-#if IS_IN (rtld)
-# define NO_ALLOCATION	1
-#endif
-
 char *
 __getcwd_system (char *buf, size_t size)
 {
   char *path;
 
-#ifndef NO_ALLOCATION
   size_t alloc_size = size;
   if (size == 0)
     {
@@ -51,9 +44,6 @@ __getcwd_system (char *buf, size_t size)
 	return NULL;
     }
   else
-#else
-# define alloc_size size
-#endif
     path = buf;
 
   int retval;
@@ -61,7 +51,6 @@ __getcwd_system (char *buf, size_t size)
   retval = INLINE_SYSCALL_CALL (getcwd, path, alloc_size);
   if (retval > 0 && path[0] == '/')
     {
-#ifndef NO_ALLOCATION
       if (buf == NULL && size == 0)
 	/* Ensure that the buffer is only as large as necessary.  */
 	buf = realloc (path, (size_t) retval);
@@ -70,15 +59,12 @@ __getcwd_system (char *buf, size_t size)
 	/* Either buf was NULL all along, or `realloc' failed but
 	   we still have the original string.  */
 	buf = path;
-#endif
 
       return buf;
     }
 
-#ifndef NO_ALLOCATION
   if (buf == NULL)
     free (path);
-#endif
 
   return NULL;
 }