[v2,4/4] stdlib: Remove lstat usage from realpath [BZ #24970]
Commit Message
The readlink already tells whether the file is a symlink, so there is
no need to call lstat to check it. However for '..' it requires an
extra readlink check if the previous component can be really accessed,
otherwise the next iteration will check a possible valid path and end
early. It should performance-wise acceptable and a gain over lstat,
afaik symlink should not update any inode information.
Checked on x86_64-linux-gnu and i686-linux-gnu.
---
stdlib/canonicalize.c | 52 ++++++++++++++++++++++---------------------
1 file changed, 27 insertions(+), 25 deletions(-)
Comments
* Adhemerval Zanella via Libc-alpha:
> The readlink already tells whether the file is a symlink, so there is
> no need to call lstat to check it. However for '..' it requires an
> extra readlink check if the previous component can be really accessed,
> otherwise the next iteration will check a possible valid path and end
> early. It should performance-wise acceptable and a gain over lstat,
> afaik symlink should not update any inode information.
Should this go into gnulib first?
Thanks,
Florian
On 27/10/2020 12:36, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
>
>> The readlink already tells whether the file is a symlink, so there is
>> no need to call lstat to check it. However for '..' it requires an
>> extra readlink check if the previous component can be really accessed,
>> otherwise the next iteration will check a possible valid path and end
>> early. It should performance-wise acceptable and a gain over lstat,
>> afaik symlink should not update any inode information.
>
> Should this go into gnulib first?
I can send to gnulib, although glibc implementation is not really in
sync with gnulib one's. Also on first patch of this set I explicit
did not sync some gnulib features that I think only clutter the
implementation with code that either won't be used by any system glibc
supports or will be removed in subsequent patches.
Paul, do you think this could be included in gnulib?
On 10/27/20 10:30 AM, Adhemerval Zanella wrote:
> Paul, do you think this could be included in gnulib?
I'd rather keep the two in sync. This should be doable by putting the
Gnulib-specific stuff in "#ifndef _LIBC" areas that shouldn't bother Glibc
maintenance significantly. We've had a reasonable amount of luck doing that in
other library code. I could give a shot at doing this, install the result into
Gnulib, and having you look at the result (we could think of this as being part
of the glibc review process :-).
@@ -55,6 +55,7 @@ __realpath (const char *name, char *resolved)
const char *start, *end, *rpath_limit;
const size_t path_max = PATH_MAX;
int num_links = 0;
+ char buf[PATH_MAX];
char extra_buf[PATH_MAX];
if (name == NULL)
@@ -104,12 +105,6 @@ __realpath (const char *name, char *resolved)
for (end = start; *start; start = end)
{
-#ifdef _LIBC
- struct stat64 st;
-#else
- struct stat st;
-#endif
-
/* Skip sequence of multiple path-separators. */
while (*start == '/')
++start;
@@ -118,12 +113,25 @@ __realpath (const char *name, char *resolved)
for (end = start; *end && *end != '/'; ++end)
/* Nothing. */;
- if (end - start == 0)
- break;
- else if (end - start == 1 && start[0] == '.')
+ if (end - start == 1 && start[0] == '.')
/* nothing */;
else if (end - start == 2 && start[0] == '.' && start[1] == '.')
{
+ ssize_t n;
+
+ if (dest[-1] != '/')
+ *dest++ = '/';
+ *dest = '\0';
+
+ n = __readlink (rpath, buf, path_max - 1);
+ if (n == -1)
+ {
+ if (errno == ENOTDIR && dest[-1] == '/')
+ dest[-1] = '\0';
+ if (errno != EINVAL)
+ goto error;
+ }
+
/* Back up to previous component, ignore if at root already. */
if (dest > rpath + 1)
for (--dest; dest > rpath && dest[-1] != '/'; --dest)
@@ -132,6 +140,7 @@ __realpath (const char *name, char *resolved)
else
{
size_t new_size;
+ ssize_t n;
if (dest[-1] != '/')
*dest++ = '/';
@@ -166,25 +175,23 @@ __realpath (const char *name, char *resolved)
dest = __mempcpy (dest, start, end - start);
*dest = '\0';
- if (__lstat64 (rpath, &st) < 0)
- goto error;
-
- if (S_ISLNK (st.st_mode))
+ n = __readlink (rpath, buf, path_max - 1);
+ if (n < 0)
+ {
+ if (errno == ENOTDIR && dest[-1] == '/')
+ dest[-1] = '\0';
+ if (errno != EINVAL)
+ goto error;
+ }
+ else
{
- char buf[PATH_MAX];
size_t len;
- ssize_t n;
if (++num_links > __eloop_threshold ())
{
__set_errno (ELOOP);
goto error; }
- n = __readlink (rpath, buf, path_max - 1);
- if (n < 0)
- goto error;
- buf[n] = '\0';
-
len = strlen (end);
/* Check that n + len + 1 doesn't overflow and is <= path_max. */
if (n >= SIZE_MAX - len || n + len >= path_max)
@@ -211,11 +218,6 @@ __realpath (const char *name, char *resolved)
continue;
}
}
- else if (!S_ISDIR (st.st_mode) && *end != '\0')
- {
- __set_errno (ENOTDIR);
- goto error;
- }
}
}
if (dest > rpath + 1 && dest[-1] == '/')