diff mbox series

Discard any push-back from a memory stream before finishing it (bug 34020)

Message ID mvm7bqtiivh.fsf@suse.de (mailing list archive)
State Under Review
Delegated to: DJ Delorie
Headers
Series Discard any push-back from a memory stream before finishing it (bug 34020) |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Build passed
redhat-pt-bot/TryBot-32bit fail Patch caused testsuite regressions
linaro-tcwg-bot/tcwg_glibc_check--master-arm fail Test failed
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 fail Test failed

Commit Message

Andreas Schwab March 30, 2026, 12:47 p.m. UTC 
  This makes sure that the write base is pointing to the main area, not the
backup area.
---
The wide char test requires
https://patchwork.sourceware.org/project/glibc/patch/20260323171742.1039768-1-marocketbd@gmail.com/
to pass.
---
 libio/Makefile          |  2 +
 libio/memstream.c       |  3 ++
 libio/tst-memstream.h   |  4 ++
 libio/tst-memstream6.c  | 81 +++++++++++++++++++++++++++++++++++++++++
 libio/tst-wmemstream6.c | 20 ++++++++++
 libio/wmemstream.c      |  3 ++
 6 files changed, 113 insertions(+)
 create mode 100644 libio/tst-memstream6.c
 create mode 100644 libio/tst-wmemstream6.c

Comments

DJ Delorie March 30, 2026, 7:07 p.m. UTC | #1 
Andreas Schwab <schwab@suse.de> writes:
> This makes sure that the write base is pointing to the main area, not the
> backup area.

So I just realized, after working in this code for years, that "backup"
here doesn't mean "in case we run out" it means "going backwards" :-P No
wonder it seemed weird to me.

One question below about fseek, but otherwise OK.

