[PATCH 0/3] <fd_to_filename.h> improvements

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/3] <fd_to_filename.h> improvements

Florian Weimer-5
This series ports fd_to_filename to Hurd and adds more type safety (to
reduce the risk of buffer overflows).

Florian Weimer (3):
  <fd_to_filename.h>: Add type safety and port to Hurd
  Linux: Port ttyname, ttyname_r to <fd_file_name.h>
  Linux: Port fexecve to <fd_to_filename.h>

 libio/freopen.c                               |   4 +-
 libio/freopen64.c                             |   4 +-
 misc/Makefile                                 |   6 +-
 misc/fd_to_filename.c                         | 104 +++++++++++++++++
 misc/tst-fd_to_filename.c                     | 106 ++++++++++++++++++
 sysdeps/generic/arch-fd_to_filename.h         |  19 ++++
 sysdeps/generic/fd_to_filename.h              |  25 +++--
 sysdeps/mach/hurd/arch-fd_to_filename.h       |  19 ++++
 ...fd_to_filename.h => arch-fd_to_filename.h} |  22 +---
 sysdeps/unix/sysv/linux/fexecve.c             |   6 +-
 sysdeps/unix/sysv/linux/ttyname.c             |   7 +-
 sysdeps/unix/sysv/linux/ttyname_r.c           |   7 +-
 12 files changed, 285 insertions(+), 44 deletions(-)
 create mode 100644 misc/fd_to_filename.c
 create mode 100644 misc/tst-fd_to_filename.c
 create mode 100644 sysdeps/generic/arch-fd_to_filename.h
 create mode 100644 sysdeps/mach/hurd/arch-fd_to_filename.h
 rename sysdeps/unix/sysv/linux/{fd_to_filename.h => arch-fd_to_filename.h} (58%)

--
2.24.1

Reply | Threaded
Open this post in threaded view
|

[PATCH 1/3] <fd_to_filename.h>: Add type safety and port to Hurd

Florian Weimer-5
The new type struct fd_to_filename makes the allocation of the
backing storage explicit.

Hurd uses /dev/fd, not /proc/self/fd.
---
 libio/freopen.c                               |   4 +-
 libio/freopen64.c                             |   4 +-
 misc/Makefile                                 |   6 +-
 misc/fd_to_filename.c                         | 104 +++++++++++++++++
 misc/tst-fd_to_filename.c                     | 105 ++++++++++++++++++
 sysdeps/generic/arch-fd_to_filename.h         |  19 ++++
 sysdeps/generic/fd_to_filename.h              |  25 +++--
 sysdeps/mach/hurd/arch-fd_to_filename.h       |  19 ++++
 ...fd_to_filename.h => arch-fd_to_filename.h} |  22 +---
 9 files changed, 275 insertions(+), 33 deletions(-)
 create mode 100644 misc/fd_to_filename.c
 create mode 100644 misc/tst-fd_to_filename.c
 create mode 100644 sysdeps/generic/arch-fd_to_filename.h
 create mode 100644 sysdeps/mach/hurd/arch-fd_to_filename.h
 rename sysdeps/unix/sysv/linux/{fd_to_filename.h => arch-fd_to_filename.h} (58%)

