diff mbox

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

Message ID aca141a5-b534-4c2b-331a-16c97c533f56@linaro.org
State Dropped
Headers show

Commit Message

Adhemerval Zanella May 22, 2017, 5:25 p.m. UTC
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.

---

Comments

Siddhesh Poyarekar May 22, 2017, 8:04 p.m. UTC | #1
On Monday 22 May 2017 10:55 PM, Adhemerval Zanella wrote:
>  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}.

On Linux, @code{freopen} may fail and set @code{errno} to @{EBUSY} when
the kernel structure for the old file descriptor was not initialized
completely before @code{freopen} was called.

OK with that change.

Siddhesh
Adhemerval Zanella May 22, 2017, 8:18 p.m. UTC | #2
On 22/05/2017 17:04, Siddhesh Poyarekar wrote:
> On Monday 22 May 2017 10:55 PM, Adhemerval Zanella wrote:
>>  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}.
> 
> On Linux, @code{freopen} may fail and set @code{errno} to @{EBUSY} when
> the kernel structure for the old file descriptor was not initialized
> completely before @code{freopen} was called.
> 
> OK with that change.
> 
> Siddhesh
> 

Alright, I will change it (I based this doc from freopen(3) btw).
Siddhesh Poyarekar May 22, 2017, 8:21 p.m. UTC | #3
On Tuesday 23 May 2017 01:48 AM, Adhemerval Zanella wrote:
> Alright, I will change it (I based this doc from freopen(3) btw).

Oh, which version?  I was going to suggest writing a man page patch as
well :)

Siddhesh
Zack Weinberg May 22, 2017, 8:25 p.m. UTC | #4
On Mon, May 22, 2017 at 4:04 PM, Siddhesh Poyarekar <siddhesh@gotplt.org> wrote:
> On Monday 22 May 2017 10:55 PM, Adhemerval Zanella wrote:
>>  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}.
>
> On Linux, @code{freopen} may fail and set @code{errno} to @{EBUSY} when
> the kernel structure for the old file descriptor was not initialized
> completely before @code{freopen} was called.

+ "This can only happen in multi-threaded programs, when two threads
race to allocate the same file descriptor number.  To avoid the
possibility of this race, do not use @code{close} to close the
underlying file descriptor for a @code{FILE}; either use
@code{freopen} while the file is still open, or use @code{open} and
then @code{dup2} to install the new file descriptor."

zw
Adhemerval Zanella May 22, 2017, 8:25 p.m. UTC | #5
On 22/05/2017 17:21, Siddhesh Poyarekar wrote:
> On Tuesday 23 May 2017 01:48 AM, Adhemerval Zanella wrote:
>> Alright, I will change it (I based this doc from freopen(3) btw).
> 
> Oh, which version?  I was going to suggest writing a man page patch as
> well :)
> 
> Siddhesh
> 

Release 4.04 and I in fact it came from dup3(2) (my mistake):

       EBUSY  (Linux only) This may be returned by dup2() or dup3() during a 
race condition with open(2) and dup().
Adhemerval Zanella May 22, 2017, 8:26 p.m. UTC | #6
On 22/05/2017 17:25, Zack Weinberg wrote:
> "This can only happen in multi-threaded programs, when two threads
> race to allocate the same file descriptor number.  To avoid the
> possibility of this race, do not use @code{close} to close the
> underlying file descriptor for a @code{FILE}; either use
> @code{freopen} while the file is still open, or use @code{open} and
> then @code{dup2} to install the new file descriptor."

Ack, I will add it as well.
diff mbox

Patch

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