This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v2 2/2] libio: Fix seek-past-end returned size for open_{w}memstream (BZ#15298)
Ping.
On 08/08/2016 17:43, Adhemerval Zanella wrote:
> This patch fixes how open_{w}memstream updates the returned buffer
> size on a fclose/fflush operation. POSIX states [1]:
>
> "After a successful fflush() or fclose() [...] the variable pointed
> to by sizep shall contain the smaller of the current buffer length and
> the number of bytes for open_memstream(), or the number of wide characters
> for open_wmemstream(), between the beginning of the buffer and the current
> file position indicator."
>
> Current GLIBC behavior returns the seek position even if there is no previous
> write operation. To correctly report the buffer size the implementation must
> track both the buffer positiob and currently bytes written. However internal
> _IO_write_ptr is updated on both write and seek operations.
>
> This patch fixes it by adding two new internal fields to keep track of both
> previous and next position after a write operation. It allows the sync and
> close callbacks to know if a write has occurred based on previous and
> current _IO_write_ptr value.
>
> Tested on x86_64.
>
> [BZ #15298]
> * libio/memstream.c (_IO_FILE_memstream): Add prevwriteptr and
> seekwriteptr fields.
> (_IO_mem_seekoff): New function.
> (_IO_mem_jumps): Use _IO_mem_seekoff.
> (__open_memstream): Initialize prevwriteptr and seekwriteptr.
> (_IO_mem_sync): Update sizeloc based on written bytes instead of buffer
> current position.
> (_IO_mem_finish): Likewise.
> * libio/wmemstream.c (_IO_FILE_wmemstream): Add prevwriteptr and
> seekwriteptr fields.
> (_IO_wmem_seekoff): New function.
> (_IO_wmem_jumps): Use _IO_mem_seekoff.
> (__open_wmemstream): Initialize prevwriteptr and seekwriteptr.
> (_IO_mem_wsync): Update sizeloc based on written bytes instead of buffer
> current position.
> (_IO_mem_wfinish): Likewise.
> * libio/tst-memstream3.c (do_test_bz15298): New function.
> (do_test_bz18241): Check for expected size after a fseek followed by a
> fflush.
> (do_test): Call do_test_bz15298.
>
> [1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/open_memstream.html
> ---
> ChangeLog | 22 +++++++++++++++
> libio/memstream.c | 52 ++++++++++++++++++++++++++++++++----
> libio/tst-memstream3.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++---
> libio/wmemstream.c | 54 ++++++++++++++++++++++++++++++++-----
> 4 files changed, 185 insertions(+), 15 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index 28d9012..ea16b48 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,27 @@
> 2016-08-05 Adhemerval Zanella <adhemerval.zanella@linaro.org>
>
> + [BZ #15298]
> + * libio/memstream.c (_IO_FILE_memstream): Add prevwriteptr and
> + seekwriteptr fields.
> + (_IO_mem_seekoff): New function.
> + (_IO_mem_jumps): Use _IO_mem_seekoff.
> + (__open_memstream): Initialize prevwriteptr and seekwriteptr.
> + (_IO_mem_sync): Update sizeloc based on written bytes instead of buffer
> + current position.
> + (_IO_mem_finish): Likewise.
> + * libio/wmemstream.c (_IO_FILE_wmemstream): Add prevwriteptr and
> + seekwriteptr fields.
> + (_IO_wmem_seekoff): New function.
> + (_IO_wmem_jumps): Use _IO_mem_seekoff.
> + (__open_wmemstream): Initialize prevwriteptr and seekwriteptr.
> + (_IO_mem_wsync): Update sizeloc based on written bytes instead of buffer
> + current position.
> + (_IO_mem_wfinish): Likewise.
> + * libio/tst-memstream3.c (do_test_bz15298): New function.
> + (do_test_bz18241): Check for expected size after a fseek followed by a
> + fflush.
> + (do_test): Call do_test_bz15298.
> +
> [BZ #18241]
> [BZ #20181]
> * libio/Makefile (test): Add tst-memstream3 and tst-wmemstream3.
> diff --git a/libio/memstream.c b/libio/memstream.c
> index f1e8d58..cb3af9a 100644
> --- a/libio/memstream.c
> +++ b/libio/memstream.c
> @@ -26,12 +26,15 @@ struct _IO_FILE_memstream
> _IO_strfile _sf;
> char **bufloc;
> _IO_size_t *sizeloc;
> + char *prevwriteptr;
> + char *seekwriteptr;
> };
>
>
> static int _IO_mem_sync (_IO_FILE* fp) __THROW;
> static void _IO_mem_finish (_IO_FILE* fp, int) __THROW;
> -
> +static _IO_off64_t _IO_mem_seekoff (_IO_FILE *fp, _IO_off64_t offset,
> + int dir, int mode) __THROW;
>
> static const struct _IO_jump_t _IO_mem_jumps libio_vtable =
> {
> @@ -43,7 +46,7 @@ static const struct _IO_jump_t _IO_mem_jumps libio_vtable =
> JUMP_INIT (pbackfail, _IO_str_pbackfail),
> JUMP_INIT (xsputn, _IO_default_xsputn),
> JUMP_INIT (xsgetn, _IO_default_xsgetn),
> - JUMP_INIT (seekoff, _IO_str_seekoff),
> + JUMP_INIT (seekoff, _IO_mem_seekoff),
> JUMP_INIT (seekpos, _IO_default_seekpos),
> JUMP_INIT (setbuf, _IO_default_setbuf),
> JUMP_INIT (sync, _IO_mem_sync),
> @@ -95,6 +98,24 @@ __open_memstream (char **bufloc, _IO_size_t *sizeloc)
>
> new_f->fp.bufloc = bufloc;
> new_f->fp.sizeloc = sizeloc;
> + /* To correctly report the buffer size the implementation must track both
> + the buffer size and currently bytes written. However _IO_write_ptr is
> + updated on both write and seek operations. So to track current written
> + bytes two fields are used:
> +
> + - prevwriteptr: track previous _IO_write_ptr before a seek operation on
> + the stream.
> + - seekwriteptr: track resulted _IO_write_ptr after a seek operation on
> + the stream.
> +
> + Also, prevwriteptr is only updated iff _IO_write_ptr changed over calls
> + (meaning that a write operation occured)
> +
> + So final buffer size is based on current _IO_write_ptr only if
> + its value is different than seekwriteptr, otherwise it uses the old
> + _IO_write_ptr value before seek operation (prevwriteptr). */
> + new_f->fp.prevwriteptr = new_f->fp.seekwriteptr =
> + new_f->fp._sf._sbf._f._IO_write_ptr;
>
> return (_IO_FILE *) &new_f->fp._sf._sbf;
> }
> @@ -114,7 +135,9 @@ _IO_mem_sync (_IO_FILE *fp)
> }
>
> *mp->bufloc = fp->_IO_write_base;
> - *mp->sizeloc = fp->_IO_write_ptr - fp->_IO_write_base;
> + char *ptr = (fp->_IO_write_ptr == mp->seekwriteptr)
> + ? mp->prevwriteptr : fp->_IO_write_ptr;
> + *mp->sizeloc = ptr - fp->_IO_write_base;
>
> return 0;
> }
> @@ -129,11 +152,30 @@ _IO_mem_finish (_IO_FILE *fp, int dummy)
> fp->_IO_write_ptr - fp->_IO_write_base + 1);
> if (*mp->bufloc != NULL)
> {
> - (*mp->bufloc)[fp->_IO_write_ptr - fp->_IO_write_base] = '\0';
> - *mp->sizeloc = fp->_IO_write_ptr - fp->_IO_write_base;
> + size_t len;
> + if (fp->_IO_write_ptr == mp->seekwriteptr)
> + len = mp->prevwriteptr - fp->_IO_write_base;
> + else
> + {
> + /* An '\0' should be appended iff a write operation ocurred. */
> + len = fp->_IO_write_ptr - fp->_IO_write_base;
> + (*mp->bufloc)[len] = '\0';
> + }
> + *mp->sizeloc = len;
>
> fp->_IO_buf_base = NULL;
> }
>
> _IO_str_finish (fp, 0);
> }
> +
> +static _IO_off64_t
> +_IO_mem_seekoff (_IO_FILE *fp, _IO_off64_t offset, int dir, int mode)
> +{
> + struct _IO_FILE_memstream *mp = (struct _IO_FILE_memstream *) fp;
> + if (fp->_IO_write_ptr != mp->seekwriteptr)
> + mp->prevwriteptr = fp->_IO_write_ptr;
> + _IO_off64_t ret = _IO_str_seekoff (fp, offset, dir, mode);
> + mp->seekwriteptr = fp->_IO_write_ptr;
> + return ret;
> +}
> diff --git a/libio/tst-memstream3.c b/libio/tst-memstream3.c
> index 34b04e5..65c95ad 100644
> --- a/libio/tst-memstream3.c
> +++ b/libio/tst-memstream3.c
> @@ -57,6 +57,69 @@ error_printf (int line, const char *fmt, ...)
> { error_printf(__LINE__, __VA_ARGS__); return 1; }
>
> static int
> +do_test_bz15298 (void)
> +{
> + CHAR_T *buf;
> + size_t size;
> + size_t ret;
> +
> + FILE *fp = OPEN_MEMSTREAM (&buf, &size);
> + if (fp == NULL)
> + ERROR_RET1 ("%s failed\n", S(OPEN_MEMSTREAM));
> +
> + /* Move internal position but do not write any bytes. Final size should
> + be 0. */
> + if (fseek (fp, 10, SEEK_SET) == -1)
> + ERROR_RET1 ("fseek failed (errno = %d)\n", errno);
> + if (fseek (fp, 20, SEEK_CUR) == -1)
> + ERROR_RET1 ("fseek failed (errno = %d)\n", errno);
> + if (fseek (fp, 30, SEEK_CUR) == -1)
> + ERROR_RET1 ("fseek failed (errno = %d)\n", errno);
> + if (fflush (fp) != 0)
> + ERROR_RET1 ("fflush failed (errno = %d)\n", errno);
> + if (size != 0)
> + ERROR_RET1 ("size != 0 (got %zu)\n", size);
> +
> + /* Now write some bytes and change internal position. Final size should
> + be based on written bytes. */
> + if (fseek (fp, 0, SEEK_SET) == -1)
> + ERROR_RET1 ("fseek failed (errno = %d)\n", errno);
> + if ((ret = FWRITE (W("abc"), 1, 3, fp)) != 3)
> + ERROR_RET1 ("%s failed (errno = %d)\n", S(FWRITE), errno);
> + if (fseek (fp, 20, SEEK_CUR) == -1)
> + ERROR_RET1 ("fseek failed (errno = %d)\n", errno);
> + if (fseek (fp, 30, SEEK_CUR) == -1)
> + ERROR_RET1 ("fseek failed (errno = %d)\n", errno);
> + if (fflush (fp) != 0)
> + ERROR_RET1 ("fflush failed (errno = %d)\n", errno);
> + if (size != 3)
> + ERROR_RET1 ("size != 3 (got %zu)\n", size);
> +
> + /* Finally set position, write some bytes and change position again. Final
> + size should be based again on write position. */
> + size_t offset = 2048;
> + if (fseek (fp, offset, SEEK_SET) == -1)
> + ERROR_RET1 ("fseek failed (errno = %d)\n", errno);
> + if ((ret = FWRITE (W("abc"), 1, 3, fp)) != 3)
> + ERROR_RET1 ("%s failed (errno = %d)\n", S(FWRITE), errno);
> + if (fseek (fp, 20, SEEK_CUR) == -1)
> + ERROR_RET1 ("fseek failed (errno = %d)\n", errno);
> + if (fseek (fp, 30, SEEK_CUR) == -1)
> + ERROR_RET1 ("fseek failed (errno = %d)\n", errno);
> + if (fflush (fp) != 0)
> + ERROR_RET1 ("fflush failed (errno = %d)\n", errno);
> + if (size != offset + 3)
> + ERROR_RET1 ("size != %zu (got %zu)\n", offset + 3, size);
> +
> + if (fclose (fp) != 0)
> + ERROR_RET1 ("fclose failed (errno = %d\n", errno);
> +
> + free (buf);
> +
> + return 0;
> +}
> +
> +static int
> do_test_bz18241 (void)
> {
> CHAR_T *buf;
> @@ -124,15 +187,17 @@ do_test_bz20181 (void)
> if (fflush (fp) != 0)
> ERROR_RET1 ("fflush failed (errno = %d)\n", errno);
>
> - /* Avoid truncating the buffer on close. */
> + /* fseek updates the internal buffer, but open_memstream should set the
> + size to smaller of the buffer size and number of bytes written. Since
> + it was written just character ('z') final size should be 1. */
> if (fseek (fp, 3, SEEK_SET) != 0)
> ERROR_RET1 ("fseek failed (errno = %d)\n", errno);
>
> if (fclose (fp) != 0)
> ERROR_RET1 ("fclose failed (errno = %d\n", errno);
>
> - if (size != 3)
> - ERROR_RET1 ("size != 3\n");
> + if (size != 1)
> + ERROR_RET1 ("size != 1 (got %zu)\n", size);
>
> if (buf[0] != W('z')
> || buf[1] != W('b')
> @@ -155,6 +220,7 @@ do_test (void)
>
> mcheck_pedantic (mcheck_abort);
>
> + ret += do_test_bz15298 ();
> ret += do_test_bz18241 ();
> ret += do_test_bz20181 ();
>
> diff --git a/libio/wmemstream.c b/libio/wmemstream.c
> index fd01be0..2f8f22c 100644
> --- a/libio/wmemstream.c
> +++ b/libio/wmemstream.c
> @@ -27,12 +27,15 @@ struct _IO_FILE_wmemstream
> _IO_strfile _sf;
> wchar_t **bufloc;
> _IO_size_t *sizeloc;
> + wchar_t *prevwriteptr;
> + wchar_t *seekwriteptr;
> };
>
>
> static int _IO_wmem_sync (_IO_FILE* fp) __THROW;
> static void _IO_wmem_finish (_IO_FILE* fp, int) __THROW;
> -
> +static _IO_off64_t _IO_wmem_seekoff (_IO_FILE *fp, _IO_off64_t offset,
> + int dir, int mode) __THROW;
>
> static const struct _IO_jump_t _IO_wmem_jumps libio_vtable =
> {
> @@ -44,7 +47,7 @@ static const struct _IO_jump_t _IO_wmem_jumps libio_vtable =
> JUMP_INIT (pbackfail, (_IO_pbackfail_t) _IO_wstr_pbackfail),
> JUMP_INIT (xsputn, _IO_wdefault_xsputn),
> JUMP_INIT (xsgetn, _IO_wdefault_xsgetn),
> - JUMP_INIT (seekoff, _IO_wstr_seekoff),
> + JUMP_INIT (seekoff, _IO_wmem_seekoff),
> JUMP_INIT (seekpos, _IO_default_seekpos),
> JUMP_INIT (setbuf, _IO_default_setbuf),
> JUMP_INIT (sync, _IO_wmem_sync),
> @@ -97,6 +100,24 @@ open_wmemstream (wchar_t **bufloc, _IO_size_t *sizeloc)
>
> new_f->fp.bufloc = bufloc;
> new_f->fp.sizeloc = sizeloc;
> + /* To correctly report the buffer size the implementation must track both
> + the buffer size and currently bytes written. However _IO_write_ptr is
> + updated on both write and seek operations. So to track current written
> + bytes two fields are used:
> +
> + - prevwriteptr: track previous _IO_write_ptr before a seek operation on
> + the stream.
> + - seekwriteptr: track resulted _IO_write_ptr after a seek operation on
> + the stream.
> +
> + Also, prevwriteptr is only updated iff _IO_write_ptr changed over calls
> + (meaning that a write operation occured)
> +
> + So final buffer size is based on current _IO_write_ptr only if
> + its value is different than seekwriteptr, otherwise it uses the old
> + _IO_write_ptr value before seek operation (prevwriteptr). */
> + new_f->fp.prevwriteptr = new_f->fp.seekwriteptr =
> + new_f->fp._sf._sbf._f._wide_data->_IO_write_ptr;
>
> return (_IO_FILE *) &new_f->fp._sf._sbf;
> }
> @@ -114,8 +135,9 @@ _IO_wmem_sync (_IO_FILE *fp)
> }
>
> *mp->bufloc = fp->_wide_data->_IO_write_base;
> - *mp->sizeloc = (fp->_wide_data->_IO_write_ptr
> - - fp->_wide_data->_IO_write_base);
> + wchar_t *ptr = (fp->_wide_data->_IO_write_ptr == mp->seekwriteptr)
> + ? mp->prevwriteptr : fp->_wide_data->_IO_write_ptr;
> + *mp->sizeloc = ptr - fp->_wide_data->_IO_write_base;
>
> return 0;
> }
> @@ -132,9 +154,16 @@ _IO_wmem_finish (_IO_FILE *fp, int dummy)
> * sizeof (wchar_t));
> if (*mp->bufloc != NULL)
> {
> - size_t len = (fp->_wide_data->_IO_write_ptr
> - - fp->_wide_data->_IO_write_base);
> - (*mp->bufloc)[len] = '\0';
> + size_t len;
> + if (fp->_wide_data->_IO_write_ptr == mp->seekwriteptr)
> + len = mp->prevwriteptr - fp->_wide_data->_IO_write_base;
> + else
> + {
> + /* An '\0' should be appended iff a write operation ocurred. */
> + len = fp->_wide_data->_IO_write_ptr
> + - fp->_wide_data->_IO_write_base;
> + (*mp->bufloc)[len] = L'\0';
> + }
> *mp->sizeloc = len;
>
> fp->_wide_data->_IO_buf_base = NULL;
> @@ -142,3 +171,14 @@ _IO_wmem_finish (_IO_FILE *fp, int dummy)
>
> _IO_wstr_finish (fp, 0);
> }
> +
> +static _IO_off64_t
> +_IO_wmem_seekoff (_IO_FILE *fp, _IO_off64_t offset, int dir, int mode)
> +{
> + struct _IO_FILE_wmemstream *mp = (struct _IO_FILE_wmemstream *) fp;
> + if (fp->_wide_data->_IO_write_ptr != mp->seekwriteptr)
> + mp->prevwriteptr = fp->_wide_data->_IO_write_ptr;
> + _IO_off64_t ret = _IO_wstr_seekoff (fp, offset, dir, mode);
> + mp->seekwriteptr = fp->_wide_data->_IO_write_ptr;
> + return ret;
> +}
>