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

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

[Adhemerval Zanella]

As described in BZ#21398 (close as dup of 21393) report current
freopen implementation fails when one tries to freopen STDIN_FILENO,
STDOUT_FILENO, or STDERR_FILENO.  Although on bug report the
discussion leads to argue if a close followed by a freopen on the
standard file is a valid operation, the underlying issue is not
really the check for dup3 returned value, but rather calling it
if the returned file descriptor is equal as the input one.

So for a quality of implementation this patch avoid calling dup3
for the aforementioned case.  It also adds a dup3 error case check
for the two possible failures, with one being Linux only: EINTR and
EBUSY.  The EBUSY issue is better explained on this stackoverflow
thread [1], but in a short it is due the internal Linux
implementation which allows a race condition window for dup2 due
the logic dissociation of file descriptor allocation and actual
VFS 'install' operation.  For both outliers failures all allocated
memory is freed and a NULL FILE* is returned.

With this patch the example on BZ#21398 is now actually possible
(I used as the testcase for the bug report).  Checked on
x86_64-linux-gnu.

	[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.

[1] http://stackoverflow.com/questions/23440216/race-condition-when-using-dup2
---
 ChangeLog           |  10 +++++
 libio/freopen.c     |  22 +++++++++--
 libio/freopen64.c   |  22 +++++++++--
 libio/tst-freopen.c | 107 ++++++++++++++++++++++++++++------------------------
 4 files changed, 103 insertions(+), 58 deletions(-)

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>
-- 
2.7.4

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

[Siddhesh Poyarekar]

On Friday 05 May 2017 08:39 PM, Adhemerval Zanella wrote:
> As described in BZ#21398 (close as dup of 21393) report current
> freopen implementation fails when one tries to freopen STDIN_FILENO,
> STDOUT_FILENO, or STDERR_FILENO.  Although on bug report the
> discussion leads to argue if a close followed by a freopen on the
> standard file is a valid operation, the underlying issue is not
> really the check for dup3 returned value, but rather calling it
> if the returned file descriptor is equal as the input one.
> 
> So for a quality of implementation this patch avoid calling dup3
> for the aforementioned case.  It also adds a dup3 error case check
> for the two possible failures, with one being Linux only: EINTR and
> EBUSY.  The EBUSY issue is better explained on this stackoverflow
> thread [1], but in a short it is due the internal Linux
> implementation which allows a race condition window for dup2 due
> the logic dissociation of file descriptor allocation and actual
> VFS 'install' operation.  For both outliers failures all allocated
> memory is freed and a NULL FILE* is returned.

Carlos and I had a private conversation with Mathew Wilcox months ago
about the EBUSY issue and how it probably doesn't strictly adhere to the
current wording in POSIX.  The trouble here is that failing due to an
EBUSY could possibly cause a false negative in a number of cases where
the backing store (nfs or whatever) just isn't quick enough and
applications will then have to specifically look out for the EBUSY and
retry.

To make the behaviour strictly conforming and clean (and not have
applications change), we would have to retry repeatedly (maybe a limited
number of times) until dup3 either succeeds or fails with a different
error.  However, in the ideal world the kernel ought to do this for us.

Since 21398 would be solved by just ensuring that the old and new fd are
different to warrant closing the old fd before associating the new one,
I would suggest dropping the error check for dup3 for now and then deal
with the conformity issue separately.

Carlos, what do you think?

Siddhesh

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

[Adhemerval Zanella]

On 18/05/2017 16:03, Siddhesh Poyarekar wrote:
> On Friday 05 May 2017 08:39 PM, Adhemerval Zanella wrote:
>> As described in BZ#21398 (close as dup of 21393) report current
>> freopen implementation fails when one tries to freopen STDIN_FILENO,
>> STDOUT_FILENO, or STDERR_FILENO.  Although on bug report the
>> discussion leads to argue if a close followed by a freopen on the
>> standard file is a valid operation, the underlying issue is not
>> really the check for dup3 returned value, but rather calling it
>> if the returned file descriptor is equal as the input one.
>>
>> So for a quality of implementation this patch avoid calling dup3
>> for the aforementioned case.  It also adds a dup3 error case check
>> for the two possible failures, with one being Linux only: EINTR and
>> EBUSY.  The EBUSY issue is better explained on this stackoverflow
>> thread [1], but in a short it is due the internal Linux
>> implementation which allows a race condition window for dup2 due
>> the logic dissociation of file descriptor allocation and actual
>> VFS 'install' operation.  For both outliers failures all allocated
>> memory is freed and a NULL FILE* is returned.
> 
> Carlos and I had a private conversation with Mathew Wilcox months ago
> about the EBUSY issue and how it probably doesn't strictly adhere to the
> current wording in POSIX.  The trouble here is that failing due to an
> EBUSY could possibly cause a false negative in a number of cases where
> the backing store (nfs or whatever) just isn't quick enough and
> applications will then have to specifically look out for the EBUSY and
> retry.
> 
> To make the behaviour strictly conforming and clean (and not have
> applications change), we would have to retry repeatedly (maybe a limited
> number of times) until dup3 either succeeds or fails with a different
> error.  However, in the ideal world the kernel ought to do this for us.
> 
> Since 21398 would be solved by just ensuring that the old and new fd are
> different to warrant closing the old fd before associating the new one,
> I would suggest dropping the error check for dup3 for now and then deal
> with the conformity issue separately.
> 
> Carlos, what do you think?
> 
> Siddhesh
> 

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.

Also I think for proper underlying kernel support it would potentially
require dup3 to indefinitely block (which still be even more troublesome due
underlying FS like NFS that might timeout).  I would require to extend dup3
semantic, possible making it a cancelable entrypoint, and imho I think kernel
won't take this path lightly.

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

[Siddhesh Poyarekar]

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.

> Also I think for proper underlying kernel support it would potentially
> require dup3 to indefinitely block (which still be even more troublesome due
> underlying FS like NFS that might timeout).  I would require to extend dup3
> semantic, possible making it a cancelable entrypoint, and imho I think kernel
> won't take this path lightly.

They don't.

Siddhesh

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

[Adhemerval Zanella]

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

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

[Siddhesh Poyarekar]

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

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

[Adhemerval Zanella]

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).

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

[Siddhesh Poyarekar]

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

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

[Zack Weinberg]

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

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

[Adhemerval Zanella]

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().

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

[Adhemerval Zanella]

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.