diff mbox series

[3/3] ungetwc: Guarantee single char pushback

Message ID 20241108171450.411932-4-siddhesh@sourceware.org
State New
Headers show
Series Guarantee first pushback in ungetc and ungetwc | expand

Commit Message

Siddhesh Poyarekar Nov. 8, 2024, 5:14 p.m. UTC
The C standard requires that ungetwc guarantees at least one pushback,
so put a single byte pushback buffer in the _wide_data struct to enable
that.

Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
---
 libio/Makefile            |  1 +
 libio/libio.h             |  1 +
 libio/tst-ungetwc-nomem.c | 94 +++++++++++++++++++++++++++++++++++++++
 libio/wfileops.c          | 28 ++++--------
 libio/wgenops.c           | 29 ++++++------
 5 files changed, 119 insertions(+), 34 deletions(-)
 create mode 100644 libio/tst-ungetwc-nomem.c

Comments

Florian Weimer Nov. 28, 2024, 3:19 p.m. UTC | #1
* Siddhesh Poyarekar:

> The C standard requires that ungetwc guarantees at least one pushback,
> so put a single byte pushback buffer in the _wide_data struct to enable
> that.

I think it's possible to call ungetwc on an unoriented stream, so a fix
will have to deal with a _wide_data allocation failure, too.

Thanks,
Florian
Siddhesh Poyarekar Nov. 29, 2024, 2:46 p.m. UTC | #2
On 2024-11-28 10:19, Florian Weimer wrote:
> * Siddhesh Poyarekar:
> 
>> The C standard requires that ungetwc guarantees at least one pushback,
>> so put a single byte pushback buffer in the _wide_data struct to enable
>> that.
> 
> I think it's possible to call ungetwc on an unoriented stream, so a fix
> will have to deal with a _wide_data allocation failure, too.

Ack, I'll post this one separately then and only deal with ungetc for now.

Thanks,
Sid
diff mbox series

Patch

diff --git a/libio/Makefile b/libio/Makefile
index 4370152964..c6581ab761 100644
--- a/libio/Makefile
+++ b/libio/Makefile
@@ -128,6 +128,7 @@  tests = \
   tst-sprintf-ub \
   tst-sscanf \
   tst-swscanf \
+  tst-ungetwc-nomem \
   tst-ungetwc1 \
   tst-ungetwc2 \
   tst-wfile-sync \
diff --git a/libio/libio.h b/libio/libio.h
index f89614b8a6..e6d655c289 100644
--- a/libio/libio.h
+++ b/libio/libio.h
@@ -139,6 +139,7 @@  struct _IO_wide_data
   struct _IO_codecvt _codecvt;
 
   wchar_t _shortbuf[1];
+  wchar_t _short_backupbuf[1];
 
   const struct _IO_jump_t *_wide_vtable;
 };
