ungetc: Guarantee single char pushback

The C standard requires that ungetc guarantees at least one pushback,
but the malloc call to allocate the pushback buffer could fail, thus
violating that requirement.  Fix this by adding a single byte pushback
buffer in the FILE struct that the pushback can fall back to if malloc
fails.

The side-effect is that if the initial malloc fails and the 1-byte
fallback buffer is used, future resizing (if it succeeds) will be
2-bytes, 4-bytes and so on, which is suboptimal but it's after a malloc
failure, so maybe even desirable.

A future optimization here could be to have the pushback code use the
single byte buffer first and only fall back to malloc for subsequent
calls.

Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
Reviewed-by: Maciej W. Rozycki <macro@redhat.com>

diff --git a/libio/bits/types/struct_FILE.h b/libio/bits/types/struct_FILE.h
index d8d26639d1256595999153f1e5374a43a6b1f231..87197a328cfda6aa328a925b47e334d6f6133bff 100644 (file)
--- a/libio/bits/types/struct_FILE.h
+++ b/libio/bits/types/struct_FILE.h
@@ -1,4 +1,5 @@
 /* Copyright (C) 1991-2024 Free Software Foundation, Inc.
+   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
@@ -70,7 +71,9 @@ struct _IO_FILE
   struct _IO_FILE *_chain;
 
   int _fileno;
-  int _flags2;
+  int _flags2:24;
+  /* Fallback buffer to use when malloc fails to allocate one.  */
+  char _short_backupbuf[1];
   __off_t _old_offset; /* This used to be _offset but it's too small.  */
 
   /* 1+column number of pbase(); 0 is unknown. */

diff --git a/libio/fileops.c b/libio/fileops.c
index 759d737ec7eb0e386f01512bbe7f3d9f552c8153..d49e489f55d3a283337cb549759e7297aabe3faa 100644 (file)
--- a/libio/fileops.c
+++ b/libio/fileops.c
@@ -1,4 +1,5 @@
 /* Copyright (C) 1993-2024 Free Software Foundation, Inc.
+   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
@@ -480,7 +481,7 @@ _IO_new_file_underflow (FILE *fp)
       /* Maybe we already have a push back pointer.  */
       if (fp->_IO_save_base != NULL)
        {
-         free (fp->_IO_save_base);
+         _IO_free_backup_buf (fp, fp->_IO_save_base);
          fp->_flags &= ~_IO_IN_BACKUP;
        }
       _IO_doallocbuf (fp);
@@ -932,7 +933,7 @@ _IO_new_file_seekoff (FILE *fp, off64_t offset, int dir, int mode)
       /* It could be that we already have a pushback buffer.  */
       if (fp->_IO_read_base != NULL)
        {
-         free (fp->_IO_read_base);
+         _IO_free_backup_buf (fp, fp->_IO_read_base);
          fp->_flags &= ~_IO_IN_BACKUP;
        }
       _IO_doallocbuf (fp);
@@ -1282,7 +1283,7 @@ _IO_file_xsgetn (FILE *fp, void *data, size_t n)
       /* Maybe we already have a push back pointer.  */
       if (fp->_IO_save_base != NULL)
        {
-         free (fp->_IO_save_base);
+         _IO_free_backup_buf (fp, fp->_IO_save_base);
          fp->_flags &= ~_IO_IN_BACKUP;
        }
       _IO_doallocbuf (fp);

diff --git a/libio/genops.c b/libio/genops.c
index d7e35e67d56cbe658be26f71efcd912981a15d0e..02e159d6a83151e473bb2cccacb174502990c7b2 100644 (file)
--- a/libio/genops.c
+++ b/libio/genops.c
@@ -1,4 +1,5 @@
 /* Copyright (C) 1993-2024 Free Software Foundation, Inc.
+   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
@@ -212,7 +213,7 @@ _IO_free_backup_area (FILE *fp)
 {
   if (_IO_in_backup (fp))
     _IO_switch_to_main_get_area (fp);  /* Just in case. */
-  free (fp->_IO_save_base);
+  _IO_free_backup_buf (fp, fp->_IO_save_base);
   fp->_IO_save_base = NULL;
   fp->_IO_save_end = NULL;
   fp->_IO_backup_base = NULL;
@@ -260,7 +261,7 @@ save_for_backup (FILE *fp, char *end_p)
        memcpy (new_buffer + avail,
                fp->_IO_read_base + least_mark,
                needed_size);
-      free (fp->_IO_save_base);
+      _IO_free_backup_buf (fp, fp->_IO_save_base);
       fp->_IO_save_base = new_buffer;
       fp->_IO_save_end = new_buffer + avail + needed_size;
     }
@@ -636,7 +637,7 @@ _IO_default_finish (FILE *fp, int dummy)
 
   if (fp->_IO_save_base)
     {
-      free (fp->_IO_save_base);
+      _IO_free_backup_buf (fp, fp->_IO_save_base);
       fp->_IO_save_base = NULL;
     }
 
