From patchwork Mon Mar 10 12:23:06 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Siddhesh Poyarekar X-Patchwork-Id: 22 Return-Path: X-Original-To: siddhesh@wilcox.dreamhost.com Delivered-To: siddhesh@wilcox.dreamhost.com Received: from homiemail-mx23.g.dreamhost.com (caibbdcaabja.dreamhost.com [208.113.200.190]) by wilcox.dreamhost.com (Postfix) with ESMTP id F3E33360198 for ; Mon, 10 Mar 2014 05:22:38 -0700 (PDT) Received: by homiemail-mx23.g.dreamhost.com (Postfix, from userid 14307373) id A29B162E72D96; Mon, 10 Mar 2014 05:22:38 -0700 (PDT) X-Original-To: glibc@patchwork.siddhesh.in Delivered-To: x14307373@homiemail-mx23.g.dreamhost.com Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by homiemail-mx23.g.dreamhost.com (Postfix) with ESMTPS id 8517862E72DBD for ; Mon, 10 Mar 2014 05:22:38 -0700 (PDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:cc:subject:message-id :mime-version:content-type; q=dns; s=default; b=Zt9Ay8UV6mTf7xng pKin95C0NL01BaEUzBtTxhZlRzIz0LQDnCcqfaHlQlA1jPPtvoYoVQekqir5yhUW 4DqCx95uL2eadM4QqbkZ3WxRzH3QTrg9ej1yyy4TY64zBFo1MkGpFd7jzdEGbib8 KnDnXvz+vyCh426BHdiGDRvsATU= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:cc:subject:message-id :mime-version:content-type; s=default; bh=3CCMyPWl2os8S+rnbcte0F 1oZHM=; b=sfujVS74IMYuCs07u8XFqJnGCwcAh0Xy9rn6DVPGr17VFTSUm9onU4 90obJNMwcSMylvP8+WDFWwdgqZKwVMpF+Ts9PM0+AxqnlFwsO0a76S1zeo9PHaDs JsGmIX2Oo5J8E91ZC1SMQbfDcKIZgSk8lRpIe9b/pFT14vjuoRKLA= Received: (qmail 11220 invoked by alias); 10 Mar 2014 12:22:36 -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 11206 invoked by uid 89); 10 Mar 2014 12:22:36 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.8 required=5.0 tests=AWL, BAYES_00, SPF_HELO_PASS, SPF_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Date: Mon, 10 Mar 2014 17:53:06 +0530 From: Siddhesh Poyarekar To: libc-alpha@sourceware.org Cc: Rich Felker Subject: [PATCH] Never cache offset when the stream handle is not active (BZ #16680) Message-ID: <20140310122306.GF1656@spoyarek.pnq.redhat.com> MIME-Version: 1.0 Content-Disposition: inline User-Agent: Mutt/1.5.22.1-rc1 (2013-10-16) X-DH-Original-To: glibc@patchwork.siddhesh.in Hi, The ftell implementation was made conservative to ensure that incorrectly cached offsets never affect it. However, this causes problems for append mode when a file stream is rewound. For a and a+ mode files in read mode, rewinding the stream should result in ftell returning 0 as the offset, but without caching, it just assumes that the file offset is the end of file (as opposed to SEEK_CUR, since rewind correctly sets it). Now I couldn't find anything in POSIX that specifies the stream position after rewind() for a file opened in 'a' mode, but for 'a+' mode it should be set to 0. For 'a' mode too, it probably makes sense to keep it set to 0 in the interest of retaining old behavior. The best way to fix this would be to avoid caching the offset before the file handle is active. With this change, the only time the offset cache is not reliable is when the file is writing in any of the append modes. I have added a test case to the active-ftell-handle test to detect this bug and verify that it is fixed. I have also verified that there are no regressions in the testsuite. Siddhesh [BZ #16680] * libio/fileops.c (_IO_file_open): Don't change offset cache. (do_ftell): Use cached offset when available and when file is not in append mode. * libio/wfileops.c (do_ftell_wide): Likewise. * libio/tst-ftell-active-handler.c (do_rewind_test): New test case. (do_one_test): Call it. (do_ftell_test): Fix test output. --- libio/fileops.c | 31 +++--------- libio/tst-ftell-active-handler.c | 104 ++++++++++++++++++++++++++++++++++++++- libio/wfileops.c | 25 +++------- 3 files changed, 117 insertions(+), 43 deletions(-) diff --git a/libio/fileops.c b/libio/fileops.c index 2e7bc8d..dbae602 100644 --- a/libio/fileops.c +++ b/libio/fileops.c @@ -232,13 +232,6 @@ _IO_file_open (fp, filename, posix_mode, prot, read_write, is32not64) return NULL; fp->_fileno = fdesc; _IO_mask_flags (fp, read_write,_IO_NO_READS+_IO_NO_WRITES+_IO_IS_APPENDING); - if ((read_write & _IO_IS_APPENDING) && (read_write & _IO_NO_READS)) - if (_IO_SEEKOFF (fp, (_IO_off64_t)0, _IO_seek_end, _IOS_INPUT|_IOS_OUTPUT) - == _IO_pos_BAD && errno != ESPIPE) - { - close_not_cancel (fdesc); - return NULL; - } _IO_link_in ((struct _IO_FILE_plus *) fp); return fp; } @@ -974,29 +967,19 @@ do_ftell (_IO_FILE *fp) bool was_writing = (fp->_IO_write_ptr > fp->_IO_write_base || _IO_in_put_mode (fp)); + bool append_mode = (fp->_flags & _IO_IS_APPENDING) == _IO_IS_APPENDING; + /* Adjust for unflushed data. */ if (!was_writing) result -= fp->_IO_read_end - fp->_IO_read_ptr; else result += fp->_IO_write_ptr - fp->_IO_read_end; - /* It is safe to use the cached offset when available if there is - unbuffered data (indicating that the file handle is active) and the - handle is not for a file open in a+ mode. The latter condition is - because there could be a scenario where there is a switch from read - mode to write mode using an fseek to an arbitrary position. In this - case, there would be unbuffered data due to be appended to the end of - the file, but the offset may not necessarily be the end of the - file. It is fine to use the cached offset when the a+ stream is in - read mode though, since the offset is maintained correctly in that - case. Note that this is not a comprehensive set of cases when the - offset is reliable. The offset may be reliable even in some cases - where there is no unflushed input and the handle is active, but it's - just that we don't have a way to identify that condition reliably. */ - use_cached_offset = (result != 0 && fp->_offset != _IO_pos_BAD - && ((fp->_flags & (_IO_IS_APPENDING | _IO_NO_READS)) - == (_IO_IS_APPENDING | _IO_NO_READS) - && was_writing)); + /* It should be safe to use the cached offset if it is available and we + are not writing in a+ mode. Any attempt to set the offset when the + stream handle is not active, is a bug. */ + use_cached_offset = (fp->_offset != _IO_pos_BAD + && !(was_writing && append_mode)); } if (use_cached_offset) diff --git a/libio/tst-ftell-active-handler.c b/libio/tst-ftell-active-handler.c index 5d5fc26..4168c54 100644 --- a/libio/tst-ftell-active-handler.c +++ b/libio/tst-ftell-active-handler.c @@ -88,6 +88,107 @@ static size_t file_len; typedef int (*fputs_func_t) (const void *data, FILE *fp); fputs_func_t fputs_func; +/* Test that ftell output after a rewind is correct. */ +static int +do_rewind_test (const char *filename) +{ + int ret = 0; + struct test + { + const char *mode; + int fd_mode; + size_t old_off; + size_t new_off; + } test_modes[] = { + {"w", O_WRONLY, 0, data_len}, + {"w+", O_RDWR, 0, data_len}, + {"r+", O_RDWR, 0, data_len}, + /* The new offsets for 'a' and 'a+' modes have to factor in the + previous writes since they compulsorily append to the end of the + file. */ + {"a", O_WRONLY, 0, 3 * data_len}, + {"a+", O_RDWR, 0, 4 * data_len}, + }; + + /* Empty the file before the test so that our offsets are simple to + calculate. */ + FILE *fp = fopen (filename, "w"); + if (fp == NULL) + { + printf ("Failed to open file for emptying\n"); + return 1; + } + fclose (fp); + + for (int j = 0; j < 2; j++) + { + for (int i = 0; i < sizeof (test_modes) / sizeof (struct test); i++) + { + FILE *fp; + int fd; + int fileret; + + printf ("\trewind: %s (file, \"%s\"): ", j == 0 ? "fdopen" : "fopen", + test_modes[i].mode); + + if (j == 0) + fileret = get_handles_fdopen (filename, fd, fp, + test_modes[i].fd_mode, + test_modes[i].mode); + else + fileret = get_handles_fopen (filename, fd, fp, test_modes[i].mode); + + if (fileret != 0) + return fileret; + + /* Write some content to the file, rewind and ensure that the ftell + output after the rewind is 0. POSIX does not specify what the + behavior is when a file is rewound in 'a' mode, so we retain + current behavior, which is to keep the 0 offset. */ + size_t written = fputs_func (data, fp); + + if (written == EOF) + { + printf ("fputs[1] failed to write data\n"); + ret |= 1; + } + + rewind (fp); + long offset = ftell (fp); + + if (offset != test_modes[i].old_off) + { + printf ("Incorrect old offset. Expected %zu, but got %ld, ", + test_modes[i].old_off, offset); + ret |= 1; + } + else + printf ("old offset = %ld, ", offset); + + written = fputs_func (data, fp); + + if (written == EOF) + { + printf ("fputs[1] failed to write data\n"); + ret |= 1; + } + + /* After this write, the offset in append modes should factor in the + implicit lseek to the end of file. */ + offset = ftell (fp); + if (offset != test_modes[i].new_off) + { + printf ("Incorrect new offset. Expected %zu, but got %ld\n", + test_modes[i].new_off, offset); + ret |= 1; + } + else + printf ("new offset = %ld\n", offset); + } + } + return ret; +} + /* Test that the value of ftell is not cached when the stream handle is not active. */ static int @@ -157,7 +258,7 @@ do_ftell_test (const char *filename) if (off != test_modes[i].new_off) { printf ("Incorrect new offset. Expected %zu but got %ld\n", - test_modes[i].old_off, off); + test_modes[i].new_off, off); ret |= 1; } else @@ -322,6 +423,7 @@ do_one_test (const char *filename) ret |= do_ftell_test (filename); ret |= do_write_test (filename); ret |= do_append_test (filename); + ret |= do_rewind_test (filename); return ret; } diff --git a/libio/wfileops.c b/libio/wfileops.c index 8b2e108..eaeb527 100644 --- a/libio/wfileops.c +++ b/libio/wfileops.c @@ -704,24 +704,13 @@ do_ftell_wide (_IO_FILE *fp) offset -= fp->_IO_read_end - fp->_IO_write_ptr; } - /* It is safe to use the cached offset when available if there is - unbuffered data (indicating that the file handle is active) and - the handle is not for a file open in a+ mode. The latter - condition is because there could be a scenario where there is a - switch from read mode to write mode using an fseek to an arbitrary - position. In this case, there would be unbuffered data due to be - appended to the end of the file, but the offset may not - necessarily be the end of the file. It is fine to use the cached - offset when the a+ stream is in read mode though, since the offset - is maintained correctly in that case. Note that this is not a - comprehensive set of cases when the offset is reliable. The - offset may be reliable even in some cases where there is no - unflushed input and the handle is active, but it's just that we - don't have a way to identify that condition reliably. */ - use_cached_offset = (offset != 0 && fp->_offset != _IO_pos_BAD - && ((fp->_flags & (_IO_IS_APPENDING | _IO_NO_READS)) - == (_IO_IS_APPENDING | _IO_NO_READS) - && was_writing)); + bool append_mode = (fp->_flags & _IO_IS_APPENDING) == _IO_IS_APPENDING; + + /* It should be safe to use the cached offset if it is available and we + are not writing in a+ mode. Any attempt to set the offset when the + stream handle is not active, is a bug. */ + use_cached_offset = (fp->_offset != _IO_pos_BAD + && !(was_writing && append_mode)); } if (use_cached_offset)