From patchwork Mon May 22 17:25:22 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella Netto X-Patchwork-Id: 20532 Received: (qmail 129782 invoked by alias); 22 May 2017 17:25:30 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 129762 invoked by uid 89); 22 May 2017 17:25:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.9 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, RCVD_IN_SORBS_SPAM, SPF_PASS autolearn=ham version=3.3.2 spammy=2284 X-HELO: mail-qk0-f181.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=odHHQICOkbt7x0p9RL+GJCeA8Mc9zzKQbyeSpxSiAd0=; b=e0PUjI4F1POBI9hMSwF+4+GeGovdt+Vuw1YxvauKppItaWpGbaupw4CjZc/W/yYbvw V7K0WGmov0m+q1w1lYRt/0OyS5Htv7wloan5BpOT0Yq4k52TVITZkrvDc3L25nwwZJnh dKd/mlWvOSiqce4E2/QP7f4OhBWooY12t90StTImFdjhHniaeeysgpnGaNCFDKfztme0 sgurNhPuc1KwPHYVK+uqpKiP4vb6h22wcdjv2xeIEqhqcpm5lEeVy3P9RFrQaAXtbxKe dr1cKsTiO76/5DRf4/SuvQO7ToUrtyvGu5AAWF9vfQilqzk1S1yRLthYescx/fJaEF04 CFAQ== X-Gm-Message-State: AODbwcA7/soE25vjCU9xCRuxPowKpu2eYgVrAmLuc9FAaAx21IEXl5dl +UQO5QSrxbzASuBbvZHevg== X-Received: by 10.55.73.133 with SMTP id w127mr23739741qka.4.1495473928159; Mon, 22 May 2017 10:25:28 -0700 (PDT) Subject: Re: [PATCH] libio: Avoid dup already opened file descriptor [BZ#21393] To: Siddhesh Poyarekar , libc-alpha@sourceware.org Cc: Carlos O'Donell References: <1493996954-14556-1-git-send-email-adhemerval.zanella@linaro.org> <352a1abe-d4ba-322f-ed9d-158f7cd56e8e@gotplt.org> From: Adhemerval Zanella Message-ID: Date: Mon, 22 May 2017 14:25:22 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: <352a1abe-d4ba-322f-ed9d-158f7cd56e8e@gotplt.org> 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 #include -static int -do_test (void) +#include +#include + +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 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