diff mbox

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

Message ID 8736b9nsx4.fsf@oldenburg2.str.redhat.com
State Committed
Headers

Commit Message

Florian Weimer Feb. 17, 2020, 3:18 p.m. UTC 
  * 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(-)

Comments

Paul Eggert Feb. 17, 2020, 6:26 p.m. UTC | #1 
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.
Adhemerval Zanella Feb. 17, 2020, 6:59 p.m. UTC | #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.
Paul Eggert Feb. 17, 2020, 7:14 p.m. UTC | #3 
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.
diff mbox

Patch

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