@@ -998,11 +999,14 @@ _IO_default_pbackfail (FILE *fp, int c)
          else if (!_IO_have_backup (fp))
            {
              /* No backup buffer: allocate one. */
-             /* Use nshort buffer, if unused? (probably not)  FIXME */
              int backup_size = 128;
              char *bbuf = (char *) malloc (backup_size);
              if (bbuf == NULL)
-               return EOF;
+               {
+                 /* Guarantee a 1-char pushback.  */
+                 bbuf = fp->_short_backupbuf;
+                 backup_size = 1;
+               }
              fp->_IO_save_base = bbuf;
              fp->_IO_save_end = fp->_IO_save_base + backup_size;
              fp->_IO_backup_base = fp->_IO_save_end;
@@ -1022,7 +1026,7 @@ _IO_default_pbackfail (FILE *fp, int c)
            return EOF;
          memcpy (new_buf + (new_size - old_size), fp->_IO_read_base,
                  old_size);
-         free (fp->_IO_read_base);
+         _IO_free_backup_buf (fp, fp->_IO_read_base);
          _IO_setg (fp, new_buf, new_buf + (new_size - old_size),
                    new_buf + new_size);
          fp->_IO_backup_base = fp->_IO_read_ptr;

diff --git a/libio/libioP.h b/libio/libioP.h
index ad45579e138ca10a23c114dc77dd5aaa71d105f1..714abbd549500b58a6b060ded51e9b6adb7d6385 100644 (file)
--- a/libio/libioP.h
+++ b/libio/libioP.h
@@ -1,4 +1,5 @@
 /* Copyright (C) 1993-2024 Free Software Foundation, Inc.
+   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
@@ -911,30 +912,30 @@ extern int _IO_vscanf (const char *, va_list) __THROW;
 #  define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \
        { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \
         NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, \
-        NULL, NULL, (FILE *) CHAIN, FD, \
-        0, _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock }
+        NULL, NULL, (FILE *) CHAIN, FD, 0, { 0 }, \
+        _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock }
 # else
 #  define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \
        { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \
         NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, \
-        NULL, NULL, (FILE *) CHAIN, FD, \
-        0, _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock, _IO_pos_BAD,\
-        NULL, WDP, NULL }
+        NULL, NULL, (FILE *) CHAIN, FD, 0, { 0 }, \
+        _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock, \
+        _IO_pos_BAD, NULL, WDP, NULL }
 # endif
 #else
 # ifdef _IO_USE_OLD_IO_FILE
 #  define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \
        { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \
         NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, \
-        NULL, NULL, (FILE *) CHAIN, FD, \
-        0, _IO_pos_BAD }
+        NULL, NULL, (FILE *) CHAIN, FD, 0, { 0 }, \
+        _IO_pos_BAD }
 # else
 #  define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \
        { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \
         NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, \
-        NULL, NULL, (FILE *) CHAIN, FD, \
-        0, _IO_pos_BAD, 0, 0, { 0 }, NULL, _IO_pos_BAD, \
-        NULL, WDP, NULL }
+        NULL, NULL, (FILE *) CHAIN, FD, 0, { 0 }, \
+        _IO_pos_BAD, 0, 0, { 0 }, NULL, \
+        _IO_pos_BAD, NULL, WDP, NULL }
 # endif
 #endif
 
@@ -1040,6 +1041,15 @@ IO_validate_vtable (const struct _IO_jump_t *vtable)
   return vtable;
 }
 
+/* In case of an allocation failure, we resort to using the fixed buffer
+   _SHORT_BACKUPBUF.  Free PTR unless it points to that buffer.  */
+static __always_inline void
+_IO_free_backup_buf (FILE *fp, char *ptr)
+{
+  if (ptr != fp->_short_backupbuf)
+    free (ptr);
+}
+
 /* Character set conversion.  */
 
 enum __codecvt_result

diff --git a/libio/oldfileops.c b/libio/oldfileops.c
index 8f775c90940badc67bdfc4ba8bb86c8f2edf5b14..03f4d76a57d1788b0fa7a22fcecac35c9890e208 100644 (file)
--- a/libio/oldfileops.c
+++ b/libio/oldfileops.c
@@ -1,4 +1,5 @@
 /* Copyright (C) 1993-2024 Free Software Foundation, Inc.
+   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
@@ -311,7 +312,7 @@ _IO_old_file_underflow (FILE *fp)
       /* Maybe we already have a push back pointer.  */
       if (fp->_IO_save_base != NULL)
        {
-         free (fp->_IO_save_base);
+         _IO_free_backup_buf (fp, fp->_IO_save_base);
          fp->_flags &= ~_IO_IN_BACKUP;
        }
       _IO_doallocbuf (fp);
@@ -464,7 +465,7 @@ _IO_old_file_seekoff (FILE *fp, off64_t offset, int dir, int mode)
       /* It could be that we already have a pushback buffer.  */
       if (fp->_IO_read_base != NULL)
        {
-         free (fp->_IO_read_base);
+         _IO_free_backup_buf (fp, fp->_IO_read_base);
          fp->_flags &= ~_IO_IN_BACKUP;
        }
       _IO_doallocbuf (fp);

diff --git a/libio/wfileops.c b/libio/wfileops.c
index 16beab1f3ae1e804e24f21b789e8352bd6dd0ca4..a96bfa589b5ad91ced38494907440119d06c25a5 100644 (file)
--- a/libio/wfileops.c
+++ b/libio/wfileops.c
@@ -1,4 +1,5 @@
 /* Copyright (C) 1993-2024 Free Software Foundation, Inc.
+   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
@@ -175,7 +176,7 @@ _IO_wfile_underflow (FILE *fp)
       /* Maybe we already have a push back pointer.  */
       if (fp->_IO_save_base != NULL)
        {
-         free (fp->_IO_save_base);
+         _IO_free_backup_buf (fp, fp->_IO_save_base);
          fp->_flags &= ~_IO_IN_BACKUP;
        }
       _IO_doallocbuf (fp);

diff --git a/stdio-common/Makefile b/stdio-common/Makefile
index e76e40e58796ec5b79592a6be0665014e3d326cb..b1a04fd0647918f247d9ff400aaf2997d4a58953 100644 (file)
--- a/stdio-common/Makefile
+++ b/stdio-common/Makefile
@@ -1,4 +1,5 @@
 # Copyright (C) 1991-2024 Free Software Foundation, Inc.
+# 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
@@ -303,6 +304,7 @@ tests := \
   tst-tmpnam \
   tst-ungetc \
   tst-ungetc-leak \
+  tst-ungetc-nomem \
   tst-unlockedio \
   tst-vfprintf-mbs-prec \
   tst-vfprintf-user-type \

diff --git a/stdio-common/tst-ungetc-nomem.c b/stdio-common/tst-ungetc-nomem.c
new file mode 100644 (file)
index 0000000..0872de6
--- /dev/null
+++ b/stdio-common/tst-ungetc-nomem.c
@@ -0,0 +1,121 @@
+/* Test ungetc 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 <dlfcn.h>
+#include <stdio.h>
+#include <string.h>
+#include <support/check.h>
+#include <support/support.h>
+#include <support/temp_file.h>
+#include <support/xstdio.h>
+
+static volatile bool fail = false;
+
+/* Induce a malloc failure whenever FAIL is set; we use the __LIBC_MALLOC entry
+   point to avoid the other alternative, which is RTLD_NEXT.  */
+void *
+malloc (size_t sz)
+{
+  if (fail)
+    return NULL;
+
+  static void *(*real_malloc) (size_t);
+
+  if (real_malloc == NULL)
+    real_malloc = dlsym (RTLD_NEXT, "malloc");
+
+  return real_malloc (sz);
+}
+
+static int
+do_test (void)
+{
+  char *filename = NULL;
+  struct stat props = {};
+  size_t bufsz = 0;
+
+  create_temp_file ("tst-ungetc-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.  This is to ensure that
+     the test runs at the read underflow boundary as well.  */
+  bufsz = props.st_blksize + 2;
+
+  char *buf = xmalloc (bufsz);
+  memset (buf, 'a', bufsz);
+
+  if (fwrite (buf, sizeof (char), bufsz, fp) != bufsz)
+    FAIL_EXIT1 ("fwrite failed: %m\n");
+  xfclose (fp);
+
+  /* Begin test.  */
+  fp = xfopen (filename, "r");
+
+  while (!feof (fp))
+    {
+      /* Reset the pushback buffer state.  */
+      fseek (fp, 0, SEEK_CUR);
+
+      fail = true;
+      /* 1: First ungetc should always succeed, as the standard requires.  */
+      TEST_COMPARE (ungetc ('b', fp), 'b');
+
+      /* 2: This will result in resizing, which should fail.  */
+      TEST_COMPARE (ungetc ('c', fp), EOF);
+
+      /* 3: Now allow the resizing, which should immediately fill up the buffer
+         too, since this allocates only double the current buffer, i.e.
+         2-bytes.  */
+      fail = false;
+      TEST_COMPARE (ungetc ('d', fp), 'd');
+
+      /* 4: And fail again because this again forces an alloc, which fails.  */
+      fail = true;
+      TEST_COMPARE (ungetc ('e', fp), EOF);
+
+      /* 5: Enable allocations again so that we now get a 4-byte buffer.  Now
+         both calls should work.  */
+      fail = false;
+      TEST_COMPARE (ungetc ('f', fp), 'f');
+      fail = true;
+      TEST_COMPARE (ungetc ('g', fp), 'g');
+
+      /* Drain out the x's.  */
+      TEST_COMPARE (fgetc (fp), 'g');
+      TEST_COMPARE (fgetc (fp), 'f');
+      TEST_COMPARE (fgetc (fp), 'd');
+
+      /* Finally, drain out the first char we had pushed back, followed by one
+        more char from the stream, if present.  */
+      TEST_COMPARE (fgetc (fp), 'b');
+      char c = fgetc (fp);
+      if (!feof (fp))
+       TEST_COMPARE (c, 'a');
+    }
+
+  /* Final sanity check before we're done.  */
+  TEST_COMPARE (ferror (fp), 0);
+  xfclose (fp);
+
+  return 0;
+}
+
+#include <support/test-driver.c>