libio: Avoid dup already opened file descriptor [BZ#21393]
Commit Message
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
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
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).
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
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
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().
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.
@@ -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);
@@ -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);
@@ -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>
@@ -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