diff --git a/libio/freopen.c b/libio/freopen.c
index bab3ba204a..884cdb2961 100644
--- a/libio/freopen.c
+++ b/libio/freopen.c
@@ -37,7 +37,7 @@ FILE *
 freopen (const char *filename, const char *mode, FILE *fp)
 {
   FILE *result = NULL;
-  char fdfilename[FD_TO_FILENAME_SIZE];
+  struct fd_to_filename fdfilename;
 
   CHECK_FILE (fp, NULL);
 
@@ -50,7 +50,7 @@ freopen (const char *filename, const char *mode, FILE *fp)
 
   int fd = _IO_fileno (fp);
   const char *gfilename
-    = filename != NULL ? filename : fd_to_filename (fd, fdfilename);
+    = filename != NULL ? filename : __fd_to_filename (fd, &fdfilename);
 
   fp->_flags2 |= _IO_FLAGS2_NOCLOSE;
 #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
diff --git a/libio/freopen64.c b/libio/freopen64.c
index c0ce604e6e..0d2c5264c7 100644
--- a/libio/freopen64.c
+++ b/libio/freopen64.c
@@ -36,7 +36,7 @@ FILE *
 freopen64 (const char *filename, const char *mode, FILE *fp)
 {
   FILE *result = NULL;
-  char fdfilename[FD_TO_FILENAME_SIZE];
+  struct fd_to_filename fdfilename;
 
   CHECK_FILE (fp, NULL);
 
@@ -49,7 +49,7 @@ freopen64 (const char *filename, const char *mode, FILE *fp)
 
   int fd = _IO_fileno (fp);
   const char *gfilename
-    = filename != NULL ? filename : fd_to_filename (fd, fdfilename);
+    = filename != NULL ? filename : __fd_to_filename (fd, &fdfilename);
 
   fp->_flags2 |= _IO_FLAGS2_NOCLOSE;
   _IO_file_close_it (fp);
diff --git a/misc/Makefile b/misc/Makefile
index e0465980c7..b8fed5783d 100644
--- a/misc/Makefile
+++ b/misc/Makefile
@@ -72,7 +72,7 @@ routines := brk sbrk sstk ioctl \
     fgetxattr flistxattr fremovexattr fsetxattr getxattr \
     listxattr lgetxattr llistxattr lremovexattr lsetxattr \
     removexattr setxattr getauxval ifunc-impl-list makedev \
-    allocate_once
+    allocate_once fd_to_filename
 
 generated += tst-error1.mtrace tst-error1-mem.out \
   tst-allocate_once.mtrace tst-allocate_once-mem.out
@@ -97,6 +97,10 @@ endif
 tests-internal := tst-atomic tst-atomic-long tst-allocate_once
 tests-static := tst-empty
 
+# Test for the internal, non-exported __fd_to_filename function.
+tests-internal += tst-fd_to_filename
+tests-static += tst-fd_to_filename
+
 ifeq ($(run-built-tests),yes)
 tests-special += $(objpfx)tst-error1-mem.out \
   $(objpfx)tst-allocate_once-mem.out
diff --git a/misc/fd_to_filename.c b/misc/fd_to_filename.c
new file mode 100644
index 0000000000..911c22cf2b
--- /dev/null
+++ b/misc/fd_to_filename.c
@@ -0,0 +1,104 @@
+/* Construct a pathname under /proc/self/fd (or /dev/fd for Hurd).
+   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 <fd_to_filename.h>
+#include <string.h>
+
+/* The split into groups of three digits is arbitrary, but it covers
+   the common case of descriptors less than 1000 fairly
+   efficiently.  */
+
+/* Writes two digits to P.  VALUE must be between 0 and 99
+   (inclusive).  Returns the address after the written digits.  */
+static char *
+digit2 (char *p, int value)
+{
+  p[0] = '0' + (value / 10);
+  p[1] = '0' + (value % 10);
+  return p + 2;
+}
+
+/* Writes three digits to P.  VALUE must be between 0 and 999
+   (inclusive).  Returns the address after the written digits.  */
+static char *
+digit3 (char *p, int value)
+{
+  p[0] = '0' + (value / 100);
+  p[1] = '0' + ((value / 10) % 10);
+  p[2] = '0' + (value % 10);
+  return p + 3;
+}
+
+/* Writes one to three digits to P, depending on VALUE, which must be
+   less than 1000.  Returns the address after the written digits.  */
+static char *
+digit123 (char *p, int value)
+{
+  if (value < 10)
+    {
+      p[0] = '0' + value;
+      return p + 1;
+    }
+  else if (value < 100)
+    return digit2 (p, value);
+  else
+    return digit3 (p, value);
+}
+
+char *
+__fd_to_filename (int descriptor, struct fd_to_filename *storage)
+{
+  char *p = mempcpy (storage->buffer, FD_TO_FILENAME_PREFIX,
+                     strlen (FD_TO_FILENAME_PREFIX));
+  if (__glibc_likely (descriptor >= 0))
+    {
+      if (descriptor < 1000)
+        p = digit123 (p, descriptor);
+      else if (descriptor < 1000 * 1000)
+        {
+          p = digit123 (p, descriptor / 1000);
+          p = digit3 (p, descriptor % 1000);
+        }
+      else if (descriptor < 1000 * 1000 * 1000)
+        {
+          p = digit123 (p, descriptor / (1000 * 1000));
+          p = digit3 (p, (descriptor / 1000) % 1000);
+          p = digit3 (p, descriptor % 1000);
+        }
+      else
+        {
+          if (descriptor < 2000 * 1000 * 1000)
+            {
+              *p = '1';
+              descriptor -= 1000 * 1000 * 1000;
+            }
+          else
+            {
+              *p = '2';
+              descriptor -= 2000 * 1000 * 1000;
+            }
+          ++p;
+          p = digit3 (p, descriptor / (1000 * 1000));
+          p = digit3 (p, (descriptor / 1000) % 1000);
+          p = digit3 (p, descriptor % 1000);
+        }
+    }
+
+  *p = '\0';
+  return storage->buffer;
+}
diff --git a/misc/tst-fd_to_filename.c b/misc/tst-fd_to_filename.c
new file mode 100644
index 0000000000..480f613dcf
--- /dev/null
+++ b/misc/tst-fd_to_filename.c
@@ -0,0 +1,105 @@
+/* Test for /proc/self/fd (or /dev/fd) pathname construction.
+   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 <fd_to_filename.h>
+#include <stdio.h>
+#include <support/check.h>
+#include <support/xunistd.h>
+
+/* Run a check on one value.  */
+static void
+check (int value)
+{
+  struct fd_to_filename storage;
+  char *actual = __fd_to_filename (value, &storage);
+  char expected[100];
+  snprintf (expected, sizeof (expected), FD_TO_FILENAME_PREFIX "%d", value);
+
+  if (value < 0)
+    /* Negative inputs are invalid, but the output should still be
+       deterministic.  */
+    TEST_COMPARE_STRING (actual, FD_TO_FILENAME_PREFIX);
+  else
+    TEST_COMPARE_STRING (actual, expected);
+}
+
+/* Check various ranges constructed around powers.  */
+static void
+check_ranges (int base)
+{
+  unsigned int power = 1;
+  do
+    {
+      for (int factor = 1; factor < base; ++factor)
+        for (int shift = -1000; shift <= 1000; ++shift)
+          {
+            check (factor * power + shift);
+            check (-(factor * power + shift));
+          }
+    }
+  while (!__builtin_mul_overflow (power, base, &power));
+}
+
+/* Check that it is actually possible to use a the constructed
+   name.  */
+static void
+check_open (void)
+{
+  int pipes[2];
+  xpipe (pipes);
+
+  struct fd_to_filename storage;
+  int read_alias = xopen (__fd_to_filename (pipes[0], &storage), O_RDONLY, 0);
+  int write_alias = xopen (__fd_to_filename (pipes[1], &storage), O_WRONLY, 0);
+
+  /* Ensure that all the descriptor numbers are different.  */
+  TEST_VERIFY (pipes[0] < pipes[1]);
+  TEST_VERIFY (pipes[1] < read_alias);
+  TEST_VERIFY (read_alias < write_alias);
+
+  xwrite (write_alias, "1", 1);
+  char buf[16];
+  TEST_COMPARE_BLOB ("1", 1, buf, read (pipes[0], buf, sizeof (buf)));
+
+  xwrite (pipes[1], "2", 1);
+  TEST_COMPARE_BLOB ("2", 1, buf, read (read_alias, buf, sizeof (buf)));
+
+  xwrite (write_alias, "3", 1);
+  TEST_COMPARE_BLOB ("3", 1, buf, read (read_alias, buf, sizeof (buf)));
+
+  xwrite (pipes[1], "4", 1);
+  TEST_COMPARE_BLOB ("4", 1, buf, read (pipes[0], buf, sizeof (buf)));
+
+  xclose (write_alias);
+  xclose (read_alias);
+  xclose (pipes[1]);
+  xclose (pipes[0]);
+}
+
+static int
+do_test (void)
+{
+  check_ranges (2);
+  check_ranges (10);
+
+  check_open ();
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/sysdeps/generic/arch-fd_to_filename.h b/sysdeps/generic/arch-fd_to_filename.h
new file mode 100644
index 0000000000..ecaaa14dba
--- /dev/null
+++ b/sysdeps/generic/arch-fd_to_filename.h
@@ -0,0 +1,19 @@
+/* Query filename corresponding to an open FD.  Generic stub.
+   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/>.  */
+
+#error "<arch-fd_to_filename.h> must be ported to this architecture"
diff --git a/sysdeps/generic/fd_to_filename.h b/sysdeps/generic/fd_to_filename.h
index eff6ca211b..7889db879c 100644
--- a/sysdeps/generic/fd_to_filename.h
+++ b/sysdeps/generic/fd_to_filename.h
@@ -1,4 +1,4 @@
-/* Query filename corresponding to an open FD.  Generic version.
+/* Query filename corresponding to an open FD.
    Copyright (C) 2001-2020 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
@@ -16,12 +16,21 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
-#define FD_TO_FILENAME_SIZE 0
+#ifndef _FD_TO_FILENAME_H
+#define _FD_TO_FILENAME_H
 
-/* In general there is no generic way to query filename for an open
-   file descriptor.  */
-static inline const char *
-fd_to_filename (int fd, char *buf)
+#include <arch-fd_to_filename.h>
+
+struct fd_to_filename
 {
-  return NULL;
-}
+  /* A positive int value has at most 10 decimal digits.  */
+  char buffer[sizeof (FD_TO_FILENAME_PREFIX) + 10];
+};
+
+/* Writes a /proc/self/fd-style path for DESCRIPTOR to *STORAGE and
+   returns a pointer to the start of the string.  DESCRIPTOR must be
+   non-negative.  */
+char *__fd_to_filename (int descriptor, struct fd_to_filename *storage)
+  attribute_hidden;
+
+#endif /* _FD_TO_FILENAME_H */
diff --git a/sysdeps/mach/hurd/arch-fd_to_filename.h b/sysdeps/mach/hurd/arch-fd_to_filename.h
new file mode 100644
index 0000000000..b45cd8d836
--- /dev/null
+++ b/sysdeps/mach/hurd/arch-fd_to_filename.h
@@ -0,0 +1,19 @@
+/* Query filename corresponding to an open FD.  Hurd version.
+   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/>.  */
+
+#define FD_TO_FILENAME_PREFIX "/dev/fd/"
diff --git a/sysdeps/unix/sysv/linux/fd_to_filename.h b/sysdeps/unix/sysv/linux/arch-fd_to_filename.h
similarity index 58%
rename from sysdeps/unix/sysv/linux/fd_to_filename.h
rename to sysdeps/unix/sysv/linux/arch-fd_to_filename.h
index 92a5e02976..b6017214c7 100644
--- a/sysdeps/unix/sysv/linux/fd_to_filename.h
+++ b/sysdeps/unix/sysv/linux/arch-fd_to_filename.h
@@ -1,5 +1,5 @@
 /* Query filename corresponding to an open FD.  Linux version.
-   Copyright (C) 2001-2020 Free Software Foundation, Inc.
+   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
@@ -16,22 +16,4 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
-#include <sys/stat.h>
-#include <string.h>
-#include <_itoa.h>
-
-#define FD_TO_FILENAME_SIZE ((sizeof ("/proc/self/fd/") - 1) \
-     + (sizeof ("4294967295") - 1) + 1)
-
-static inline const char *
-fd_to_filename (unsigned int fd, char *buf)
-{
-  *_fitoa_word (fd, __stpcpy (buf, "/proc/self/fd/"), 10, 0) = '\0';
-
-  /* We must make sure the file exists.  */
-  struct stat64 st;
-  if (__lxstat64 (_STAT_VER, buf, &st) < 0)
-    /* /proc is not mounted or something else happened.  */
-    return NULL;
-  return buf;
-}
+#define FD_TO_FILENAME_PREFIX "/proc/self/fd/"
--
2.24.1


Reply | Threaded
Open this post in threaded view
|

[PATCH 2/3] Linux: Port ttyname, ttyname_r to <fd_file_name.h>

Florian Weimer-5
In reply to this post by Florian Weimer-5
---
 misc/tst-fd_to_filename.c           | 1 +
 sysdeps/unix/sysv/linux/ttyname.c   | 7 +++----
 sysdeps/unix/sysv/linux/ttyname_r.c | 7 +++----
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/misc/tst-fd_to_filename.c b/misc/tst-fd_to_filename.c
index 480f613dcf..eb08dd7e25 100644
--- a/misc/tst-fd_to_filename.c
+++ b/misc/tst-fd_to_filename.c
@@ -16,6 +16,7 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
+#include <fcntl.h>
 #include <fd_to_filename.h>
 #include <stdio.h>
 #include <support/check.h>
diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c
index c05ca687f5..6034e132b0 100644
--- a/sysdeps/unix/sysv/linux/ttyname.c
+++ b/sysdeps/unix/sysv/linux/ttyname.c
@@ -25,8 +25,7 @@
 #include <unistd.h>
 #include <string.h>
 #include <stdlib.h>
-
-#include <_itoa.h>
+#include <fd_to_filename.h>
 
 #include "ttyname.h"
 
@@ -112,7 +111,6 @@ char *
 ttyname (int fd)
 {
   static size_t buflen;
-  char procname[30];
   struct stat64 st, st1;
   int dostat = 0;
   int doispty = 0;
@@ -129,7 +127,8 @@ ttyname (int fd)
     return NULL;
 
   /* We try using the /proc filesystem.  */
-  *_fitoa_word (fd, __stpcpy (procname, "/proc/self/fd/"), 10, 0) = '\0';
+  struct fd_to_filename procname_storage;
+  char *procname = __fd_to_filename (fd, &procname_storage);
 
   if (buflen == 0)
     {
diff --git a/sysdeps/unix/sysv/linux/ttyname_r.c b/sysdeps/unix/sysv/linux/ttyname_r.c
index 4a1ae40bb2..e16761b43d 100644
--- a/sysdeps/unix/sysv/linux/ttyname_r.c
+++ b/sysdeps/unix/sysv/linux/ttyname_r.c
@@ -25,8 +25,7 @@
 #include <unistd.h>
 #include <string.h>
 #include <stdlib.h>
-
-#include <_itoa.h>
+#include <fd_to_filename.h>
 
 #include "ttyname.h"
 
@@ -92,7 +91,6 @@ getttyname_r (char *buf, size_t buflen, const struct stat64 *mytty,
 int
 __ttyname_r (int fd, char *buf, size_t buflen)
 {
-  char procname[30];
   struct stat64 st, st1;
   int dostat = 0;
   int doispty = 0;
@@ -122,7 +120,8 @@ __ttyname_r (int fd, char *buf, size_t buflen)
     return errno;
 
   /* We try using the /proc filesystem.  */
-  *_fitoa_word (fd, __stpcpy (procname, "/proc/self/fd/"), 10, 0) = '\0';
+  struct fd_to_filename procname_storage;
+  char *procname = __fd_to_filename (fd, &procname_storage);
 
   ssize_t ret = __readlink (procname, buf, buflen - 1);
   if (__glibc_unlikely (ret == -1 && errno == ENAMETOOLONG))
--
2.24.1


Reply | Threaded
Open this post in threaded view
|

[PATCH 3/3] Linux: Port fexecve to <fd_to_filename.h>

Florian Weimer-5
In reply to this post by Florian Weimer-5
---
 sysdeps/unix/sysv/linux/fexecve.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/fexecve.c b/sysdeps/unix/sysv/linux/fexecve.c
index 23c9799f5d..d8cf924faf 100644
--- a/sysdeps/unix/sysv/linux/fexecve.c
+++ b/sysdeps/unix/sysv/linux/fexecve.c
@@ -25,6 +25,7 @@
 #include <sysdep.h>
 #include <sys/syscall.h>
 #include <kernel-features.h>
+#include <fd_to_filename.h>
 
 
 /* Execute the file FD refers to, overlaying the running program image.
@@ -50,11 +51,10 @@ fexecve (int fd, char *const argv[], char *const envp[])
 #ifndef __ASSUME_EXECVEAT
   /* We use the /proc filesystem to get the information.  If it is not
      mounted we fail.  */
-  char buf[sizeof "/proc/self/fd/" + sizeof (int) * 3];
-  __snprintf (buf, sizeof (buf), "/proc/self/fd/%d", fd);
+  struct fd_to_filename storage;
 
   /* We do not need the return value.  */
-  __execve (buf, argv, envp);
+  __execve (__fd_to_filename (fd, &storage), argv, envp);
 
   int save = errno;
 
--
2.24.1

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/3] <fd_to_filename.h>: Add type safety and port to Hurd

Samuel Thibault
In reply to this post by Florian Weimer-5
Florian Weimer, le ven. 14 févr. 2020 19:11:09 +0100, a ecrit:
> The new type struct fd_to_filename makes the allocation of the
> backing storage explicit.
>
> Hurd uses /dev/fd, not /proc/self/fd.

Acked-by: Samuel Thibault <[hidden email]>

Thanks!

> ---
>  libio/freopen.c                               |   4 +-
>  libio/freopen64.c                             |   4 +-
>  misc/Makefile                                 |   6 +-
>  misc/fd_to_filename.c                         | 104 +++++++++++++++++
>  misc/tst-fd_to_filename.c                     | 105 ++++++++++++++++++
>  sysdeps/generic/arch-fd_to_filename.h         |  19 ++++
>  sysdeps/generic/fd_to_filename.h              |  25 +++--
>  sysdeps/mach/hurd/arch-fd_to_filename.h       |  19 ++++
>  ...fd_to_filename.h => arch-fd_to_filename.h} |  22 +---
>  9 files changed, 275 insertions(+), 33 deletions(-)
>  create mode 100644 misc/fd_to_filename.c
>  create mode 100644 misc/tst-fd_to_filename.c
>  create mode 100644 sysdeps/generic/arch-fd_to_filename.h
>  create mode 100644 sysdeps/mach/hurd/arch-fd_to_filename.h
>  rename sysdeps/unix/sysv/linux/{fd_to_filename.h => arch-fd_to_filename.h} (58%)
>
> diff --git a/libio/freopen.c b/libio/freopen.c
> index bab3ba204a..884cdb2961 100644
> --- a/libio/freopen.c
> +++ b/libio/freopen.c
> @@ -37,7 +37,7 @@ FILE *
>  freopen (const char *filename, const char *mode, FILE *fp)
>  {
>    FILE *result = NULL;
> -  char fdfilename[FD_TO_FILENAME_SIZE];
> +  struct fd_to_filename fdfilename;
>  
>    CHECK_FILE (fp, NULL);
>  
> @@ -50,7 +50,7 @@ freopen (const char *filename, const char *mode, FILE *fp)
>  
>    int fd = _IO_fileno (fp);
>    const char *gfilename
> -    = filename != NULL ? filename : fd_to_filename (fd, fdfilename);
> +    = filename != NULL ? filename : __fd_to_filename (fd, &fdfilename);
>  
>    fp->_flags2 |= _IO_FLAGS2_NOCLOSE;
>  #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
> diff --git a/libio/freopen64.c b/libio/freopen64.c
> index c0ce604e6e..0d2c5264c7 100644
> --- a/libio/freopen64.c
> +++ b/libio/freopen64.c
> @@ -36,7 +36,7 @@ FILE *
>  freopen64 (const char *filename, const char *mode, FILE *fp)
>  {
>    FILE *result = NULL;
> -  char fdfilename[FD_TO_FILENAME_SIZE];
> +  struct fd_to_filename fdfilename;
>  
>    CHECK_FILE (fp, NULL);
>  
> @@ -49,7 +49,7 @@ freopen64 (const char *filename, const char *mode, FILE *fp)
>  
>    int fd = _IO_fileno (fp);
>    const char *gfilename
> -    = filename != NULL ? filename : fd_to_filename (fd, fdfilename);
> +    = filename != NULL ? filename : __fd_to_filename (fd, &fdfilename);
>  
>    fp->_flags2 |= _IO_FLAGS2_NOCLOSE;
>    _IO_file_close_it (fp);
> diff --git a/misc/Makefile b/misc/Makefile
> index e0465980c7..b8fed5783d 100644
> --- a/misc/Makefile
> +++ b/misc/Makefile
> @@ -72,7 +72,7 @@ routines := brk sbrk sstk ioctl \
>      fgetxattr flistxattr fremovexattr fsetxattr getxattr \
>      listxattr lgetxattr llistxattr lremovexattr lsetxattr \
>      removexattr setxattr getauxval ifunc-impl-list makedev \
> -    allocate_once
> +    allocate_once fd_to_filename
>  
>  generated += tst-error1.mtrace tst-error1-mem.out \
>    tst-allocate_once.mtrace tst-allocate_once-mem.out
> @@ -97,6 +97,10 @@ endif
>  tests-internal := tst-atomic tst-atomic-long tst-allocate_once
>  tests-static := tst-empty
>  
> +# Test for the internal, non-exported __fd_to_filename function.
> +tests-internal += tst-fd_to_filename
> +tests-static += tst-fd_to_filename
> +
>  ifeq ($(run-built-tests),yes)
>  tests-special += $(objpfx)tst-error1-mem.out \
>    $(objpfx)tst-allocate_once-mem.out
> diff --git a/misc/fd_to_filename.c b/misc/fd_to_filename.c
> new file mode 100644
> index 0000000000..911c22cf2b
> --- /dev/null
> +++ b/misc/fd_to_filename.c
> @@ -0,0 +1,104 @@
> +/* Construct a pathname under /proc/self/fd (or /dev/fd for Hurd).
> +   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 <fd_to_filename.h>
> +#include <string.h>
> +
> +/* The split into groups of three digits is arbitrary, but it covers
> +   the common case of descriptors less than 1000 fairly
> +   efficiently.  */
> +
> +/* Writes two digits to P.  VALUE must be between 0 and 99
> +   (inclusive).  Returns the address after the written digits.  */
> +static char *
> +digit2 (char *p, int value)
> +{
> +  p[0] = '0' + (value / 10);
> +  p[1] = '0' + (value % 10);
> +  return p + 2;
> +}
> +
> +/* Writes three digits to P.  VALUE must be between 0 and 999
> +   (inclusive).  Returns the address after the written digits.  */
> +static char *
> +digit3 (char *p, int value)
> +{
> +  p[0] = '0' + (value / 100);
> +  p[1] = '0' + ((value / 10) % 10);
> +  p[2] = '0' + (value % 10);
> +  return p + 3;
> +}
> +
> +/* Writes one to three digits to P, depending on VALUE, which must be
> +   less than 1000.  Returns the address after the written digits.  */
> +static char *
> +digit123 (char *p, int value)
> +{
> +  if (value < 10)
> +    {
> +      p[0] = '0' + value;
> +      return p + 1;
> +    }
> +  else if (value < 100)
> +    return digit2 (p, value);
> +  else
> +    return digit3 (p, value);
> +}
> +
> +char *
> +__fd_to_filename (int descriptor, struct fd_to_filename *storage)
> +{
> +  char *p = mempcpy (storage->buffer, FD_TO_FILENAME_PREFIX,
> +                     strlen (FD_TO_FILENAME_PREFIX));
> +  if (__glibc_likely (descriptor >= 0))
> +    {
> +      if (descriptor < 1000)
> +        p = digit123 (p, descriptor);
> +      else if (descriptor < 1000 * 1000)
> +        {
> +          p = digit123 (p, descriptor / 1000);
> +          p = digit3 (p, descriptor % 1000);
> +        }
> +      else if (descriptor < 1000 * 1000 * 1000)
> +        {
> +          p = digit123 (p, descriptor / (1000 * 1000));
> +          p = digit3 (p, (descriptor / 1000) % 1000);
> +          p = digit3 (p, descriptor % 1000);
> +        }
> +      else
> +        {
> +          if (descriptor < 2000 * 1000 * 1000)
> +            {
> +              *p = '1';
> +              descriptor -= 1000 * 1000 * 1000;
> +            }
> +          else
> +            {
> +              *p = '2';
> +              descriptor -= 2000 * 1000 * 1000;
> +            }
> +          ++p;
> +          p = digit3 (p, descriptor / (1000 * 1000));
> +          p = digit3 (p, (descriptor / 1000) % 1000);
> +          p = digit3 (p, descriptor % 1000);
> +        }
> +    }
> +
> +  *p = '\0';
> +  return storage->buffer;
> +}
> diff --git a/misc/tst-fd_to_filename.c b/misc/tst-fd_to_filename.c
> new file mode 100644
> index 0000000000..480f613dcf
> --- /dev/null
> +++ b/misc/tst-fd_to_filename.c
> @@ -0,0 +1,105 @@
> +/* Test for /proc/self/fd (or /dev/fd) pathname construction.
> +   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 <fd_to_filename.h>
> +#include <stdio.h>
> +#include <support/check.h>
> +#include <support/xunistd.h>
> +
> +/* Run a check on one value.  */
> +static void
> +check (int value)
> +{
> +  struct fd_to_filename storage;
> +  char *actual = __fd_to_filename (value, &storage);
> +  char expected[100];
> +  snprintf (expected, sizeof (expected), FD_TO_FILENAME_PREFIX "%d", value);
> +
> +  if (value < 0)
> +    /* Negative inputs are invalid, but the output should still be
> +       deterministic.  */
> +    TEST_COMPARE_STRING (actual, FD_TO_FILENAME_PREFIX);
> +  else
> +    TEST_COMPARE_STRING (actual, expected);
> +}
> +
> +/* Check various ranges constructed around powers.  */
> +static void
> +check_ranges (int base)
> +{
> +  unsigned int power = 1;
> +  do
> +    {
> +      for (int factor = 1; factor < base; ++factor)
> +        for (int shift = -1000; shift <= 1000; ++shift)
> +          {
> +            check (factor * power + shift);
> +            check (-(factor * power + shift));
> +          }
> +    }
> +  while (!__builtin_mul_overflow (power, base, &power));
> +}
> +
> +/* Check that it is actually possible to use a the constructed
> +   name.  */
> +static void
> +check_open (void)
> +{
> +  int pipes[2];
> +  xpipe (pipes);
> +
> +  struct fd_to_filename storage;
> +  int read_alias = xopen (__fd_to_filename (pipes[0], &storage), O_RDONLY, 0);
> +  int write_alias = xopen (__fd_to_filename (pipes[1], &storage), O_WRONLY, 0);
> +
> +  /* Ensure that all the descriptor numbers are different.  */
> +  TEST_VERIFY (pipes[0] < pipes[1]);
> +  TEST_VERIFY (pipes[1] < read_alias);
> +  TEST_VERIFY (read_alias < write_alias);
> +
> +  xwrite (write_alias, "1", 1);
> +  char buf[16];
> +  TEST_COMPARE_BLOB ("1", 1, buf, read (pipes[0], buf, sizeof (buf)));
> +
> +  xwrite (pipes[1], "2", 1);
> +  TEST_COMPARE_BLOB ("2", 1, buf, read (read_alias, buf, sizeof (buf)));
> +
> +  xwrite (write_alias, "3", 1);
> +  TEST_COMPARE_BLOB ("3", 1, buf, read (read_alias, buf, sizeof (buf)));
> +
> +  xwrite (pipes[1], "4", 1);
> +  TEST_COMPARE_BLOB ("4", 1, buf, read (pipes[0], buf, sizeof (buf)));
> +
> +  xclose (write_alias);
> +  xclose (read_alias);
> +  xclose (pipes[1]);
> +  xclose (pipes[0]);
> +}
> +
> +static int
> +do_test (void)
> +{
> +  check_ranges (2);
> +  check_ranges (10);
> +
> +  check_open ();
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/sysdeps/generic/arch-fd_to_filename.h b/sysdeps/generic/arch-fd_to_filename.h
> new file mode 100644
> index 0000000000..ecaaa14dba
> --- /dev/null
> +++ b/sysdeps/generic/arch-fd_to_filename.h
> @@ -0,0 +1,19 @@
> +/* Query filename corresponding to an open FD.  Generic stub.
> +   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/>.  */
> +
> +#error "<arch-fd_to_filename.h> must be ported to this architecture"
> diff --git a/sysdeps/generic/fd_to_filename.h b/sysdeps/generic/fd_to_filename.h
> index eff6ca211b..7889db879c 100644
> --- a/sysdeps/generic/fd_to_filename.h
> +++ b/sysdeps/generic/fd_to_filename.h
> @@ -1,4 +1,4 @@
> -/* Query filename corresponding to an open FD.  Generic version.
> +/* Query filename corresponding to an open FD.
>     Copyright (C) 2001-2020 Free Software Foundation, Inc.
>     This file is part of the GNU C Library.
>  
> @@ -16,12 +16,21 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> -#define FD_TO_FILENAME_SIZE 0
> +#ifndef _FD_TO_FILENAME_H
> +#define _FD_TO_FILENAME_H
>  
> -/* In general there is no generic way to query filename for an open
> -   file descriptor.  */
> -static inline const char *
> -fd_to_filename (int fd, char *buf)
> +#include <arch-fd_to_filename.h>
> +
> +struct fd_to_filename
>  {
> -  return NULL;
> -}
> +  /* A positive int value has at most 10 decimal digits.  */
> +  char buffer[sizeof (FD_TO_FILENAME_PREFIX) + 10];
> +};
> +
> +/* Writes a /proc/self/fd-style path for DESCRIPTOR to *STORAGE and
> +   returns a pointer to the start of the string.  DESCRIPTOR must be
> +   non-negative.  */
> +char *__fd_to_filename (int descriptor, struct fd_to_filename *storage)
> +  attribute_hidden;
> +
> +#endif /* _FD_TO_FILENAME_H */
> diff --git a/sysdeps/mach/hurd/arch-fd_to_filename.h b/sysdeps/mach/hurd/arch-fd_to_filename.h
> new file mode 100644
> index 0000000000..b45cd8d836
> --- /dev/null
> +++ b/sysdeps/mach/hurd/arch-fd_to_filename.h
> @@ -0,0 +1,19 @@
> +/* Query filename corresponding to an open FD.  Hurd version.
> +   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/>.  */
> +
> +#define FD_TO_FILENAME_PREFIX "/dev/fd/"
> diff --git a/sysdeps/unix/sysv/linux/fd_to_filename.h b/sysdeps/unix/sysv/linux/arch-fd_to_filename.h
> similarity index 58%
> rename from sysdeps/unix/sysv/linux/fd_to_filename.h
> rename to sysdeps/unix/sysv/linux/arch-fd_to_filename.h
> index 92a5e02976..b6017214c7 100644
> --- a/sysdeps/unix/sysv/linux/fd_to_filename.h
> +++ b/sysdeps/unix/sysv/linux/arch-fd_to_filename.h
> @@ -1,5 +1,5 @@
>  /* Query filename corresponding to an open FD.  Linux version.
> -   Copyright (C) 2001-2020 Free Software Foundation, Inc.
> +   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
> @@ -16,22 +16,4 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> -#include <sys/stat.h>
> -#include <string.h>
> -#include <_itoa.h>
> -
> -#define FD_TO_FILENAME_SIZE ((sizeof ("/proc/self/fd/") - 1) \
> -     + (sizeof ("4294967295") - 1) + 1)
> -
> -static inline const char *
> -fd_to_filename (unsigned int fd, char *buf)
> -{
> -  *_fitoa_word (fd, __stpcpy (buf, "/proc/self/fd/"), 10, 0) = '\0';
> -
> -  /* We must make sure the file exists.  */
> -  struct stat64 st;
> -  if (__lxstat64 (_STAT_VER, buf, &st) < 0)
> -    /* /proc is not mounted or something else happened.  */
> -    return NULL;
> -  return buf;
> -}
> +#define FD_TO_FILENAME_PREFIX "/proc/self/fd/"
> --
> 2.24.1
>
>

--
Samuel
"2 + 2 = 5 pour d'assez grandes valeurs de 2"
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/3] <fd_to_filename.h>: Add type safety and port to Hurd

Adhemerval Zanella-2
In reply to this post by Florian Weimer-5


On 14/02/2020 15:11, Florian Weimer wrote:

> The new type struct fd_to_filename makes the allocation of the
> backing storage explicit.
>
> Hurd uses /dev/fd, not /proc/self/fd.
> ---
>  libio/freopen.c                               |   4 +-
>  libio/freopen64.c                             |   4 +-
>  misc/Makefile                                 |   6 +-
>  misc/fd_to_filename.c                         | 104 +++++++++++++++++
>  misc/tst-fd_to_filename.c                     | 105 ++++++++++++++++++
>  sysdeps/generic/arch-fd_to_filename.h         |  19 ++++
>  sysdeps/generic/fd_to_filename.h              |  25 +++--
>  sysdeps/mach/hurd/arch-fd_to_filename.h       |  19 ++++
>  ...fd_to_filename.h => arch-fd_to_filename.h} |  22 +---
>  9 files changed, 275 insertions(+), 33 deletions(-)
>  create mode 100644 misc/fd_to_filename.c
>  create mode 100644 misc/tst-fd_to_filename.c
>  create mode 100644 sysdeps/generic/arch-fd_to_filename.h
>  create mode 100644 sysdeps/mach/hurd/arch-fd_to_filename.h
>  rename sysdeps/unix/sysv/linux/{fd_to_filename.h => arch-fd_to_filename.h} (58%)
>
> diff --git a/libio/freopen.c b/libio/freopen.c
> index bab3ba204a..884cdb2961 100644
> --- a/libio/freopen.c
> +++ b/libio/freopen.c
> @@ -37,7 +37,7 @@ FILE *
>  freopen (const char *filename, const char *mode, FILE *fp)
>  {
>    FILE *result = NULL;
> -  char fdfilename[FD_TO_FILENAME_SIZE];
> +  struct fd_to_filename fdfilename;
>  
>    CHECK_FILE (fp, NULL);
>  
> @@ -50,7 +50,7 @@ freopen (const char *filename, const char *mode, FILE *fp)
>  
>    int fd = _IO_fileno (fp);
>    const char *gfilename
> -    = filename != NULL ? filename : fd_to_filename (fd, fdfilename);
> +    = filename != NULL ? filename : __fd_to_filename (fd, &fdfilename);
>  
>    fp->_flags2 |= _IO_FLAGS2_NOCLOSE;
>  #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
> diff --git a/libio/freopen64.c b/libio/freopen64.c
> index c0ce604e6e..0d2c5264c7 100644
> --- a/libio/freopen64.c
> +++ b/libio/freopen64.c
> @@ -36,7 +36,7 @@ FILE *
>  freopen64 (const char *filename, const char *mode, FILE *fp)
>  {
>    FILE *result = NULL;
> -  char fdfilename[FD_TO_FILENAME_SIZE];
> +  struct fd_to_filename fdfilename;
>  
>    CHECK_FILE (fp, NULL);
>  
> @@ -49,7 +49,7 @@ freopen64 (const char *filename, const char *mode, FILE *fp)
>  
>    int fd = _IO_fileno (fp);
>    const char *gfilename
> -    = filename != NULL ? filename : fd_to_filename (fd, fdfilename);
> +    = filename != NULL ? filename : __fd_to_filename (fd, &fdfilename);
>  
>    fp->_flags2 |= _IO_FLAGS2_NOCLOSE;
>    _IO_file_close_it (fp);
> diff --git a/misc/Makefile b/misc/Makefile
> index e0465980c7..b8fed5783d 100644
> --- a/misc/Makefile
> +++ b/misc/Makefile
> @@ -72,7 +72,7 @@ routines := brk sbrk sstk ioctl \
>      fgetxattr flistxattr fremovexattr fsetxattr getxattr \
>      listxattr lgetxattr llistxattr lremovexattr lsetxattr \
>      removexattr setxattr getauxval ifunc-impl-list makedev \
> -    allocate_once
> +    allocate_once fd_to_filename
>  
>  generated += tst-error1.mtrace tst-error1-mem.out \
>    tst-allocate_once.mtrace tst-allocate_once-mem.out
> @@ -97,6 +97,10 @@ endif
>  tests-internal := tst-atomic tst-atomic-long tst-allocate_once
>  tests-static := tst-empty
>  
> +# Test for the internal, non-exported __fd_to_filename function.
> +tests-internal += tst-fd_to_filename
> +tests-static += tst-fd_to_filename
> +
>  ifeq ($(run-built-tests),yes)
>  tests-special += $(objpfx)tst-error1-mem.out \
>    $(objpfx)tst-allocate_once-mem.out
> diff --git a/misc/fd_to_filename.c b/misc/fd_to_filename.c
> new file mode 100644
> index 0000000000..911c22cf2b
> --- /dev/null
> +++ b/misc/fd_to_filename.c
> @@ -0,0 +1,104 @@
> +/* Construct a pathname under /proc/self/fd (or /dev/fd for Hurd).
> +   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 <fd_to_filename.h>
> +#include <string.h>
> +
> +/* The split into groups of three digits is arbitrary, but it covers
> +   the common case of descriptors less than 1000 fairly
> +   efficiently.  */
> +
> +/* Writes two digits to P.  VALUE must be between 0 and 99
> +   (inclusive).  Returns the address after the written digits.  */
> +static char *
> +digit2 (char *p, int value)
> +{
> +  p[0] = '0' + (value / 10);
> +  p[1] = '0' + (value % 10);
> +  return p + 2;
> +}
> +
> +/* Writes three digits to P.  VALUE must be between 0 and 999
> +   (inclusive).  Returns the address after the written digits.  */
> +static char *
> +digit3 (char *p, int value)
> +{
> +  p[0] = '0' + (value / 100);
> +  p[1] = '0' + ((value / 10) % 10);
> +  p[2] = '0' + (value % 10);
> +  return p + 3;
> +}
> +
> +/* Writes one to three digits to P, depending on VALUE, which must be
> +   less than 1000.  Returns the address after the written digits.  */
> +static char *
> +digit123 (char *p, int value)
> +{
> +  if (value < 10)
> +    {
> +      p[0] = '0' + value;
> +      return p + 1;
> +    }
> +  else if (value < 100)
> +    return digit2 (p, value);
> +  else
> +    return digit3 (p, value);
> +}
> +
> +char *
> +__fd_to_filename (int descriptor, struct fd_to_filename *storage)
> +{
> +  char *p = mempcpy (storage->buffer, FD_TO_FILENAME_PREFIX,
> +                     strlen (FD_TO_FILENAME_PREFIX));
> +  if (__glibc_likely (descriptor >= 0))
> +    {
> +      if (descriptor < 1000)
> +        p = digit123 (p, descriptor);
> +      else if (descriptor < 1000 * 1000)
> +        {
> +          p = digit123 (p, descriptor / 1000);
> +          p = digit3 (p, descriptor % 1000);
> +        }
> +      else if (descriptor < 1000 * 1000 * 1000)
> +        {
> +          p = digit123 (p, descriptor / (1000 * 1000));
> +          p = digit3 (p, (descriptor / 1000) % 1000);
> +          p = digit3 (p, descriptor % 1000);
> +        }
> +      else
> +        {
> +          if (descriptor < 2000 * 1000 * 1000)
> +            {
> +              *p = '1';
> +              descriptor -= 1000 * 1000 * 1000;
> +            }
> +          else
> +            {
> +              *p = '2';
> +              descriptor -= 2000 * 1000 * 1000;
> +            }
> +          ++p;
> +          p = digit3 (p, descriptor / (1000 * 1000));
> +          p = digit3 (p, (descriptor / 1000) % 1000);
> +          p = digit3 (p, descriptor % 1000);
> +        }
> +    }
> +
> +  *p = '\0';
> +  return storage->buffer;
> +}

Do we really such optimization which results in somewhat complicate
code? Couldn't we use something simple as:

char *
__fd_to_filename (int descriptor, struct fd_to_filename *storage)
{
  char *buf = storage->buffer;
  int i, j;

  for (i = 0; (buf[i] = FD_TO_FILENAME_PREFIX[i]); i++);
  if (descriptor == 0)
    {
      buf[i] = '0';
      buf[i+1] = '\0';
      return storage->buffer;
    }

  for (j = descriptor; j != 0; j /= 10, i++);
  buf[i] = '\0';
  for (; descriptor != 0; descriptor /= 10)
    buf[--i] = '0' + descriptor % 10;

  return storage->buffer;
}


> diff --git a/misc/tst-fd_to_filename.c b/misc/tst-fd_to_filename.c
> new file mode 100644
> index 0000000000..480f613dcf
> --- /dev/null
> +++ b/misc/tst-fd_to_filename.c
> @@ -0,0 +1,105 @@
> +/* Test for /proc/self/fd (or /dev/fd) pathname construction.
> +   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 <fd_to_filename.h>
> +#include <stdio.h>
> +#include <support/check.h>
> +#include <support/xunistd.h>
> +
> +/* Run a check on one value.  */
> +static void
> +check (int value)
> +{
> +  struct fd_to_filename storage;
> +  char *actual = __fd_to_filename (value, &storage);
> +  char expected[100];
> +  snprintf (expected, sizeof (expected), FD_TO_FILENAME_PREFIX "%d", value);
> +
> +  if (value < 0)
> +    /* Negative inputs are invalid, but the output should still be
> +       deterministic.  */
> +    TEST_COMPARE_STRING (actual, FD_TO_FILENAME_PREFIX);
> +  else
> +    TEST_COMPARE_STRING (actual, expected);
> +}
> +
> +/* Check various ranges constructed around powers.  */
> +static void
> +check_ranges (int base)
> +{
> +  unsigned int power = 1;
> +  do
> +    {
> +      for (int factor = 1; factor < base; ++factor)
> +        for (int shift = -1000; shift <= 1000; ++shift)
> +          {
> +            check (factor * power + shift);
> +            check (-(factor * power + shift));
> +          }
> +    }
> +  while (!__builtin_mul_overflow (power, base, &power));
> +}
> +
> +/* Check that it is actually possible to use a the constructed
> +   name.  */
> +static void
> +check_open (void)
> +{
> +  int pipes[2];
> +  xpipe (pipes);
> +
> +  struct fd_to_filename storage;
> +  int read_alias = xopen (__fd_to_filename (pipes[0], &storage), O_RDONLY, 0);
> +  int write_alias = xopen (__fd_to_filename (pipes[1], &storage), O_WRONLY, 0);
> +
> +  /* Ensure that all the descriptor numbers are different.  */
> +  TEST_VERIFY (pipes[0] < pipes[1]);
> +  TEST_VERIFY (pipes[1] < read_alias);
> +  TEST_VERIFY (read_alias < write_alias);
> +
> +  xwrite (write_alias, "1", 1);
> +  char buf[16];
> +  TEST_COMPARE_BLOB ("1", 1, buf, read (pipes[0], buf, sizeof (buf)));
> +
> +  xwrite (pipes[1], "2", 1);
> +  TEST_COMPARE_BLOB ("2", 1, buf, read (read_alias, buf, sizeof (buf)));
> +
> +  xwrite (write_alias, "3", 1);
> +  TEST_COMPARE_BLOB ("3", 1, buf, read (read_alias, buf, sizeof (buf)));
> +
> +  xwrite (pipes[1], "4", 1);
> +  TEST_COMPARE_BLOB ("4", 1, buf, read (pipes[0], buf, sizeof (buf)));
> +
> +  xclose (write_alias);
> +  xclose (read_alias);
> +  xclose (pipes[1]);
> +  xclose (pipes[0]);
> +}
> +
> +static int
> +do_test (void)
> +{
> +  check_ranges (2);
> +  check_ranges (10);
> +
> +  check_open ();
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/sysdeps/generic/arch-fd_to_filename.h b/sysdeps/generic/arch-fd_to_filename.h
> new file mode 100644
> index 0000000000..ecaaa14dba
> --- /dev/null
> +++ b/sysdeps/generic/arch-fd_to_filename.h
> @@ -0,0 +1,19 @@
> +/* Query filename corresponding to an open FD.  Generic stub.
> +   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/>.  */
> +
> +#error "<arch-fd_to_filename.h> must be ported to this architecture"
> diff --git a/sysdeps/generic/fd_to_filename.h b/sysdeps/generic/fd_to_filename.h
> index eff6ca211b..7889db879c 100644
> --- a/sysdeps/generic/fd_to_filename.h
> +++ b/sysdeps/generic/fd_to_filename.h
> @@ -1,4 +1,4 @@
> -/* Query filename corresponding to an open FD.  Generic version.
> +/* Query filename corresponding to an open FD.
>     Copyright (C) 2001-2020 Free Software Foundation, Inc.
>     This file is part of the GNU C Library.
>  
> @@ -16,12 +16,21 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> -#define FD_TO_FILENAME_SIZE 0
> +#ifndef _FD_TO_FILENAME_H
> +#define _FD_TO_FILENAME_H
>  
> -/* In general there is no generic way to query filename for an open
> -   file descriptor.  */
> -static inline const char *
> -fd_to_filename (int fd, char *buf)
> +#include <arch-fd_to_filename.h>
> +
> +struct fd_to_filename
>  {
> -  return NULL;
> -}
> +  /* A positive int value has at most 10 decimal digits.  */
> +  char buffer[sizeof (FD_TO_FILENAME_PREFIX) + 10];
> +};
> +
> +/* Writes a /proc/self/fd-style path for DESCRIPTOR to *STORAGE and
> +   returns a pointer to the start of the string.  DESCRIPTOR must be
> +   non-negative.  */
> +char *__fd_to_filename (int descriptor, struct fd_to_filename *storage)
> +  attribute_hidden;
> +
> +#endif /* _FD_TO_FILENAME_H */
> diff --git a/sysdeps/mach/hurd/arch-fd_to_filename.h b/sysdeps/mach/hurd/arch-fd_to_filename.h
> new file mode 100644
> index 0000000000..b45cd8d836
> --- /dev/null
> +++ b/sysdeps/mach/hurd/arch-fd_to_filename.h
> @@ -0,0 +1,19 @@
> +/* Query filename corresponding to an open FD.  Hurd version.
> +   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/>.  */
> +
> +#define FD_TO_FILENAME_PREFIX "/dev/fd/"
> diff --git a/sysdeps/unix/sysv/linux/fd_to_filename.h b/sysdeps/unix/sysv/linux/arch-fd_to_filename.h
> similarity index 58%
> rename from sysdeps/unix/sysv/linux/fd_to_filename.h
> rename to sysdeps/unix/sysv/linux/arch-fd_to_filename.h
> index 92a5e02976..b6017214c7 100644
> --- a/sysdeps/unix/sysv/linux/fd_to_filename.h
> +++ b/sysdeps/unix/sysv/linux/arch-fd_to_filename.h
> @@ -1,5 +1,5 @@
>  /* Query filename corresponding to an open FD.  Linux version.
> -   Copyright (C) 2001-2020 Free Software Foundation, Inc.
> +   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
> @@ -16,22 +16,4 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> -#include <sys/stat.h>
> -#include <string.h>
> -#include <_itoa.h>
> -
> -#define FD_TO_FILENAME_SIZE ((sizeof ("/proc/self/fd/") - 1) \
> -     + (sizeof ("4294967295") - 1) + 1)
> -
> -static inline const char *
> -fd_to_filename (unsigned int fd, char *buf)
> -{
> -  *_fitoa_word (fd, __stpcpy (buf, "/proc/self/fd/"), 10, 0) = '\0';
> -
> -  /* We must make sure the file exists.  */
> -  struct stat64 st;
> -  if (__lxstat64 (_STAT_VER, buf, &st) < 0)
> -    /* /proc is not mounted or something else happened.  */
> -    return NULL;
> -  return buf;
> -}
> +#define FD_TO_FILENAME_PREFIX "/proc/self/fd/"
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/3] <fd_to_filename.h>: Add type safety and port to Hurd

Florian Weimer-5
* Adhemerval Zanella:

> Do we really such optimization which results in somewhat complicate
> code? Couldn't we use something simple as:
>
> char *
> __fd_to_filename (int descriptor, struct fd_to_filename *storage)
> {
>   char *buf = storage->buffer;
>   int i, j;
>
>   for (i = 0; (buf[i] = FD_TO_FILENAME_PREFIX[i]); i++);
>   if (descriptor == 0)
>     {
>       buf[i] = '0';
>       buf[i+1] = '\0';
>       return storage->buffer;
>     }
>
>   for (j = descriptor; j != 0; j /= 10, i++);
>   buf[i] = '\0';
>   for (; descriptor != 0; descriptor /= 10)
>     buf[--i] = '0' + descriptor % 10;
>
>   return storage->buffer;
> }

I don't have a strong opinion about this (but I think we should probably
keep the mempcpy).  Most descriptors will be fairly small, so the
smaller code size is probably more important.

Thanks,
Florian

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/3] <fd_to_filename.h>: Add type safety and port to Hurd

