From patchwork Fri May 5 15:09:14 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: 20295 Received: (qmail 55699 invoked by alias); 5 May 2017 15:09:25 -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 55112 invoked by uid 89); 5 May 2017 15:09:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.6 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=orientation, 21398, 2291 X-HELO: mail-qk0-f180.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:date:message-id; bh=3P1+qg8DNbiPrK2aATJKdbE7N9lo2HMSoKZEALC2UY4=; b=jgcik9WnyoNINkRLKUzn7Xy1jq14IUsYJsQMJa2GjkrNghnPH78mkzKOcd2Ea84+Dg 1cfGA6+j4p2rwC/i+GXMt7wGOpYOESvYbqgQPPAwzO4jZQ0ZLIxPk9dyi3V1s+WdRK5i 0eRGPHA7aRqJDQB6YVv1ZOkpxTFgo6OinXLTk+TuDPOBKUXnAd8CFCN7WEUAuDfVnK81 qTi/itUbAgaY/2q0NtVoCOcFpv7ZG8u79QvoxTMToqKEMKOzC7ltgLJxLsvuvqyuLok0 W2puRhZQ9unfOZYf/xOg0cyBDhtA6hrrihKmmzKFEaT34Hs7IYQKfyeBr1OU7E7fZlIT fN+Q== X-Gm-Message-State: AN3rC/7WbMxMKnR+Mtc4UXqWAkXMmNimGMRW2cXohEdLdgPhplAUAgYt P8kcjv29P0u/8XYLQGQv8g== X-Received: by 10.55.165.196 with SMTP id o187mr12949684qke.88.1493996961052; Fri, 05 May 2017 08:09:21 -0700 (PDT) From: Adhemerval Zanella To: libc-alpha@sourceware.org Subject: [PATCH] libio: Avoid dup already opened file descriptor [BZ#21393] Date: Fri, 5 May 2017 12:09:14 -0300 Message-Id: <1493996954-14556-1-git-send-email-adhemerval.zanella@linaro.org> 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 #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