[4/4] io: Reorganize the getcwd implementation
Commit Message
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.
---
include/unistd.h | 2 +
io/Makefile | 2 +-
sysdeps/posix/getcwd.c => io/getcwd-generic.c | 7 +--
io/getcwd-system.c | 28 +++++++++
io/getcwd.c | 9 ++-
sysdeps/unix/sysv/linux/Makefile | 3 +-
sysdeps/unix/sysv/linux/alpha/dl-fxstatat64.c | 1 -
sysdeps/unix/sysv/linux/dl-fxstatat64.c | 1 -
sysdeps/unix/sysv/linux/dl-getcwd.c | 1 -
.../sysv/linux/{getcwd.c => getcwd-system.c} | 58 +------------------
.../sysv/linux/sparc/sparc64/dl-fxstatat64.c | 1 -
.../sysv/linux/wordsize-64/dl-fxstatat64.c | 1 -
12 files changed, 40 insertions(+), 74 deletions(-)
rename sysdeps/posix/getcwd.c => io/getcwd-generic.c (99%)
create mode 100644 io/getcwd-system.c
delete mode 100644 sysdeps/unix/sysv/linux/alpha/dl-fxstatat64.c
delete mode 100644 sysdeps/unix/sysv/linux/dl-fxstatat64.c
delete mode 100644 sysdeps/unix/sysv/linux/dl-getcwd.c
rename sysdeps/unix/sysv/linux/{getcwd.c => getcwd-system.c} (53%)
delete mode 100644 sysdeps/unix/sysv/linux/sparc/sparc64/dl-fxstatat64.c
delete mode 100644 sysdeps/unix/sysv/linux/wordsize-64/dl-fxstatat64.c
Comments
On 8/26/20 2:02 PM, Adhemerval Zanella via Libc-alpha wrote:
> +/* Architectur specific getcwd implementation. Generic implementation.
Missing "e" at the end of "Architecture".
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;
}
* Adhemerval Zanella:
> 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).
Thanks for doing the experiment.
> 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)
Right, that's what I had in mind.
> 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;
That part I'm less sure about. I think this could allocate a 64K page
that's never freed? Maybe that's a bit excessive.
Thanks,
Florian
On 27/08/2020 10:21, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> 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).
>
> Thanks for doing the experiment.
>
>> 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)
>
> Right, that's what I had in mind.
>
>> 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;
>
> That part I'm less sure about. I think this could allocate a 64K page
> that's never freed? Maybe that's a bit excessive.
The original patch limits the maximum allocation to PATH_MAX and for
a successful __NR_getcwd syscall the memory will be realloced to fit the
returned size from kernel. So the worst case of returning the maximum
allocation size would just happen if realloc fails.
Another possibility would be to use the the scratch_buffer strategy I used
on my realpath Linux optimization [1]: call __NR_getcwd in a loop and either
return the strdup if scratch_buffer did not allocated memory or the resulting
buffer otherwise. I think once the realpath is up I can send a patch with
this strategy, but it trades some performance (to avoid calling realloc)
with some extra syscalls in case or large paths.
[1] https://sourceware.org/pipermail/libc-alpha/2020-August/116935.html
On 27/08/2020 09:35, Adhemerval Zanella wrote:
> 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;
> }
And this part is obviously wrong since the algorithm will append the realname
on the resulting string afterward. I will send a update version with the
rtld code size optimization, but without this bogus change.
@@ -76,6 +76,8 @@ extern int __lchown (const char *__file, __uid_t __owner,
__gid_t __group);
extern int __chdir (const char *__path) attribute_hidden;
extern int __fchdir (int __fd) attribute_hidden;
+extern char *__getcwd_generic (char *__buf, size_t __size) attribute_hidden;
+extern char *__getcwd_system (char *__buf, size_t __size) attribute_hidden;
extern char *__getcwd (char *__buf, size_t __size);
libc_hidden_proto (__getcwd)
extern int __rmdir (const char *__path) attribute_hidden;
@@ -46,7 +46,7 @@ routines := \
close dup dup2 dup3 pipe pipe2 \
creat creat64 \
chdir fchdir \
- getcwd getwd getdirname \
+ getcwd getwd getcwd-system getcwd-generic getdirname \
chown fchown lchown fchownat \
ttyname ttyname_r isatty \
link linkat symlink symlinkat readlink readlinkat \
similarity index 99%
rename from sysdeps/posix/getcwd.c
rename to io/getcwd-generic.c
@@ -154,7 +154,7 @@ getcwd_nothrow (char *buf, size_t size)
bytes long, unless SIZE == 0, in which case it is as big as necessary. */
char *
-__getcwd (char *buf, size_t size)
+__getcwd_generic (char *buf, size_t size)
{
/* Lengths of big file name components and entire file names, and a
deep level of file name nesting. These numbers are not upper
@@ -486,8 +486,3 @@ __getcwd (char *buf, size_t size)
}
return NULL;
}
-
-#if defined _LIBC && !defined __getcwd
-libc_hidden_def (__getcwd)
-weak_alias (__getcwd, getcwd)
-#endif
new file mode 100644
@@ -0,0 +1,28 @@
+/* Architectur specific getcwd implementation. Generic implementation.
+ Copyright (C) 2020 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+#include <unistd.h>
+
+/* This function is called by the generic 'getcwd' implementation to allow
+ a system to implement if the it provides a faster or simpler way to obtain
+ the current direction (e.g. through a syscall). */
+char *
+__getcwd_system (char *buf, size_t size)
+{
+ return NULL;
+}
@@ -29,11 +29,10 @@
char *
__getcwd (char *buf, size_t size)
{
- __set_errno (ENOSYS);
- return NULL;
+ char *r = __getcwd_system (buf, size);
+ if (r == NULL)
+ r = __getcwd_generic (buf, size);
+ return r;
}
libc_hidden_def (__getcwd)
weak_alias (__getcwd, getcwd)
-
-stub_warning (__getcwd)
-stub_warning (getcwd)
@@ -280,8 +280,7 @@ tests += tst-fallocate tst-fallocate64 tst-o_path-locks
endif
ifeq ($(subdir),elf)
-sysdep-rtld-routines += dl-brk dl-sbrk dl-getcwd dl-openat64 dl-opendir \
- dl-fxstatat64
+sysdep-rtld-routines += dl-brk dl-sbrk dl-openat64 dl-opendir
libof-lddlibc4 = lddlibc4
deleted file mode 100644
@@ -1 +0,0 @@
-#include "fxstatat.c"
deleted file mode 100644
@@ -1 +0,0 @@
-#include <fxstatat64.c>
deleted file mode 100644
@@ -1 +0,0 @@
-#include "getcwd.c"
similarity index 53%
rename from sysdeps/unix/sysv/linux/getcwd.c
rename to sysdeps/unix/sysv/linux/getcwd-system.c
@@ -17,16 +17,8 @@
License along with the GNU C Library; if not, see
<https://www.gnu.org/licenses/>. */
-#include <assert.h>
-#include <errno.h>
-#include <limits.h>
-#include <stdlib.h>
#include <unistd.h>
-#include <sys/param.h>
-
#include <sysdep.h>
-#include <sys/syscall.h>
-
/* If we compile the file for use in ld.so we don't need the feature
that getcwd() allocates the buffers itself. */
@@ -34,19 +26,10 @@
# define NO_ALLOCATION 1
#endif
-
-/* The "proc" filesystem provides an easy method to retrieve the value.
- For each process, the corresponding directory contains a symbolic link
- named `cwd'. Reading the content of this link immediate gives us the
- information. But we have to take care for systems which do not have
- the proc filesystem mounted. Use the POSIX implementation in this case. */
-static char *generic_getcwd (char *buf, size_t size);
-
char *
-__getcwd (char *buf, size_t size)
+__getcwd_system (char *buf, size_t size)
{
char *path;
- char *result;
#ifndef NO_ALLOCATION
size_t alloc_size = size;
@@ -58,7 +41,7 @@ __getcwd (char *buf, size_t size)
return NULL;
}
- alloc_size = MAX (PATH_MAX, __getpagesize ());
+ alloc_size = PATH_MAX;
}
if (buf == NULL)
@@ -75,7 +58,7 @@ __getcwd (char *buf, size_t size)
int retval;
- retval = INLINE_SYSCALL (getcwd, 2, path, alloc_size);
+ retval = INLINE_SYSCALL_CALL (getcwd, path, alloc_size);
if (retval > 0 && path[0] == '/')
{
#ifndef NO_ALLOCATION
@@ -92,34 +75,6 @@ __getcwd (char *buf, size_t size)
return buf;
}
- /* The system call either cannot handle paths longer than a page
- or can succeed without returning an absolute path. Just use the
- generic implementation right away. */
- if (retval >= 0 || errno == ENAMETOOLONG)
- {
-#ifndef NO_ALLOCATION
- if (buf == NULL && size == 0)
- {
- free (path);
- path = NULL;
- }
-#endif
-
- result = generic_getcwd (path, size);
-
-#ifndef NO_ALLOCATION
- if (result == NULL && buf == NULL && size != 0)
- free (path);
-#endif
-
- return result;
- }
-
- /* It should never happen that the `getcwd' syscall failed because
- the buffer is too small if we allocated the buffer ourselves
- large enough. */
- assert (errno != ERANGE || buf != NULL || size != 0);
-
#ifndef NO_ALLOCATION
if (buf == NULL)
free (path);
@@ -127,10 +82,3 @@ __getcwd (char *buf, size_t size)
return NULL;
}
-libc_hidden_def (__getcwd)
-weak_alias (__getcwd, getcwd)
-
-/* Get the code for the generic version. */
-#define GETCWD_RETURN_TYPE static char *
-#define __getcwd generic_getcwd
-#include <sysdeps/posix/getcwd.c>
deleted file mode 100644
@@ -1 +0,0 @@
-#include "fxstatat.c"
deleted file mode 100644
@@ -1 +0,0 @@
-#include "fxstatat.c"