> diff --git a/libio/memstream.c b/libio/memstream.c
> index 0456adb92f..fcc925df87 100644
> --- a/libio/memstream.c
> +++ b/libio/memstream.c
> @@ -100,6 +100,9 @@ _IO_mem_finish (FILE *fp, int dummy)
>  {
>    struct _IO_FILE_memstream *mp = (struct _IO_FILE_memstream *) fp;
>  
> +  if (_IO_in_backup (fp))
> +    _IO_switch_to_main_get_area (fp);
> +
>    *mp->bufloc = (char *) realloc (fp->_IO_write_base,
>  				  fp->_IO_write_ptr - fp->_IO_write_base + 1);
>    if (*mp->bufloc != NULL)

Ok.

> diff --git a/libio/tst-memstream6.c b/libio/tst-memstream6.c
> +static void
> +mcheck_abort (enum mcheck_status ev)
> +{
> +  printf ("mecheck failed with status %d\n", (int) ev);
> +  exit (1);
> +}

Ok.

> +void
> +do_test_ungetc_1 (void)
> +{
> +  CHAR_T *buf;
> +  size_t size;
> +
> +  FILE *fp = OPEN_MEMSTREAM (&buf, &size);
> +  TEST_VERIFY_EXIT (fp != NULL);
> +
> +  FPUTC (W('A'), fp);
> +  fflush (fp);
> +
> +  TEST_COMPARE (FGETC (fp), W('A'));

I would have expected you to need an fseek here.  fflush() on a
memstream synchronizes the buffer and size pointers but says nothing
about moving the file pointer.

> +  TEST_COMPARE (UNGETC (W('B'), fp), W('B'));
> +
> +  TEST_COMPARE (fclose (fp), 0);
> +
> +  TEST_COMPARE (buf[0], W('A'));
> +
> +  free (buf);
> +}

Ok.

> +void
> +do_test_ungetc_2 (void)
> +{
> +  CHAR_T *buf;
> +  size_t size;
> +
> +  FILE *fp = OPEN_MEMSTREAM (&buf, &size);
> +  TEST_VERIFY_EXIT (fp != NULL);
> +
> +  TEST_COMPARE (UNGETC (W('A'), fp), W('A'));
> +
> +  TEST_COMPARE (fclose (fp), 0);
> +
> +  TEST_COMPARE (buf[0], 0);
> +
> +  free (buf);
> +}

Ok.

> +static int
> +do_test (void)
> +{
> +  mcheck_pedantic (mcheck_abort);
> +
> +  do_test_ungetc_1 ();
> +  do_test_ungetc_2 ();
> +
> +  return 0;
> +}

Ok.

> diff --git a/libio/tst-wmemstream6.c b/libio/tst-wmemstream6.c
> +#define TEST_WCHAR
> +#include <libio/tst-memstream6.c>

Ok.

> diff --git a/libio/wmemstream.c b/libio/wmemstream.c
> index d0c639be70..a0c61a627f 100644
> --- a/libio/wmemstream.c
> +++ b/libio/wmemstream.c
> @@ -101,6 +101,9 @@ _IO_wmem_finish (FILE *fp, int dummy)
>  {
>    struct _IO_FILE_wmemstream *mp = (struct _IO_FILE_wmemstream *) fp;
>  
> +  if (_IO_in_backup (fp))
> +    _IO_switch_to_main_wget_area (fp);
> +
>    *mp->bufloc = (wchar_t *) realloc (fp->_wide_data->_IO_write_base,
>  				     (fp->_wide_data->_IO_write_ptr
>  				      - fp->_wide_data->_IO_write_base + 1)

Ok.
Adhemerval Zanella Netto April 2, 2026, 7:12 p.m. UTC | #2 
On 30/03/26 16:07, DJ Delorie wrote:
> Andreas Schwab <schwab@suse.de> writes:
>> This makes sure that the write base is pointing to the main area, not the
>> backup area.
> 
> So I just realized, after working in this code for years, that "backup"
> here doesn't mean "in case we run out" it means "going backwards" :-P No
> wonder it seemed weird to me.
> 
> One question below about fseek, but otherwise OK.

The new testcase triggers some regression on both redhad and Linaro CI [1],
and it seems a real one.


[1] https://patchwork.sourceware.org/project/glibc/patch/mvm7bqtiivh.fsf@suse.de/

> 
>> diff --git a/libio/memstream.c b/libio/memstream.c
>> index 0456adb92f..fcc925df87 100644
>> --- a/libio/memstream.c
>> +++ b/libio/memstream.c
>> @@ -100,6 +100,9 @@ _IO_mem_finish (FILE *fp, int dummy)
>>  {
>>    struct _IO_FILE_memstream *mp = (struct _IO_FILE_memstream *) fp;
>>  
>> +  if (_IO_in_backup (fp))
>> +    _IO_switch_to_main_get_area (fp);
>> +
>>    *mp->bufloc = (char *) realloc (fp->_IO_write_base,
>>  				  fp->_IO_write_ptr - fp->_IO_write_base + 1);
>>    if (*mp->bufloc != NULL)
> 
> Ok.
> 
>> diff --git a/libio/tst-memstream6.c b/libio/tst-memstream6.c
>> +static void
>> +mcheck_abort (enum mcheck_status ev)
>> +{
>> +  printf ("mecheck failed with status %d\n", (int) ev);
>> +  exit (1);
>> +}
> 
> Ok.
> 
>> +void
>> +do_test_ungetc_1 (void)
>> +{
>> +  CHAR_T *buf;
>> +  size_t size;
>> +
>> +  FILE *fp = OPEN_MEMSTREAM (&buf, &size);
>> +  TEST_VERIFY_EXIT (fp != NULL);
>> +
>> +  FPUTC (W('A'), fp);
>> +  fflush (fp);
>> +
>> +  TEST_COMPARE (FGETC (fp), W('A'));
> 
> I would have expected you to need an fseek here.  fflush() on a
> memstream synchronizes the buffer and size pointers but says nothing
> about moving the file pointer.
> 
>> +  TEST_COMPARE (UNGETC (W('B'), fp), W('B'));
>> +
>> +  TEST_COMPARE (fclose (fp), 0);
>> +
>> +  TEST_COMPARE (buf[0], W('A'));
>> +
>> +  free (buf);
>> +}
> 
> Ok.
> 
>> +void
>> +do_test_ungetc_2 (void)
>> +{
>> +  CHAR_T *buf;
>> +  size_t size;
>> +
>> +  FILE *fp = OPEN_MEMSTREAM (&buf, &size);
>> +  TEST_VERIFY_EXIT (fp != NULL);
>> +
>> +  TEST_COMPARE (UNGETC (W('A'), fp), W('A'));
>> +
>> +  TEST_COMPARE (fclose (fp), 0);
>> +
>> +  TEST_COMPARE (buf[0], 0);
>> +
>> +  free (buf);
>> +}
> 
> Ok.
> 
>> +static int
>> +do_test (void)
>> +{
>> +  mcheck_pedantic (mcheck_abort);
>> +
>> +  do_test_ungetc_1 ();
>> +  do_test_ungetc_2 ();
>> +
>> +  return 0;
>> +}
> 
> Ok.
> 
>> diff --git a/libio/tst-wmemstream6.c b/libio/tst-wmemstream6.c
>> +#define TEST_WCHAR
>> +#include <libio/tst-memstream6.c>
> 
> Ok.
> 
>> diff --git a/libio/wmemstream.c b/libio/wmemstream.c
>> index d0c639be70..a0c61a627f 100644
>> --- a/libio/wmemstream.c
>> +++ b/libio/wmemstream.c
>> @@ -101,6 +101,9 @@ _IO_wmem_finish (FILE *fp, int dummy)
>>  {
>>    struct _IO_FILE_wmemstream *mp = (struct _IO_FILE_wmemstream *) fp;
>>  
>> +  if (_IO_in_backup (fp))
>> +    _IO_switch_to_main_wget_area (fp);
>> +
>>    *mp->bufloc = (wchar_t *) realloc (fp->_wide_data->_IO_write_base,
>>  				     (fp->_wide_data->_IO_write_ptr
>>  				      - fp->_wide_data->_IO_write_base + 1)
> 
> Ok.
>
Andreas Schwab April 7, 2026, 10:26 a.m. UTC | #3 
On Apr 02 2026, Adhemerval Zanella Netto wrote:

> The new testcase triggers some regression on both redhad and Linaro CI [1],
> and it seems a real one.

Yes, as I said.
Adhemerval Zanella Netto April 7, 2026, 4:14 p.m. UTC | #4 
On 07/04/26 07:26, Andreas Schwab wrote:
> On Apr 02 2026, Adhemerval Zanella Netto wrote:
> 
>> The new testcase triggers some regression on both redhad and Linaro CI [1],
>> and it seems a real one.
> 
> Yes, as I said.
> 

Maybe approve it first, and they send the patch? It kinda confusing to check
on commit message to understand why CI has failed.
diff mbox series

Patch

diff --git a/libio/Makefile b/libio/Makefile
index da838cdecc..4582291b24 100644
--- a/libio/Makefile
+++ b/libio/Makefile
@@ -121,6 +121,7 @@  tests = \
   tst-memstream2 \
   tst-memstream3 \
   tst-memstream4 \
+  tst-memstream6 \
   tst-mmap-eofsync \
   tst-mmap-fflushsync \
   tst-mmap-offend \
@@ -144,6 +145,7 @@  tests = \
   tst-wmemstream3 \
   tst-wmemstream4 \
   tst-wmemstream5 \
+  tst-wmemstream6 \
   tst_getwc \
   tst_putwc \
   tst_swprintf \
diff --git a/libio/memstream.c b/libio/memstream.c
index 0456adb92f..fcc925df87 100644
--- a/libio/memstream.c
+++ b/libio/memstream.c
@@ -100,6 +100,9 @@  _IO_mem_finish (FILE *fp, int dummy)
 {
   struct _IO_FILE_memstream *mp = (struct _IO_FILE_memstream *) fp;
 
+  if (_IO_in_backup (fp))
+    _IO_switch_to_main_get_area (fp);
+
   *mp->bufloc = (char *) realloc (fp->_IO_write_base,
 				  fp->_IO_write_ptr - fp->_IO_write_base + 1);
   if (*mp->bufloc != NULL)
diff --git a/libio/tst-memstream.h b/libio/tst-memstream.h
index b420e32e66..581cfd2cc3 100644
--- a/libio/tst-memstream.h
+++ b/libio/tst-memstream.h
@@ -50,6 +50,8 @@  fwwrite (const void *ptr, size_t size, size_t nmemb, FILE *arq)
 # define FWRITE fwwrite
 # define FPUTC  fputwc
 # define FPUTS  fputws
+# define FGETC  fgetwc
+# define UNGETC	ungetwc
 # define STRCMP wcscmp
 # define STRLEN wcslen
 #else
@@ -60,6 +62,8 @@  fwwrite (const void *ptr, size_t size, size_t nmemb, FILE *arq)
 # define FWRITE fwrite
 # define FPUTC fputc
 # define FPUTS  fputs
+# define FGETC  fgetc
+# define UNGETC	ungetc
 # define STRCMP strcmp
 # define STRLEN strlen
 #endif
diff --git a/libio/tst-memstream6.c b/libio/tst-memstream6.c
new file mode 100644
index 0000000000..128e733b98
--- /dev/null
+++ b/libio/tst-memstream6.c
@@ -0,0 +1,81 @@ 
+/* Test for open_memstream BZ #34020.
+   Copyright (C) 2018-2026 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
+   <https://www.gnu.org/licenses/>.  */
+
+#include "tst-memstream.h"
+
+static void
+mcheck_abort (enum mcheck_status ev)
+{
+  printf ("mecheck failed with status %d\n", (int) ev);
+  exit (1);
+}
+
+
+void
+do_test_ungetc_1 (void)
+{
+  CHAR_T *buf;
+  size_t size;
+
+  FILE *fp = OPEN_MEMSTREAM (&buf, &size);
+  TEST_VERIFY_EXIT (fp != NULL);
+
+  FPUTC (W('A'), fp);
+  fflush (fp);
+
+  TEST_COMPARE (FGETC (fp), W('A'));
+  TEST_COMPARE (UNGETC (W('B'), fp), W('B'));
+
+  TEST_COMPARE (fclose (fp), 0);
+
+  TEST_COMPARE (buf[0], W('A'));
+
+  free (buf);
+}
+
+
+void
+do_test_ungetc_2 (void)
+{
+  CHAR_T *buf;
+  size_t size;
+
+  FILE *fp = OPEN_MEMSTREAM (&buf, &size);
+  TEST_VERIFY_EXIT (fp != NULL);
+
+  TEST_COMPARE (UNGETC (W('A'), fp), W('A'));
+
+  TEST_COMPARE (fclose (fp), 0);
+
+  TEST_COMPARE (buf[0], 0);
+
+  free (buf);
+}
+
+static int
+do_test (void)
+{
+  mcheck_pedantic (mcheck_abort);
+
+  do_test_ungetc_1 ();
+  do_test_ungetc_2 ();
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/libio/tst-wmemstream6.c b/libio/tst-wmemstream6.c
new file mode 100644
index 0000000000..23c50197cf
--- /dev/null
+++ b/libio/tst-wmemstream6.c
@@ -0,0 +1,20 @@ 
+/* Test for open_wmemstream BZ #34020.
+   Copyright (C) 2026 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
+   <https://www.gnu.org/licenses/>.  */
+
+#define TEST_WCHAR
+#include <libio/tst-memstream6.c>
diff --git a/libio/wmemstream.c b/libio/wmemstream.c
index d0c639be70..a0c61a627f 100644
--- a/libio/wmemstream.c
+++ b/libio/wmemstream.c
@@ -101,6 +101,9 @@  _IO_wmem_finish (FILE *fp, int dummy)
 {
   struct _IO_FILE_wmemstream *mp = (struct _IO_FILE_wmemstream *) fp;
 
+  if (_IO_in_backup (fp))
+    _IO_switch_to_main_wget_area (fp);
+
   *mp->bufloc = (wchar_t *) realloc (fp->_wide_data->_IO_write_base,
 				     (fp->_wide_data->_IO_write_ptr
 				      - fp->_wide_data->_IO_write_base + 1)