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

[Florian Weimer <fweimer@redhat.com>]

* 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 <eggert@cs.ucla.edu>

-----
 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/"