Bug 20181 - open_memstream(): writes not at end of stream corrupt data
Summary: open_memstream(): writes not at end of stream corrupt data
Status: RESOLVED FIXED
Alias: None
Product: glibc
Classification: Unclassified
Component: stdio (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: 2.25
Assignee: Adhemerval Zanella
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-05-30 19:05 UTC by Andrew Church
Modified: 2016-09-30 16:50 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:
fweimer: security-


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Church 2016-05-30 19:05:26 UTC
Memory buffers opened with open_memstream() (and open_wmemstream()) corrupt one byte of data on flush after write if the current write position is not at the end of the stream.

The bug is in _IO_[w]mem_sync(), which blindly writes a null byte (or wchar) to the buffer whether or not the current write position is at the end of the buffer.  This appears to contravene POSIX, which says: "If a write moves the position to a value larger than the current length, the current length shall be set to this position. *In this case* a null character for open_memstream() or a null wide character for open_wmemstream() shall be appended to the current buffer." (emphasis added)

The fix is to remove the "else *fp->_IO_write_ptr = '\0'" clause (libio/memstream.c:115-116 and libio/wmemstream.c:115-116 in current git).

Simple testcase:

char *buf;
size_t size;
FILE *f;
assert(f = open_memstream(&buf, &size));
assert(fwrite("abc", 1, 3, f) == 3);
assert(fseek(f, 0, SEEK_SET) == 0);
assert(fwrite("z", 1, 1, f) == 1);
assert(fflush(f) == 0);  // Clobbers the "b".
assert(fseek(f, 3, SEEK_SET) == 0);  // Avoid truncating the buffer on close.
assert(fclose(f) == 0);
assert(size == 3);
assert(buf[0] == 'z');
assert(buf[1] == 'b');  // Fails (clobbered by null byte).
assert(buf[2] == 'c');
Comment 1 Sourceware Commits 2016-09-30 16:15:06 UTC
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU C Library master sources".

The branch, master has been updated
       via  645f97ced4d4b35deda3f8bde0927f898b163f5d (commit)
      from  f280fa6d171c4d3414c005ad2a7529e0d1d9ee0c (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=645f97ced4d4b35deda3f8bde0927f898b163f5d

commit 645f97ced4d4b35deda3f8bde0927f898b163f5d
Author: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Date:   Mon Jul 25 14:54:29 2016 -0300

    libio: Multiple fixes for open_{w}memstram (BZ#18241 and BZ#20181)
    
    This patches fixes multiples issues on open_{w}memstream reported on both
    BZ#18241 and BZ#20181:
    
      - failed fseek does not set errno.
      - negative offset in fseek fails even when resulting position is
        a valid one.
      - a flush after write if the current write position is not at the
        end of the stream currupt data.
    
    The main fix is on seek operation for memstream (_IO_{w}str_seekoff), where
    both _IO_read_ptr and _IO_read_end pointer are updated if a write operation
    has occured (similar to default file operations).  Also, to calculate the
    offset on both read and write pointers, a temporary value is instead of
    updating the argument supplied value.  Negative offset are valid if resulting
    internal pointer is within the range of _IO_{read,write}_base and
    _IO_{read,write}_end.
    
    Also POSIX states that a null or wide null shall be appended to the current
    buffer iff a write moves the position to a value larger than the current
    lenght.  Current implementation appends a null or wide null regardless
    of this condition.  This patch fixes it by removing the 'else' condition
    on _IO_{w}mem_sync.
    
    Checked on x86_64.
    
    	[BZ #18241]
    	[BZ #20181]
    	* libio/Makefile (test): Add tst-memstream3 and tst-wmemstream3.
    	* libio/memstream.c (_IO_mem_sync): Only append a null byte if
    	write position is at the end the buffer.
    	* libio/wmemstream.c (_IO_wmem_sync): Likewise.
    	* libio/strops.c (_IO_str_switch_to_get_mode): New function.
    	(_IO_str_seekoff): Set correct offset from negative displacement and
    	set EINVAL for invalid ones.
    	* libio/wstrops.c (enlarge_userbuf): Use correct function to calculate
    	buffer length.
    	(_IO_wstr_switch_to_get_mode): New function.
    	(_IO_wstr_seekoff): Set correct offset from negative displacement and
    	set EINVAL for invalid ones.
    	* libio/tst-memstream3.c: New file.
    	* libio/tst-wmemstream3.c: Likewise.
    	* manual/examples/memstrm.c: Remove warning when priting size_t.

-----------------------------------------------------------------------

Summary of changes:
 ChangeLog                                          |   20 +++
 libio/Makefile                                     |    4 +-
 libio/memstream.c                                  |    2 -
 libio/strops.c                                     |   81 +++++++---
 libio/tst-memstream3.c                             |  165 ++++++++++++++++++++
 .../tst-wcstod-round.c => libio/tst-wmemstream3.c  |   33 +++--
 libio/wmemstream.c                                 |    2 -
 libio/wstrops.c                                    |   89 +++++++----
 manual/examples/memstrm.c                          |    4 +-
 9 files changed, 327 insertions(+), 73 deletions(-)
 create mode 100644 libio/tst-memstream3.c
 copy wcsmbs/tst-wcstod-round.c => libio/tst-wmemstream3.c (55%)
Comment 2 Adhemerval Zanella 2016-09-30 16:50:35 UTC
Fixed by 645f97ced4d4b35deda3f8bde0927f898b163f5d.