This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] libio: Avoid dup already opened file descriptor [BZ#21393]



On 18/05/2017 17:00, Siddhesh Poyarekar wrote:
> On Friday 19 May 2017 01:13 AM, Adhemerval Zanella wrote:
>> I think for BZ#21393 we won't get any sufficient conforming implementation
>> with proper kernel help or without relaxing the POSIX definition to also
>> allow this kind of errno.  So I still think that checking for EBUSY and
>> returning it is still the correct approach.  Specially because for the cases 
>> where it fails then application might still seeing spurious issue due freopen
>> returning a valid return code but without dupping correctly the file
>> descriptor.
> 
> OK, then this would require a NEWS item declaring that freopen behaviour
> is fixed and that its failure can now also set errno as EBUSY.  Maybe
> the man page needs updating as well.

Since the bug report associated with this issue will be on NEWS already, I
think updating the documentation should be suffice.  What about this patch
below:

--

	[BZ #21393]
	* libio/freopen.c (freopen): Avoid dup already opened file descriptor
	and add a check for dup3 failure.
	* libio/freopen64.c (freopen64): Likewise.
	* libio/tst-freopen.c (do_test): Rename to do_test_basic and use
	libsupport.
	(do_test_bz21398): New test.
	* manual/stdio.texi (freopen): Add documentation of EBUSY failure.

---

diff --git a/libio/freopen.c b/libio/freopen.c
index ad1c848..980523a 100644
--- a/libio/freopen.c
+++ b/libio/freopen.c
@@ -76,17 +76,31 @@ freopen (const char *filename, const char *mode, FILE *fp)
       /* unbound stream orientation */
       result->_mode = 0;
 
-      if (fd != -1)
+      if (fd != -1 && _IO_fileno (result) != fd)
 	{
-	  __dup3 (_IO_fileno (result), fd,
-		  (result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0
-		  ? O_CLOEXEC : 0);
+	  /* At this point we have both file descriptors already allocated,
+	     so __dup3 will not fail with EBADF, EINVAL, or EMFILE.  But
+	     we still need to check for EINVAL and, due Linux internal
+	     implementation, EBUSY.  It is because on how it internally opens
+	     the file by splitting the buffer allocation operation and VFS
+	     opening (a dup operation may run when a file is still pending
+	     'install' on VFS).  */
+	  if (__dup3 (_IO_fileno (result), fd,
+		      (result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0
+		      ? O_CLOEXEC : 0) == -1)
+	    {
+	      _IO_file_close_it (result);
+	      result = NULL;
+	      goto end;
+	    }
 	  __close (_IO_fileno (result));
 	  _IO_fileno (result) = fd;
 	}
     }
   else if (fd != -1)
     __close (fd);
+
+end:
   if (filename == NULL)
     free ((char *) gfilename);
 
diff --git a/libio/freopen64.c b/libio/freopen64.c
index adf749a..1e56de6 100644
--- a/libio/freopen64.c
+++ b/libio/freopen64.c
@@ -59,17 +59,31 @@ freopen64 (const char *filename, const char *mode, FILE *fp)
       /* unbound stream orientation */
       result->_mode = 0;
 
-      if (fd != -1)
+      if (fd != -1 && _IO_fileno (result) != fd)
 	{
-	  __dup3 (_IO_fileno (result), fd,
-		  (result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0
-		  ? O_CLOEXEC : 0);
+	  /* At this point we have both file descriptors already allocated,
+	     so __dup3 will not fail with EBADF, EINVAL, or EMFILE.  But
+	     we still need to check for EINVAL and, due Linux internal
+	     implementation, EBUSY.  It is because on how it internally opens
+	     the file by splitting the buffer allocation operation and VFS
+	     opening (a dup operation may run when a file is still pending
+	     'install' on VFS).  */
+	  if (__dup3 (_IO_fileno (result), fd,
+		      (result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0
+		      ? O_CLOEXEC : 0) == -1)
+	    {
+	      _IO_file_close_it (result);
+	      result = NULL;
+	      goto end;
+	    }
 	  __close (_IO_fileno (result));
 	  _IO_fileno (result) = fd;
 	}
     }
   else if (fd != -1)
     __close (fd);
+
+end:
   if (filename == NULL)
     free ((char *) gfilename);
   _IO_release_lock (fp);
diff --git a/libio/tst-freopen.c b/libio/tst-freopen.c
index 5f531e3..5ad589b 100644
--- a/libio/tst-freopen.c
+++ b/libio/tst-freopen.c
@@ -22,84 +22,91 @@
 #include <string.h>
 #include <unistd.h>
 
-static int
-do_test (void)
+#include <support/check.h>
+#include <support/temp_file.h>
+
+static int fd;
+static char *name;
+
+static void
+do_prepare (int argc, char *argv[])
+{
+  fd = create_temp_file ("tst-freopen.", &name);
+  TEST_VERIFY_EXIT (fd != -1);
+}
+
+#define PREPARE do_prepare
+
+/* Basic tests for freopen.  */
+static void
+do_test_basic (void)
 {
-  char name[] = "/tmp/tst-freopen.XXXXXX";
   const char * const test = "Let's test freopen.\n";
   char temp[strlen (test) + 1];
-  int fd = mkstemp (name);
-  FILE *f;
-
-  if (fd == -1)
-    {
-      printf ("%u: cannot open temporary file: %m\n", __LINE__);
-      exit (1);
-    }
 
-  f = fdopen (fd, "w");
+  FILE *f = fdopen (fd, "w");
   if (f == NULL)
-    {
-      printf ("%u: cannot fdopen temporary file: %m\n", __LINE__);
-      exit (1);
-    }
+    FAIL_EXIT1 ("fdopen: %m");
 
   fputs (test, f);
   fclose (f);
 
   f = fopen (name, "r");
   if (f == NULL)
-    {
-      printf ("%u: cannot fopen temporary file: %m\n", __LINE__);
-      exit (1);
-    }
+    FAIL_EXIT1 ("fopen: %m");
 
   if (fread (temp, 1, strlen (test), f) != strlen (test))
-    {
-      printf ("%u: couldn't read the file back: %m\n", __LINE__);
-      exit (1);
-    }
+    FAIL_EXIT1 ("fread: %m");
   temp [strlen (test)] = '\0';
 
   if (strcmp (test, temp))
-    {
-      printf ("%u: read different string than was written:\n%s%s",
-	      __LINE__, test, temp);
-      exit (1);
-    }
+    FAIL_EXIT1 ("read different string than was written: (%s, %s)",
+	        test, temp);
 
   f = freopen (name, "r+", f);
   if (f == NULL)
-    {
-      printf ("%u: cannot freopen temporary file: %m\n", __LINE__);
-      exit (1);
-    }
+    FAIL_EXIT1 ("freopen: %m");
 
   if (fseek (f, 0, SEEK_SET) != 0)
-    {
-      printf ("%u: couldn't fseek to start: %m\n", __LINE__);
-      exit (1);
-    }
+    FAIL_EXIT1 ("fseek: %m");
 
   if (fread (temp, 1, strlen (test), f) != strlen (test))
-    {
-      printf ("%u: couldn't read the file back: %m\n", __LINE__);
-      exit (1);
-    }
+    FAIL_EXIT1 ("fread: %m");
   temp [strlen (test)] = '\0';
 
   if (strcmp (test, temp))
-    {
-      printf ("%u: read different string than was written:\n%s%s",
-	      __LINE__, test, temp);
-      exit (1);
-    }
+    FAIL_EXIT1 ("read different string than was written: (%s, %s)",
+	        test, temp);
 
   fclose (f);
+}
+
+/* Test for BZ#21398, where it tries to freopen stdio after the close
+   of its file descriptor.  */
+static void
+do_test_bz21398 (void)
+{
+  (void) close (STDIN_FILENO);
+
+  FILE *f = freopen (name, "r", stdin);
+  if (f == NULL)
+    FAIL_EXIT1 ("freopen: %m");
+
+  TEST_VERIFY_EXIT (ferror (f) == 0);
+
+  char buf[128];
+  char *ret = fgets (buf, sizeof (buf), stdin);
+  TEST_VERIFY_EXIT (ret != NULL);
+  TEST_VERIFY_EXIT (ferror (f) == 0);
+}
+
+static int
+do_test (void)
+{
+  do_test_basic ();
+  do_test_bz21398 ();
 
-  unlink (name);
-  exit (0);
+  return 0;
 }
 
-#define TEST_FUNCTION do_test ()
-#include "../test-skeleton.c"
+#include <support/test-driver.c>
diff --git a/manual/stdio.texi b/manual/stdio.texi
index dbb21ca..17a93e4 100644
--- a/manual/stdio.texi
+++ b/manual/stdio.texi
@@ -316,7 +316,9 @@ actually done any output using the stream.)  Then the file named by
 and associated with the same stream object @var{stream}.
 
 If the operation fails, a null pointer is returned; otherwise,
-@code{freopen} returns @var{stream}.
+@code{freopen} returns @var{stream}.  Also for Linux, due internal dup3
+usage, it might fail with errno set to EBUSY during a race condition
+with @code{open} and @code{dup}.
 
 @code{freopen} has traditionally been used to connect a standard stream
 such as @code{stdin} with a file of your own choice.  This is useful in


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]