Paul Eggert
In reply to this post by Florian Weimer-5
On 2/14/20 10:11 AM, Florian Weimer wrote:
> +  /* A positive int value has at most 10 decimal digits.  */
> +  char buffer[sizeof (FD_TO_FILENAME_PREFIX) + 10];

This should use 'INT_STRLEN_BOUND (int)' rather than '10', where
INT_STRLEN_BOUND is taken from intprops.h. Then you don't need the
comment (and the code won't break on future ILP64 platforms :-).

> +char *
> +__fd_to_filename (int descriptor, struct fd_to_filename *storage)
> +{
> +  char *p = mempcpy (storage->buffer, FD_TO_FILENAME_PREFIX,
> +                     strlen (FD_TO_FILENAME_PREFIX));
> +  if (__glibc_likely (descriptor >= 0))
> +    {
> +      if (descriptor < 1000)
> +        p = digit123 (p, descriptor);
> +      else if (descriptor < 1000 * 1000) ...

It's not clear what that "descriptor >= 0" test is doing. I assume that
a precondition is that DESCRIPTOR is nonnegative; if so, this should be
mentioned and the test removed. If it's not a precondition, the code
should do the right thing if DESCRIPTOR is negative (I suppose, return a
filename that cannot be opened, though it doesn't do that currently) --
which should also be documented but I think this'd be overkill.

As Adhemerval writes, there's no need for special cases for powers of
1000 etc. Something like the following should do (this is a bit shorter
than Adhemerval's version):

   /* Convert nonnegative DESCRIPTOR to a file name.  */

   char *
   __fd_to_filename (int descriptor, struct fd_to_filename *storage)
   {
     char *p = mempcpy (storage->buffer, FD_TO_FILENAME_PREFIX,
                       strlen (FD_TO_FILENAME_PREFIX) - 1);
     for (int d = descriptor; p++, (d /= 10) != 0; )
       continue;
     *p = '\0';
     for (int d = descriptor; *--p = '0' + d % 10, (d /= 10) != 0; )
       continue;
     return storage->buffer;
   }

GCC turns those divisions and remainders into the usual
multiply+shift+add and this should be good enough.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/3] <fd_to_filename.h>: Add type safety and port to Hurd

Florian Weimer-5
* Paul Eggert:

> On 2/14/20 10:11 AM, Florian Weimer wrote:
>> +  /* A positive int value has at most 10 decimal digits.  */
>> +  char buffer[sizeof (FD_TO_FILENAME_PREFIX) + 10];
>
> This should use 'INT_STRLEN_BOUND (int)' rather than '10', where
> INT_STRLEN_BOUND is taken from intprops.h. Then you don't need the
> comment (and the code won't break on future ILP64 platforms :-).

But INT_STRLEN_BOUND is 11, right?

>> +char *
>> +__fd_to_filename (int descriptor, struct fd_to_filename *storage)
>> +{
>> +  char *p = mempcpy (storage->buffer, FD_TO_FILENAME_PREFIX,
>> +                     strlen (FD_TO_FILENAME_PREFIX));
>> +  if (__glibc_likely (descriptor >= 0))
>> +    {
>> +      if (descriptor < 1000)
>> +        p = digit123 (p, descriptor);
>> +      else if (descriptor < 1000 * 1000) ...
>
> It's not clear what that "descriptor >= 0" test is doing. I assume
> that a precondition is that DESCRIPTOR is nonnegative; if so, this
> should be mentioned and the test removed. If it's not a precondition,
> the code should do the right thing if DESCRIPTOR is negative (I
> suppose, return a filename that cannot be opened, though it doesn't do
> that currently) -- which should also be documented but I think this'd
> be overkill.

The problem is when an application passes an invalid descriptor to some
libc function and that ends up with __fd_to_filename.  We should not
make matters worse in that case.

Maybe we should use an assert?

Thanks,
Florian

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/3] <fd_to_filename.h>: Add type safety and port to Hurd

Paul Eggert
On 2/15/20 5:16 AM, Florian Weimer wrote:

> INT_STRLEN_BOUND is 11, right?

Yes, it's a bound on the string length of a printed int, and that's 11 in the
typical case of 32-bit int because the int might be negative.  I didn't lose
sleep over the wasted byte, but if we want a tighter bound then we could use
INT_STRLEN_BOUND (int) - 1 instead. However, it might be better to leave it
alone so that we can use the code below.

> The problem is when an application passes an invalid descriptor to some
> libc function and that ends up with __fd_to_filename.  We should not
> make matters worse in that case.

If it's not a precondition that the descriptor is nonnegative, we can't simply
return a copy of FD_TO_FILENAME_PREFIX as that's an existing filename. Instead,
how about the following? It uses a randomish garbage filename beginning with "-"
which should be good enough, and it doesn't cost a conditional branch to handle
negative descriptors.

   char *
   __fd_to_filename (int descriptor, struct fd_to_filename *storage)
   {
     char *p = mempcpy (storage->buffer, FD_TO_FILENAME_PREFIX,
                        strlen (FD_TO_FILENAME_PREFIX) - 1);

     /* If DESCRIPTOR is negative, arrange for the filename to not exist
        by prepending any byte other than '/', '.', '\0' or an ASCII digit.
        The rest of the filename will be gibberish that fits.  */
     *p = '-';
     p += descriptor < 0;

     for (int d = descriptor; p++, (d /= 10) != 0; )
       continue;
     *p = '\0';
     for (int d = descriptor; *--p = '0' + d % 10, (d /= 10) != 0; )
       continue;
     return storage->buffer;
   }
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/3] <fd_to_filename.h>: Add type safety and port to Hurd

Florian Weimer-5
* Paul Eggert:

> On 2/15/20 5:16 AM, Florian Weimer wrote:
>
>> INT_STRLEN_BOUND is 11, right?
>
> Yes, it's a bound on the string length of a printed int, and that's 11
> in the typical case of 32-bit int because the int might be negative.
> I didn't lose sleep over the wasted byte, but if we want a tighter
> bound then we could use INT_STRLEN_BOUND (int) - 1 instead. However,
> it might be better to leave it alone so that we can use the code
> below.
>
>> The problem is when an application passes an invalid descriptor to some
>> libc function and that ends up with __fd_to_filename.  We should not
>> make matters worse in that case.
>
> If it's not a precondition that the descriptor is nonnegative, we
> can't simply return a copy of FD_TO_FILENAME_PREFIX as that's an
> existing filename. Instead, how about the following? It uses a
> randomish garbage filename beginning with "-"
> which should be good enough, and it doesn't cost a conditional branch
> to handle negative descriptors.
>
>   char *
>   __fd_to_filename (int descriptor, struct fd_to_filename *storage)
>   {
>     char *p = mempcpy (storage->buffer, FD_TO_FILENAME_PREFIX,
>                        strlen (FD_TO_FILENAME_PREFIX) - 1);
>
>     /* If DESCRIPTOR is negative, arrange for the filename to not exist
>        by prepending any byte other than '/', '.', '\0' or an ASCII digit.
>        The rest of the filename will be gibberish that fits.  */
>     *p = '-';
>     p += descriptor < 0;
>
>     for (int d = descriptor; p++, (d /= 10) != 0; )
>       continue;
>     *p = '\0';
>     for (int d = descriptor; *--p = '0' + d % 10, (d /= 10) != 0; )
>       continue;
>     return storage->buffer;
>   }

Here's an updated version, which adds a dependency on <intprops.h> (a
header I really dislike) and mostly uses your implementation of
__fd_to_filename.

Okay for master?

Thanks,
Florian

8<------------------------------------------------------------------8<
The new type struct fd_to_filename makes the allocation of the
backing storage explicit.

Hurd uses /dev/fd, not /proc/self/fd.

Co-Authored-By: Paul Eggert <[hidden email]>

-----
 libio/freopen.c                                    |   4 +-
 libio/freopen64.c                                  |   4 +-
 misc/Makefile                                      |   6 +-
 misc/fd_to_filename.c                              |  38 ++++++++
 misc/tst-fd_to_filename.c                          | 100 +++++++++++++++++++++
 sysdeps/generic/arch-fd_to_filename.h              |  19 ++++
 sysdeps/generic/fd_to_filename.h                   |  26 ++++--
 sysdeps/mach/hurd/arch-fd_to_filename.h            |  19 ++++
 .../{fd_to_filename.h => arch-fd_to_filename.h}    |  22 +----
 9 files changed, 205 insertions(+), 33 deletions(-)

diff --git a/libio/freopen.c b/libio/freopen.c
index bab3ba204a..884cdb2961 100644
--- a/libio/freopen.c
+++ b/libio/freopen.c
@@ -37,7 +37,7 @@ FILE *
 freopen (const char *filename, const char *mode, FILE *fp)
 {
   FILE *result = NULL;
-  char fdfilename[FD_TO_FILENAME_SIZE];
+  struct fd_to_filename fdfilename;
 
   CHECK_FILE (fp, NULL);
 
@@ -50,7 +50,7 @@ freopen (const char *filename, const char *mode, FILE *fp)
 
   int fd = _IO_fileno (fp);
   const char *gfilename
-    = filename != NULL ? filename : fd_to_filename (fd, fdfilename);
+    = filename != NULL ? filename : __fd_to_filename (fd, &fdfilename);
 
   fp->_flags2 |= _IO_FLAGS2_NOCLOSE;
 #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
diff --git a/libio/freopen64.c b/libio/freopen64.c
index c0ce604e6e..0d2c5264c7 100644
--- a/libio/freopen64.c
+++ b/libio/freopen64.c
@@ -36,7 +36,7 @@ FILE *
 freopen64 (const char *filename, const char *mode, FILE *fp)
 {
   FILE *result = NULL;
-  char fdfilename[FD_TO_FILENAME_SIZE];
+  struct fd_to_filename fdfilename;
 
   CHECK_FILE (fp, NULL);
 
@@ -49,7 +49,7 @@ freopen64 (const char *filename, const char *mode, FILE *fp)
 
   int fd = _IO_fileno (fp);
   const char *gfilename
-    = filename != NULL ? filename : fd_to_filename (fd, fdfilename);
+    = filename != NULL ? filename : __fd_to_filename (fd, &fdfilename);
 
   fp->_flags2 |= _IO_FLAGS2_NOCLOSE;
   _IO_file_close_it (fp);
diff --git a/misc/Makefile b/misc/Makefile
index e0465980c7..b8fed5783d 100644
--- a/misc/Makefile
+++ b/misc/Makefile
@@ -72,7 +72,7 @@ routines := brk sbrk sstk ioctl \
     fgetxattr flistxattr fremovexattr fsetxattr getxattr \
     listxattr lgetxattr llistxattr lremovexattr lsetxattr \
     removexattr setxattr getauxval ifunc-impl-list makedev \
-    allocate_once
+    allocate_once fd_to_filename
 
 generated += tst-error1.mtrace tst-error1-mem.out \
   tst-allocate_once.mtrace tst-allocate_once-mem.out
@@ -97,6 +97,10 @@ endif
 tests-internal := tst-atomic tst-atomic-long tst-allocate_once
 tests-static := tst-empty
 
+# Test for the internal, non-exported __fd_to_filename function.
+tests-internal += tst-fd_to_filename
+tests-static += tst-fd_to_filename
+
 ifeq ($(run-built-tests),yes)
 tests-special += $(objpfx)tst-error1-mem.out \
   $(objpfx)tst-allocate_once-mem.out
diff --git a/misc/fd_to_filename.c b/misc/fd_to_filename.c
new file mode 100644
index 0000000000..03d19194c1
--- /dev/null
+++ b/misc/fd_to_filename.c
@@ -0,0 +1,38 @@
+/* Construct a pathname under /proc/self/fd (or /dev/fd for Hurd).
+   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 <fd_to_filename.h>
+
+#include <assert.h>
+#include <string.h>
+
+char *
+__fd_to_filename (int descriptor, struct fd_to_filename *storage)
+{
+  assert (descriptor >= 0);
+
+  char *p = mempcpy (storage->buffer, FD_TO_FILENAME_PREFIX,
+                     strlen (FD_TO_FILENAME_PREFIX));
+
+  for (int d = descriptor; p++, (d /= 10) != 0; )
+    continue;
+  *p = '\0';
+  for (int d = descriptor; *--p = '0' + d % 10, (d /= 10) != 0; )
+    continue;
+  return storage->buffer;
+}
diff --git a/misc/tst-fd_to_filename.c b/misc/tst-fd_to_filename.c
new file mode 100644
index 0000000000..3a3bccdbcf
--- /dev/null
+++ b/misc/tst-fd_to_filename.c
@@ -0,0 +1,100 @@
+/* Test for /proc/self/fd (or /dev/fd) pathname construction.
+   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 <fd_to_filename.h>
+#include <stdio.h>
+#include <support/check.h>
+#include <support/xunistd.h>
+
+/* Run a check on one value.  */
+static void
+check (int value)
+{
+  if (value < 0)
+    /* Negative descriptor values violate the precondition.  */
+    return;
+
+  struct fd_to_filename storage;
+  char *actual = __fd_to_filename (value, &storage);
+  char expected[100];
+  snprintf (expected, sizeof (expected), FD_TO_FILENAME_PREFIX "%d", value);
+  TEST_COMPARE_STRING (actual, expected);
+}
+
+/* Check various ranges constructed around powers.  */
+static void
+check_ranges (int base)
+{
+  unsigned int power = 1;
+  do
+    {
+      for (int factor = 1; factor < base; ++factor)
+        for (int shift = -1000; shift <= 1000; ++shift)
+          check (factor * power + shift);
+    }
+  while (!__builtin_mul_overflow (power, base, &power));
+}
+
+/* Check that it is actually possible to use a the constructed
+   name.  */
+static void
+check_open (void)
+{
+  int pipes[2];
+  xpipe (pipes);
+
+  struct fd_to_filename storage;
+  int read_alias = xopen (__fd_to_filename (pipes[0], &storage), O_RDONLY, 0);
+  int write_alias = xopen (__fd_to_filename (pipes[1], &storage), O_WRONLY, 0);
+
+  /* Ensure that all the descriptor numbers are different.  */
+  TEST_VERIFY (pipes[0] < pipes[1]);
+  TEST_VERIFY (pipes[1] < read_alias);
+  TEST_VERIFY (read_alias < write_alias);
+
+  xwrite (write_alias, "1", 1);
+  char buf[16];
+  TEST_COMPARE_BLOB ("1", 1, buf, read (pipes[0], buf, sizeof (buf)));
+
+  xwrite (pipes[1], "2", 1);
+  TEST_COMPARE_BLOB ("2", 1, buf, read (read_alias, buf, sizeof (buf)));
+
+  xwrite (write_alias, "3", 1);
+  TEST_COMPARE_BLOB ("3", 1, buf, read (read_alias, buf, sizeof (buf)));
+
+  xwrite (pipes[1], "4", 1);
+  TEST_COMPARE_BLOB ("4", 1, buf, read (pipes[0], buf, sizeof (buf)));
+
+  xclose (write_alias);
+  xclose (read_alias);
+  xclose (pipes[1]);
+  xclose (pipes[0]);
+}
+
+static int
+do_test (void)
+{
+  check_ranges (2);
+  check_ranges (10);
+
+  check_open ();
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/sysdeps/generic/arch-fd_to_filename.h b/sysdeps/generic/arch-fd_to_filename.h
new file mode 100644
index 0000000000..ecaaa14dba
--- /dev/null
+++ b/sysdeps/generic/arch-fd_to_filename.h
@@ -0,0 +1,19 @@
+/* Query filename corresponding to an open FD.  Generic stub.
+   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/>.  */
+
+#error "<arch-fd_to_filename.h> must be ported to this architecture"
diff --git a/sysdeps/generic/fd_to_filename.h b/sysdeps/generic/fd_to_filename.h
index eff6ca211b..5ca22f02bc 100644
--- a/sysdeps/generic/fd_to_filename.h
+++ b/sysdeps/generic/fd_to_filename.h
@@ -1,4 +1,4 @@
-/* Query filename corresponding to an open FD.  Generic version.
+/* Query filename corresponding to an open FD.
    Copyright (C) 2001-2020 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
@@ -16,12 +16,22 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
-#define FD_TO_FILENAME_SIZE 0
+#ifndef _FD_TO_FILENAME_H
+#define _FD_TO_FILENAME_H
 
-/* In general there is no generic way to query filename for an open
-   file descriptor.  */
-static inline const char *
-fd_to_filename (int fd, char *buf)
+#include <arch-fd_to_filename.h>
+#include <intprops.h>
+
+struct fd_to_filename
 {
-  return NULL;
-}
+  /* A positive int value has at most 10 decimal digits.  */
+  char buffer[sizeof (FD_TO_FILENAME_PREFIX) + INT_STRLEN_BOUND (int)];
+};
+
+/* Writes a /proc/self/fd-style path for DESCRIPTOR to *STORAGE and
+   returns a pointer to the start of the string.  DESCRIPTOR must be
+   non-negative.  */
+char *__fd_to_filename (int descriptor, struct fd_to_filename *storage)
+  attribute_hidden;
+
+#endif /* _FD_TO_FILENAME_H */
diff --git a/sysdeps/mach/hurd/arch-fd_to_filename.h b/sysdeps/mach/hurd/arch-fd_to_filename.h
new file mode 100644
index 0000000000..b45cd8d836
--- /dev/null
+++ b/sysdeps/mach/hurd/arch-fd_to_filename.h
@@ -0,0 +1,19 @@
+/* Query filename corresponding to an open FD.  Hurd version.
+   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/>.  */
+
+#define FD_TO_FILENAME_PREFIX "/dev/fd/"
diff --git a/sysdeps/unix/sysv/linux/fd_to_filename.h b/sysdeps/unix/sysv/linux/arch-fd_to_filename.h
similarity index 58%
rename from sysdeps/unix/sysv/linux/fd_to_filename.h
rename to sysdeps/unix/sysv/linux/arch-fd_to_filename.h
index 92a5e02976..b6017214c7 100644
--- a/sysdeps/unix/sysv/linux/fd_to_filename.h
+++ b/sysdeps/unix/sysv/linux/arch-fd_to_filename.h
@@ -1,5 +1,5 @@
 /* Query filename corresponding to an open FD.  Linux version.
-   Copyright (C) 2001-2020 Free Software Foundation, Inc.
+   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
@@ -16,22 +16,4 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
-#include <sys/stat.h>
-#include <string.h>
-#include <_itoa.h>
-
-#define FD_TO_FILENAME_SIZE ((sizeof ("/proc/self/fd/") - 1) \
-     + (sizeof ("4294967295") - 1) + 1)
-
-static inline const char *
-fd_to_filename (unsigned int fd, char *buf)
-{
-  *_fitoa_word (fd, __stpcpy (buf, "/proc/self/fd/"), 10, 0) = '\0';
-
-  /* We must make sure the file exists.  */
-  struct stat64 st;
-  if (__lxstat64 (_STAT_VER, buf, &st) < 0)
-    /* /proc is not mounted or something else happened.  */
-    return NULL;
-  return buf;
-}
+#define FD_TO_FILENAME_PREFIX "/proc/self/fd/"

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/3] <fd_to_filename.h>: Add type safety and port to Hurd

Paul Eggert
On 2/17/20 7:18 AM, Florian Weimer wrote:
> Here's an updated version

Thanks, it looks good.

> which adds a dependency on <intprops.h> (a
> header I really dislike)

I disliked *writing* intprops.h (what a mess that part of C is!) but I don't
dislike *using* it when it's clearer or more reliable than the alternatives.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/3] <fd_to_filename.h>: Add type safety and port to Hurd

Adhemerval Zanella-2


On 17/02/2020 15:26, Paul Eggert wrote:
> On 2/17/20 7:18 AM, Florian Weimer wrote:
>> Here's an updated version
>
> Thanks, it looks good.
>
>> which adds a dependency on <intprops.h> (a
>> header I really dislike)
>
> I disliked *writing* intprops.h (what a mess that part of C is!) but I don't dislike *using* it when it's clearer or more reliable than the alternatives.

I think the main problem with intprops.h is it contains a lot
of clutter not really useful for the current glibc targets,
and it could be simplified with current minimum build compiler
requirement.

Also, although its usage is indeed handy in some situations,
its implementation is somewhat complex and convoluted. I don't
see a better solution with given constraint though.

So the question is if it still worth to keep it in sync with
gnulib, or if is better to push for a more tailored and clean
replacement.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/3] <fd_to_filename.h>: Add type safety and port to Hurd

Paul Eggert
On 2/17/20 10:59 AM, Adhemerval Zanella wrote:
> So the question is if it still worth to keep it in sync with
> gnulib, or if is better to push for a more tailored and clean
> replacement.

Another option would be to clarify which part is used in glibc, and which part
is needed only for Gnulib. We do that in some other files shared between the two
projects, by using "#if _LIBC" and the like.