diff --git a/libio/tst-ungetwc-nomem.c b/libio/tst-ungetwc-nomem.c
new file mode 100644
index 0000000000..5e3edcd33a
--- /dev/null
+++ b/libio/tst-ungetwc-nomem.c
@@ -0,0 +1,94 @@ 
+/* Test ungetwc behavior with malloc failures.
+   Copyright The GNU Toolchain Authors.
+   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 <stdio.h>
+#include <wchar.h>
+#include <support/check.h>
+#include <support/support.h>
+#include <support/temp_file.h>
+
+extern void *__libc_malloc (size_t)
+     __attribute__ ((malloc)) __attribute__ ((alloc_size (1)));
+
+static bool fail = false;
+
+void *
+malloc (size_t sz)
+{
+  if (fail)
+    return NULL;
+
+  return __libc_malloc (sz);
+}
+
+static int
+do_test (void)
+{
+  char *filename = NULL;
+  struct stat props = {};
+  size_t bufsz = 0;
+
+  create_temp_file ("tst-ungetwc-nomem.", &filename);
+  if (stat (filename, &props) != 0)
+    FAIL_EXIT1 ("Could not get file status: %m\n");
+
+  FILE *fp = fopen (filename, "w");
+
+  /* The libio buffer sizes are the same as block size.  */
+  bufsz = (props.st_blksize + 2);
+
+  wchar_t *buf = xmalloc (bufsz * sizeof (wchar_t));
+  wmemset (buf, L'a', bufsz);
+
+  size_t remaining = bufsz;
+  while (remaining > 0)
+    {
+      size_t done = fwrite (buf, sizeof (wchar_t), remaining, fp);
+      if (done == 0)
+	break;
+      remaining -= done;
+    }
+  fclose (fp);
+
+  /* Begin test.  */
+  fp = fopen (filename, "r");
+
+
+  /* The standard requires the first ungetwc to always work.  */
+  fail = true;
+  TEST_COMPARE (ungetwc(L'y', fp), L'y');
+
+  /* Now let the buffers get allocated to allow for subsequent tests.  */
+  fail = false;
+  TEST_COMPARE (fgetwc (fp), L'y');
+  TEST_COMPARE (ungetwc(L'y', fp), L'y');
+  TEST_COMPARE (fgetwc (fp), L'y');
+
+  while (!feof (fp))
+    {
+      fail = true;
+      TEST_COMPARE (ungetwc(L'y', fp), L'y');
+      fail = false;
+      TEST_COMPARE (fgetwc (fp), L'y');
+      if (fgetwc (fp) != L'a')
+	TEST_COMPARE (ferror (fp), 0);
+    }
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/libio/wfileops.c b/libio/wfileops.c
index fdbe8692e8..292732bbbe 100644
--- a/libio/wfileops.c
+++ b/libio/wfileops.c
@@ -173,11 +173,8 @@  _IO_wfile_underflow (FILE *fp)
   if (fp->_IO_buf_base == NULL)
     {
       /* Maybe we already have a push back pointer.  */
-      if (fp->_IO_save_base != NULL)
-	{
-	  free (fp->_IO_save_base);
-	  fp->_flags &= ~_IO_IN_BACKUP;
-	}
+      if (_IO_have_backup (fp))
+	_IO_free_backup_area (fp);
       _IO_doallocbuf (fp);
 
       fp->_IO_read_base = fp->_IO_read_ptr = fp->_IO_read_end =
@@ -190,11 +187,8 @@  _IO_wfile_underflow (FILE *fp)
   if (fp->_wide_data->_IO_buf_base == NULL)
     {
       /* Maybe we already have a push back pointer.  */
-      if (fp->_wide_data->_IO_save_base != NULL)
-	{
-	  free (fp->_wide_data->_IO_save_base);
-	  fp->_flags &= ~_IO_IN_BACKUP;
-	}
+      if (_IO_have_wbackup (fp))
+	_IO_free_wbackup_area (fp);
       _IO_wdoallocbuf (fp);
     }
 
@@ -359,11 +353,8 @@  _IO_wfile_underflow_mmap (FILE *fp)
   if (fp->_wide_data->_IO_buf_base == NULL)
     {
       /* Maybe we already have a push back pointer.  */
-      if (fp->_wide_data->_IO_save_base != NULL)
-	{
-	  free (fp->_wide_data->_IO_save_base);
-	  fp->_flags &= ~_IO_IN_BACKUP;
-	}
+      if (_IO_have_wbackup (fp))
+	_IO_free_wbackup_area (fp);
       _IO_wdoallocbuf (fp);
     }
 
@@ -775,11 +766,8 @@  _IO_wfile_seekoff (FILE *fp, off64_t offset, int dir, int mode)
   if (fp->_wide_data->_IO_buf_base == NULL)
     {
       /* It could be that we already have a pushback buffer.  */
-      if (fp->_wide_data->_IO_read_base != NULL)
-	{
-	  free (fp->_wide_data->_IO_read_base);
-	  fp->_flags &= ~_IO_IN_BACKUP;
-	}
+      if (_IO_have_wbackup (fp))
+	_IO_free_wbackup_area (fp);
       _IO_doallocbuf (fp);
       _IO_setp (fp, fp->_IO_buf_base, fp->_IO_buf_base);
       _IO_setg (fp, fp->_IO_buf_base, fp->_IO_buf_base, fp->_IO_buf_base);
diff --git a/libio/wgenops.c b/libio/wgenops.c
index adfb97014f..d347600c6b 100644
--- a/libio/wgenops.c
+++ b/libio/wgenops.c
@@ -34,6 +34,13 @@ 
 
 static int save_for_wbackup (FILE *fp, wchar_t *end_p) __THROW;
 
+static void
+free_wbackup_buf (FILE *fp, wchar_t *ptr)
+{
+  if (fp->_wide_data->_short_backupbuf != ptr)
+    free (ptr);
+}
+
 /* Return minimum _pos markers
    Assumes the current get area is the main get area. */
 ssize_t
@@ -125,16 +132,10 @@  _IO_wdefault_pbackfail (FILE *fp, wint_t c)
 	    }
 	  else if (!_IO_have_wbackup (fp))
 	    {
-	      /* No backup buffer: allocate one. */
-	      /* Use nshort buffer, if unused? (probably not)  FIXME */
-	      int backup_size = 128;
-	      wchar_t *bbuf = (wchar_t *) malloc (backup_size
-						  * sizeof (wchar_t));
-	      if (bbuf == NULL)
-		return WEOF;
-	      fp->_wide_data->_IO_save_base = bbuf;
-	      fp->_wide_data->_IO_save_end = (fp->_wide_data->_IO_save_base
-					      + backup_size);
+	      /* Start with the 1-byte buffer to guarantee at least 1 wide char
+		 pushback.  */
+	      fp->_wide_data->_IO_save_base = fp->_wide_data->_short_backupbuf;
+	      fp->_wide_data->_IO_save_end = fp->_wide_data->_IO_save_base + 1;
 	      fp->_wide_data->_IO_backup_base = fp->_wide_data->_IO_save_end;
 	    }
 	  fp->_wide_data->_IO_read_base = fp->_wide_data->_IO_read_ptr;
@@ -153,7 +154,7 @@  _IO_wdefault_pbackfail (FILE *fp, wint_t c)
 	    return WEOF;
 	  __wmemcpy (new_buf + (new_size - old_size),
 		     fp->_wide_data->_IO_read_base, old_size);
-	  free (fp->_wide_data->_IO_read_base);
+	  free_wbackup_buf (fp, fp->_wide_data->_IO_read_base);
 	  _IO_wsetg (fp, new_buf, new_buf + (new_size - old_size),
 		     new_buf + new_size);
 	  fp->_wide_data->_IO_backup_base = fp->_wide_data->_IO_read_ptr;
@@ -181,7 +182,7 @@  _IO_wdefault_finish (FILE *fp, int dummy)
 
   if (fp->_IO_save_base)
     {
-      free (fp->_wide_data->_IO_save_base);
+      free_wbackup_buf (fp, fp->_wide_data->_IO_save_base);
       fp->_IO_save_base = NULL;
     }
 
@@ -416,7 +417,7 @@  _IO_free_wbackup_area (FILE *fp)
 {
   if (_IO_in_backup (fp))
     _IO_switch_to_main_wget_area (fp);  /* Just in case. */
-  free (fp->_wide_data->_IO_save_base);
+  free_wbackup_buf (fp, fp->_wide_data->_IO_save_base);
   fp->_wide_data->_IO_save_base = NULL;
   fp->_wide_data->_IO_save_end = NULL;
   fp->_wide_data->_IO_backup_base = NULL;
@@ -459,7 +460,7 @@  save_for_wbackup (FILE *fp, wchar_t *end_p)
 		     fp->_wide_data->_IO_read_base + least_mark,
 		     needed_size);
 	}
-      free (fp->_wide_data->_IO_save_base);
+      free_wbackup_buf (fp, fp->_wide_data->_IO_save_base);
       fp->_wide_data->_IO_save_base = new_buffer;
       fp->_wide_data->_IO_save_end = new_buffer + avail + needed_size;
     }