This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
Hi, So here's take two at fixing the problem with fdopen/fopen and ftell in append modes. When fdopen attaches to a file descriptor, it sets the offset at that point, which may be deceptive. It gives later ftell() calls the idea that we know the offset since the handle was active, which is not true since one could have changed the underlying file offset before the handle became active, i.e. had an operation performed on it. Additionally, with a+ mode, fopen cannot tell the correct file position if it is initially in write mode and the buffer has not been flushed, since even an lseek(SEEK_CUR) at this stage will give the wrong information. In fact, this is true for all append mode operations, but fopen gets away with it with "a" because it seeks to SEEK_END when it opens the file. So the fix here is to avoid doing any seek at fdopen time to allow ftell() to get the real offset when needed, and for append modes to always SEEK_END when the stream is being written to. All this is necessary only the first time, i.e. just after the file has been open/attached and the buffer not yet flushed. Subsequent queries should always be correct since applications are required to flush or seek to the desired position when switching between reading or writing, both of which sync up the offset. There is also a test case that verifies that both append modes work correctly with fdopen and fopen and that both write modes and the r+ mode work correctly with fdopen. The test case verifies I/O in both regular and wide modes. Siddhesh [BZ #16532] * libio/tst-fopen-append-update.c: New test case. * libio/Makefile (tests): Add it. * libio/fileops.c (_IO_new_file_seekoff): Seek to end in ftell for streams in append mode. * libio/wfileops.c (_IO_wfile_seekoff): Likewise. * libio/iofdopen.c (_IO_new_fdopen): Don't use _IO_fd_attach. --- libio/Makefile | 2 +- libio/fileops.c | 16 ++- libio/iofdopen.c | 18 ++- libio/tst-fopen-append-update.c | 275 ++++++++++++++++++++++++++++++++++++++++ libio/wfileops.c | 11 ++ 5 files changed, 310 insertions(+), 12 deletions(-) create mode 100644 libio/tst-fopen-append-update.c diff --git a/libio/Makefile b/libio/Makefile index 747a779..640b475 100644 --- a/libio/Makefile +++ b/libio/Makefile @@ -60,7 +60,7 @@ tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc \ tst-wmemstream1 tst-wmemstream2 \ bug-memstream1 bug-wmemstream1 \ tst-setvbuf1 tst-popen1 tst-fgetwc bug-wsetpos tst-fseek \ - tst-fwrite-error tst-ftell-partial-wide + tst-fwrite-error tst-ftell-partial-wide tst-fopen-append-update ifeq (yes,$(build-shared)) # Add test-fopenloc only if shared library is enabled since it depends on # shared localedata objects. diff --git a/libio/fileops.c b/libio/fileops.c index a3499be..829634f 100644 --- a/libio/fileops.c +++ b/libio/fileops.c @@ -949,7 +949,21 @@ _IO_new_file_seekoff (fp, offset, dir, mode) || _IO_in_put_mode (fp)); if (mode == 0) - dir = _IO_seek_cur, offset = 0; /* Don't move any pointers. */ + { + dir = _IO_seek_cur, offset = 0; /* Don't move any pointers. */ + + /* When we write in append mode for the first time, the offset may still + be set to the beginning of the file because we have not flushed the + buffer. Send the offset to the end of the file, where it belongs, so + that we return the correct offset through ftell. */ + if (was_writing && (fp->_flags & _IO_IS_APPENDING) == _IO_IS_APPENDING) + { + off_t ret = _IO_SYSSEEK (fp, 0, _IO_seek_end); + if (ret == EOF) + return ret; + fp->_offset = ret; + } + } /* Flush unwritten characters. (This may do an unneeded write if we seek within the buffer. diff --git a/libio/iofdopen.c b/libio/iofdopen.c index 066ff19..6dbdcb5 100644 --- a/libio/iofdopen.c +++ b/libio/iofdopen.c @@ -141,9 +141,6 @@ _IO_new_fdopen (fd, mode) #ifdef _IO_MTSAFE_IO new_f->fp.file._lock = &new_f->lock; #endif - /* Set up initially to use the `maybe_mmap' jump tables rather than using - __fopen_maybe_mmap to do it, because we need them in place before we - call _IO_file_attach or else it will allocate a buffer immediately. */ _IO_no_init (&new_f->fp.file, 0, 0, &new_f->wd, #ifdef _G_HAVE_MMAP (use_mmap && (read_write & _IO_NO_WRITES)) @@ -155,17 +152,18 @@ _IO_new_fdopen (fd, mode) (use_mmap && (read_write & _IO_NO_WRITES)) ? &_IO_file_jumps_maybe_mmap : #endif &_IO_file_jumps; + + /* _IO_file_init unsets the file position. This is convenient because it can + then be queried when we do an ftell. We don't bother with positioning the + offset in the file or even querying what it is right now since we don't + want to interfere with any reads or writes that may be happening on the + file descriptor before it becomes active, i.e. has its first + operation. */ _IO_file_init (&new_f->fp); #if !_IO_UNIFIED_JUMPTABLES new_f->fp.vtable = NULL; #endif - if (_IO_file_attach ((_IO_FILE *) &new_f->fp, fd) == NULL) - { - _IO_setb (&new_f->fp.file, NULL, NULL, 0); - _IO_un_link (&new_f->fp); - free (new_f); - return NULL; - } + new_f->fp.file._fileno = fd; new_f->fp.file._flags &= ~_IO_DELETE_DONT_CLOSE; _IO_mask_flags (&new_f->fp.file, read_write, diff --git a/libio/tst-fopen-append-update.c b/libio/tst-fopen-append-update.c new file mode 100644 index 0000000..f1b7a10 --- /dev/null +++ b/libio/tst-fopen-append-update.c @@ -0,0 +1,275 @@ +/* Verify that ftell returns the correct value when a file is opened in + append (a) and append-update (a+) mode. + Copyright (C) 2014 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <http://www.gnu.org/licenses/>. */ + +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <errno.h> +#include <unistd.h> +#include <fcntl.h> +#include <locale.h> +#include <wchar.h> + +static int do_test (void); + +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c" + +static const void *data; +static const char *char_data = "abcdef"; +static const wchar_t *wide_data = L"abcdef"; +size_t data_len; + +typedef int (*fputs_func_t) (const void *data, FILE *fp); +fputs_func_t fputs_func; + +static int +do_write_test (const char *filename, int count, const char *mode) +{ + FILE *fp = NULL; + int ret = 0; + int m; + + printf ("fdopen (file, \"%s\"): ", mode); + + if (strchr (mode, '+') != NULL) + m = O_RDWR; + else if (strchr (mode, 'w') != NULL) + m = O_WRONLY; + else + { + printf ("Invalid mode %s\n", mode); + ret = 1; + goto out; + } + + int fd = open (filename, m, 0); + + if (fd == -1) + { + printf ("Failed to open temp file: %m\n"); + ret = 1; + goto out; + } + + fp = fdopen (fd, mode); + if (fp == NULL) + { + printf ("fdopen[%d]: %m\n", count); + ret = 1; + close (fd); + goto out; + } + + /* Move offset to COUNT from the beginning. */ + lseek (fd, count, SEEK_SET); + + /* Write some data. */ + size_t written = fputs_func (data, fp); + + if (written == EOF) + { + printf ("fputs[1] failed to write data\n"); + ret = 1; + goto out; + } + + /* Verify that the offset points to the end of the file. */ + long offset = ftell (fp); + + if (offset != count + data_len) + { + printf ("Incorrect offset. Expected %zu, but got %ld\n", + (count + data_len), offset); + + ret = 1; + goto out; + } + + printf ("offset = %ld\n", offset); + +out: + if (fp) + fclose (fp); + return ret; +} + +static int +do_append_test (const char *filename, int count, bool do_fopen, bool update) +{ + FILE *fp = NULL; + int ret = 0; + + const char *mode = update ? "a+" : "a"; + + printf ("%s (file, \"%s\"): ", do_fopen ? "fopen" : "fdopen", mode); + + if (do_fopen) + { + fp = fopen (filename, mode); + if (fp == NULL) + { + printf ("fopen[%d]: %m\n", count); + ret = 1; + goto out; + } + } + else + { + int fd = open (filename, update ? O_RDWR : O_WRONLY, 0); + + if (fd == -1) + { + printf ("Failed to open temp file: %m\n"); + ret = 1; + goto out; + } + + fp = fdopen (fd, mode); + if (fp == NULL) + { + printf ("fdopen[%d]: %m\n", count); + ret = 1; + close (fd); + goto out; + } + } + + /* Write some data. */ + size_t written = fputs_func (data, fp); + + if (written == EOF) + { + printf ("fputs[1] failed to write all data\n"); + ret = 1; + goto out; + } + + /* Verify that the offset points to the end of the file. */ + long offset = ftell (fp); + + if (offset != count * data_len) + { + printf ("Incorrect offset. Expected %zu, but got %ld\n", + (count * data_len), offset); + + ret = 1; + goto out; + } + + printf ("offset = %ld\n", offset); + +out: + if (fp) + fclose (fp); + return ret; +} + +static int +do_test (void) +{ + int ret = 0; + FILE *fp = NULL; + char *filename; + size_t written; + int fd = create_temp_file ("tst-fseek-wide-partial.out", &filename); + + if (fd == -1) + { + printf ("create_temp_file: %m\n"); + return 1; + } + + fp = fdopen (fd, "w"); + if (fp == NULL) + { + printf ("fdopen[0]: %m\n"); + close (fd); + return 1; + } + + data = char_data; + data_len = strlen (char_data); + written = fputs (data, fp); + + if (written == EOF) + { + printf ("fputs[1] failed to write data\n"); + ret = 1; + } + + fclose (fp); + if (ret) + return ret; + + /* Counter to keep track of what offset to expect in the file and also to + enumerate the tests. */ + int i = 1; + + /* Tests for regular files. */ + fputs_func = (fputs_func_t) fputs; + data = char_data; + data_len = strlen (char_data); + + /* In append mode (a), fopen and fdopen should move its offset to the end of + the file. */ + ret |= do_append_test (filename, ++i, false, false); + ret |= do_append_test (filename, ++i, true, false); + + /* In append-update mode (a+), fopen and fdopen should move its offset to the + end of the file. */ + ret |= do_append_test (filename, ++i, false, true); + ret |= do_append_test (filename, ++i, true, true); + + /* The write modes and the r+ mode should adjust correctly to the underlying + change in file offset. */ + ret |= do_write_test (filename, ++i, "w"); + ret |= do_write_test (filename, ++i, "w+"); + ret |= do_write_test (filename, ++i, "r+"); + + + /* Tests for wide files. */ + if (setlocale (LC_ALL, "en_US.UTF-8") == NULL) + { + printf ("Cannot set en_US.UTF-8 locale.\n"); + return 1; + } + i -= 3; + fputs_func = (fputs_func_t) fputws; + data = wide_data; + data_len = wcslen (wide_data); + + /* In append mode (a), fopen and fdopen should move its offset to the end of + the file. */ + ret |= do_append_test (filename, ++i, false, false); + ret |= do_append_test (filename, ++i, true, false); + + /* In append-update mode (a+), fopen and fdopen should move its offset to the + end of the file. */ + ret |= do_append_test (filename, ++i, false, true); + ret |= do_append_test (filename, ++i, true, true); + + /* The write modes and the r+ mode should adjust correctly to the underlying + change in file offset. */ + ret |= do_write_test (filename, ++i, "w"); + ret |= do_write_test (filename, ++i, "w+"); + ret |= do_write_test (filename, ++i, "r+"); + + return ret; +} diff --git a/libio/wfileops.c b/libio/wfileops.c index 9cebe77..142a6c9 100644 --- a/libio/wfileops.c +++ b/libio/wfileops.c @@ -639,6 +639,17 @@ _IO_wfile_seekoff (fp, offset, dir, mode) } dir = _IO_seek_cur, offset = 0; /* Don't move any pointers. */ + /* When we write in append mode for the first time, the offset may still + be set to the beginning of the file because we have not flushed the + buffer. Send the offset to the end of the file, where it belongs, so + that we return the correct offset through ftell. */ + if (was_writing && (fp->_flags & _IO_IS_APPENDING) == _IO_IS_APPENDING) + { + off_t ret = _IO_SYSSEEK (fp, 0, _IO_seek_end); + if (ret == EOF) + return ret; + fp->_offset = ret; + } } /* Flush unwritten characters. -- 1.8.3.1
Attachment:
pgpft2GbjhEyt.pgp
Description: PGP signature
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |