[PATCH 0/3] Guarantee first pushback in ungetc and ungetwc

[PATCH 0/3] Guarantee first pushback in ungetc and ungetwc

[Siddhesh Poyarekar]

The C standard requires the first pushback after read operations to
succeed for ungetc and ungetwc.  Use a single-char buffer in the FILE
struct to guarantee this, so that the calls are not subject to a malloc
failure.

Tested on x86_64.

Siddhesh Poyarekar (3):
  libio: make _IO_least_marker static
  ungetc: Guarantee single char pushback
  ungetwc: Guarantee single char pushback

 libio/Makefile                  |  1 +
 libio/bits/types/struct_FILE.h  |  3 +-
 libio/fileops.c                 | 21 +++-----
 libio/genops.c                  | 35 ++++++------
 libio/libio.h                   |  1 +
 libio/tst-ungetwc-nomem.c       | 94 +++++++++++++++++++++++++++++++++
 libio/wfileops.c                | 28 +++-------
 libio/wgenops.c                 | 29 +++++-----
 stdio-common/Makefile           |  1 +
 stdio-common/tst-ungetc-nomem.c | 94 +++++++++++++++++++++++++++++++++
 10 files changed, 240 insertions(+), 67 deletions(-)
 create mode 100644 libio/tst-ungetwc-nomem.c
 create mode 100644 stdio-common/tst-ungetc-nomem.c

-- 
2.46.0

[PATCH 1/3] libio: make _IO_least_marker static

[Siddhesh Poyarekar]

Trivial cleanup to limit _IO_least_marker so that it's clear that it is
unused outside of genops.

Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
---
 libio/genops.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/libio/genops.c b/libio/genops.c
index 6f20d49669..6545a78ad5 100644
--- a/libio/genops.c
+++ b/libio/genops.c
@@ -135,10 +135,8 @@ libc_hidden_def (_IO_link_in)
 
 /* Return minimum _pos markers
    Assumes the current get area is the main get area. */
-ssize_t _IO_least_marker (FILE *fp, char *end_p);
-
-ssize_t
-_IO_least_marker (FILE *fp, char *end_p)
+static ssize_t
+least_marker (FILE *fp, char *end_p)
 {
   ssize_t least_so_far = end_p - fp->_IO_read_base;
   struct _IO_marker *mark;
@@ -235,7 +233,7 @@ static int
 save_for_backup (FILE *fp, char *end_p)
 {
   /* Append [_IO_read_base..end_p] to backup area. */
-  ssize_t least_mark = _IO_least_marker (fp, end_p);
+  ssize_t least_mark = least_marker (fp, end_p);
   /* needed_size is how much space we need in the backup area. */
   size_t needed_size = (end_p - fp->_IO_read_base) - least_mark;
   /* FIXME: Dubious arithmetic if pointers are NULL */
-- 
2.46.0

[PATCH 2/3] ungetc: Guarantee single char pushback

[Siddhesh Poyarekar]

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

Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
---
 libio/bits/types/struct_FILE.h  |  3 +-
 libio/fileops.c                 | 21 +++-----
 libio/genops.c                  | 27 +++++-----
 stdio-common/Makefile           |  1 +
 stdio-common/tst-ungetc-nomem.c | 94 +++++++++++++++++++++++++++++++++
 5 files changed, 118 insertions(+), 28 deletions(-)
 create mode 100644 stdio-common/tst-ungetc-nomem.c

diff --git a/libio/bits/types/struct_FILE.h b/libio/bits/types/struct_FILE.h
index d8d26639d1..6b5295fce5 100644
--- a/libio/bits/types/struct_FILE.h
+++ b/libio/bits/types/struct_FILE.h
@@ -94,8 +94,9 @@ struct _IO_FILE_complete
   void *_freeres_buf;
   struct _IO_FILE **_prevchain;
   int _mode;
+  char _short_backupbuf[1];
   /* Make sure we don't get into trouble again.  */
-  char _unused2[15 * sizeof (int) - 5 * sizeof (void *)];
+  char _unused2[15 * sizeof (int) - 5 * sizeof (void *) - sizeof (char)];
 };
 
 /* These macros are used by bits/stdio.h and internal headers.  */
diff --git a/libio/fileops.c b/libio/fileops.c
index 4db4a76f75..67e57e0d8c 100644
--- a/libio/fileops.c
+++ b/libio/fileops.c
@@ -478,11 +478,8 @@ _IO_new_file_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);
     }
 
@@ -930,11 +927,8 @@ _IO_new_file_seekoff (FILE *fp, off64_t offset, int dir, int mode)
   if (fp->_IO_buf_base == NULL)
     {
       /* It could be that we already have a pushback buffer.  */
-      if (fp->_IO_read_base != NULL)
-	{
-	  free (fp->_IO_read_base);
-	  fp->_flags &= ~_IO_IN_BACKUP;
-	}
+      if (_IO_have_backup (fp))
+	_IO_free_backup_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);
@@ -1280,11 +1274,8 @@ _IO_file_xsgetn (FILE *fp, void *data, size_t n)
   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);
     }
 
diff --git a/libio/genops.c b/libio/genops.c
index 6545a78ad5..02b38fbc9a 100644
--- a/libio/genops.c
+++ b/libio/genops.c
@@ -48,6 +48,13 @@ flush_cleanup (void *not_used)
 }
 #endif
 
+static void
+free_backup_buf (FILE *fp, char *ptr)
+{
+  if (fp->_short_backupbuf != ptr)
+    free (ptr);
+}
+
 /* Fields in struct _IO_FILE after the _lock field are internal to
    glibc and opaque to applications.  We can change them as long as
    the size of struct _IO_FILE is unchanged, which is checked as the
@@ -212,7 +219,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);
+  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 +267,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);
+      free_backup_buf (fp, fp->_IO_save_base);
       fp->_IO_save_base = new_buffer;
       fp->_IO_save_end = new_buffer + avail + needed_size;
     }
@@ -634,7 +641,7 @@ _IO_default_finish (FILE *fp, int dummy)
   for (mark = fp->_markers; mark != NULL; mark = mark->_next)
     mark->_sbuf = NULL;
 
-  if (fp->_IO_save_base)
+  if (fp->_IO_save_base && fp->_IO_save_base != fp->_short_backupbuf)
     {
       free (fp->_IO_save_base);
       fp->_IO_save_base = NULL;
@@ -997,14 +1004,10 @@ _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;
-	      fp->_IO_save_base = bbuf;
-	      fp->_IO_save_end = fp->_IO_save_base + backup_size;
+	      /* We need to guarantee one pushback, so start with the built-in
+		 1-char buffer.  */
+	      fp->_IO_save_base = fp->_short_backupbuf;
+	      fp->_IO_save_end = fp->_IO_save_base + 1;
 	      fp->_IO_backup_base = fp->_IO_save_end;
 	    }
 	  fp->_IO_read_base = fp->_IO_read_ptr;
@@ -1022,7 +1025,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);
+	  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/stdio-common/Makefile b/stdio-common/Makefile
index a166eb7cf8..5e0ec763ff 100644
--- a/stdio-common/Makefile
+++ b/stdio-common/Makefile
@@ -275,6 +275,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
index 0000000000..bf36229a9c
--- /dev/null
+++ b/stdio-common/tst-ungetc-nomem.c
@@ -0,0 +1,94 @@
+/* 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 <stdio.h>
+#include <string.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-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.  */
+  bufsz = props.st_blksize + 2;
+
+  char *buf = xmalloc (bufsz);
+  memset (buf, 'a', bufsz);
+
+  size_t remaining = bufsz;
+  while (remaining > 0)
+    {
+      size_t done = fwrite (buf, sizeof (char), remaining, fp);
+      if (done == 0)
+	break;
+      remaining -= done;
+    }
+  fclose (fp);
+
+  /* Begin test.  */
+  fp = fopen (filename, "r");
+
+
+  /* The standard requires the first ungetc to always work.  */
+  fail = true;
+  TEST_COMPARE (ungetc('y', fp), 'y');
+
+  /* Now let the buffers get allocated to allow for subsequent tests.  */
+  fail = false;
+  TEST_COMPARE (fgetc (fp), 'y');
+  TEST_COMPARE (ungetc('y', fp), 'y');
+  TEST_COMPARE (fgetc (fp), 'y');
+
+  while (!feof (fp))
+    {
+      fail = true;
+      TEST_COMPARE (ungetc('y', fp), 'y');
+      fail = false;
+      TEST_COMPARE (fgetc (fp), 'y');
+      if (fgetc (fp) != 'a')
+	TEST_COMPARE (ferror (fp), 0);
+    }
+  return 0;
+}
+
+#include <support/test-driver.c>
-- 
2.46.0

[PATCH 3/3] ungetwc: Guarantee single char pushback

[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.

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

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;
     }
-- 
2.46.0

[ping][PATCH 0/3] Guarantee first pushback in ungetc and ungetwc

[Siddhesh Poyarekar]

Ping!

On 2024-11-08 12:14, Siddhesh Poyarekar wrote:
> The C standard requires the first pushback after read operations to
> succeed for ungetc and ungetwc.  Use a single-char buffer in the FILE
> struct to guarantee this, so that the calls are not subject to a malloc
> failure.
> 
> Tested on x86_64.
> 
> Siddhesh Poyarekar (3):
>    libio: make _IO_least_marker static
>    ungetc: Guarantee single char pushback
>    ungetwc: Guarantee single char pushback
> 
>   libio/Makefile                  |  1 +
>   libio/bits/types/struct_FILE.h  |  3 +-
>   libio/fileops.c                 | 21 +++-----
>   libio/genops.c                  | 35 ++++++------
>   libio/libio.h                   |  1 +
>   libio/tst-ungetwc-nomem.c       | 94 +++++++++++++++++++++++++++++++++
>   libio/wfileops.c                | 28 +++-------
>   libio/wgenops.c                 | 29 +++++-----
>   stdio-common/Makefile           |  1 +
>   stdio-common/tst-ungetc-nomem.c | 94 +++++++++++++++++++++++++++++++++
>   10 files changed, 240 insertions(+), 67 deletions(-)
>   create mode 100644 libio/tst-ungetwc-nomem.c
>   create mode 100644 stdio-common/tst-ungetc-nomem.c
> 

[ping2][PATCH 0/3] Guarantee first pushback in ungetc and ungetwc

[Siddhesh Poyarekar]

ping!

On 2024-11-08 12:14, Siddhesh Poyarekar wrote:
> The C standard requires the first pushback after read operations to
> succeed for ungetc and ungetwc.  Use a single-char buffer in the FILE
> struct to guarantee this, so that the calls are not subject to a malloc
> failure.
> 
> Tested on x86_64.
> 
> Siddhesh Poyarekar (3):
>    libio: make _IO_least_marker static
>    ungetc: Guarantee single char pushback
>    ungetwc: Guarantee single char pushback
> 
>   libio/Makefile                  |  1 +
>   libio/bits/types/struct_FILE.h  |  3 +-
>   libio/fileops.c                 | 21 +++-----
>   libio/genops.c                  | 35 ++++++------
>   libio/libio.h                   |  1 +
>   libio/tst-ungetwc-nomem.c       | 94 +++++++++++++++++++++++++++++++++
>   libio/wfileops.c                | 28 +++-------
>   libio/wgenops.c                 | 29 +++++-----
>   stdio-common/Makefile           |  1 +
>   stdio-common/tst-ungetc-nomem.c | 94 +++++++++++++++++++++++++++++++++
>   10 files changed, 240 insertions(+), 67 deletions(-)
>   create mode 100644 libio/tst-ungetwc-nomem.c
>   create mode 100644 stdio-common/tst-ungetc-nomem.c
> 

Re: [PATCH 1/3] libio: make _IO_least_marker static

[Florian Weimer]

* Siddhesh Poyarekar:

> Trivial cleanup to limit _IO_least_marker so that it's clear that it is
> unused outside of genops.
>
> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
> ---
>  libio/genops.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/libio/genops.c b/libio/genops.c
> index 6f20d49669..6545a78ad5 100644
> --- a/libio/genops.c
> +++ b/libio/genops.c
> @@ -135,10 +135,8 @@ libc_hidden_def (_IO_link_in)
>  
>  /* Return minimum _pos markers
>     Assumes the current get area is the main get area. */
> -ssize_t _IO_least_marker (FILE *fp, char *end_p);
> -
> -ssize_t
> -_IO_least_marker (FILE *fp, char *end_p)
> +static ssize_t
> +least_marker (FILE *fp, char *end_p)
>  {
>    ssize_t least_so_far = end_p - fp->_IO_read_base;
>    struct _IO_marker *mark;
> @@ -235,7 +233,7 @@ static int
>  save_for_backup (FILE *fp, char *end_p)
>  {
>    /* Append [_IO_read_base..end_p] to backup area. */
> -  ssize_t least_mark = _IO_least_marker (fp, end_p);
> +  ssize_t least_mark = least_marker (fp, end_p);
>    /* needed_size is how much space we need in the backup area. */
>    size_t needed_size = (end_p - fp->_IO_read_base) - least_mark;
>    /* FIXME: Dubious arithmetic if pointers are NULL */

It's sometimes helpful to have namespaced static functions, so I would
just delete the prototype and add the static keyword.

Thanks,
Florian

[committed] libio: make _IO_least_marker static

[Siddhesh Poyarekar]

Trivial cleanup to limit _IO_least_marker so that it's clear that it is
unused outside of genops.

Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
---
 libio/genops.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/libio/genops.c b/libio/genops.c
index 9f18861d7b..d7e35e67d5 100644
--- a/libio/genops.c
+++ b/libio/genops.c
@@ -135,9 +135,7 @@ libc_hidden_def (_IO_link_in)
 
 /* Return minimum _pos markers
    Assumes the current get area is the main get area. */
-ssize_t _IO_least_marker (FILE *fp, char *end_p);
-
-ssize_t
+static ssize_t
 _IO_least_marker (FILE *fp, char *end_p)
 {
   ssize_t least_so_far = end_p - fp->_IO_read_base;
-- 
2.46.0

Re: [PATCH 2/3] ungetc: Guarantee single char pushback

[Florian Weimer]

* Siddhesh Poyarekar:

> The C standard requires that ungetc guarantees at least one pushback, so
> put a single byte pushback buffer in the FILE struct to enable that.
>
> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
> ---
>  libio/bits/types/struct_FILE.h  |  3 +-
>  libio/fileops.c                 | 21 +++-----
>  libio/genops.c                  | 27 +++++-----
>  stdio-common/Makefile           |  1 +
>  stdio-common/tst-ungetc-nomem.c | 94 +++++++++++++++++++++++++++++++++
>  5 files changed, 118 insertions(+), 28 deletions(-)
>  create mode 100644 stdio-common/tst-ungetc-nomem.c
>
> diff --git a/libio/bits/types/struct_FILE.h b/libio/bits/types/struct_FILE.h
> index d8d26639d1..6b5295fce5 100644
> --- a/libio/bits/types/struct_FILE.h
> +++ b/libio/bits/types/struct_FILE.h
> @@ -94,8 +94,9 @@ struct _IO_FILE_complete
>    void *_freeres_buf;
>    struct _IO_FILE **_prevchain;
>    int _mode;
> +  char _short_backupbuf[1];
>    /* Make sure we don't get into trouble again.  */
> -  char _unused2[15 * sizeof (int) - 5 * sizeof (void *)];
> +  char _unused2[15 * sizeof (int) - 5 * sizeof (void *) - sizeof (char)];
>  };

There's unused space after _shortbuf (even on m68k), so please use that
instead.  It avoids issues with legacy streams not having the extended
part.

It avoids a conflict with a patch Tulio is working on, too.

>  /* These macros are used by bits/stdio.h and internal headers.  */
> diff --git a/libio/fileops.c b/libio/fileops.c
> index 4db4a76f75..67e57e0d8c 100644
> --- a/libio/fileops.c
> +++ b/libio/fileops.c
> @@ -478,11 +478,8 @@ _IO_new_file_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);
>      }

So … why is it okay to call _IO_free_backup_area here, given the many
side effects it has?

I suggest to rename free_backup_buf to _IO_free_backup_buf, make it
extern + attribute_hidden, and only replace the relevant free calls.

> diff --git a/libio/genops.c b/libio/genops.c
> index 6545a78ad5..02b38fbc9a 100644
> --- a/libio/genops.c
> +++ b/libio/genops.c
> @@ -48,6 +48,13 @@ flush_cleanup (void *not_used)
>  }
>  #endif
>  
> +static void
> +free_backup_buf (FILE *fp, char *ptr)
> +{
> +  if (fp->_short_backupbuf != ptr)
> +    free (ptr);
> +}

I'd prefer if ptr comes first, as the scrutinee.

> @@ -634,7 +641,7 @@ _IO_default_finish (FILE *fp, int dummy)
>    for (mark = fp->_markers; mark != NULL; mark = mark->_next)
>      mark->_sbuf = NULL;
>  
> -  if (fp->_IO_save_base)
> +  if (fp->_IO_save_base && fp->_IO_save_base != fp->_short_backupbuf)
>      {
>        free (fp->_IO_save_base);
>        fp->_IO_save_base = NULL;

This skips the NULL assignment if _IO_save_base is _short_backupbuf.
That doesn't look right.

>  	  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;
> -	      fp->_IO_save_base = bbuf;
> -	      fp->_IO_save_end = fp->_IO_save_base + backup_size;
> +	      /* We need to guarantee one pushback, so start with the built-in
> +		 1-char buffer.  */
> +	      fp->_IO_save_base = fp->_short_backupbuf;
> +	      fp->_IO_save_end = fp->_IO_save_base + 1;
>  	      fp->_IO_backup_base = fp->_IO_save_end;
>  	    }

Please use the 1-character buffer only as a fallback, to avoid changing
the behavior if malloc does not fail.

> diff --git a/stdio-common/tst-ungetc-nomem.c b/stdio-common/tst-ungetc-nomem.c
> new file mode 100644
> index 0000000000..bf36229a9c
> --- /dev/null
> +++ b/stdio-common/tst-ungetc-nomem.c

> +static bool fail = false;

Should be volatile, to inhibit compiler optimizations around malloc.

> +
> +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-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.  */
> +  bufsz = props.st_blksize + 2;
> +
> +  char *buf = xmalloc (bufsz);
> +  memset (buf, 'a', bufsz);
> +
> +  size_t remaining = bufsz;
> +  while (remaining > 0)
> +    {
> +      size_t done = fwrite (buf, sizeof (char), remaining, fp);
> +      if (done == 0)
> +	break;
> +      remaining -= done;
> +    }

Why the retry loop?  I don't think fwrite works this way.

> +  fclose (fp);

xfclose?

> +  /* Begin test.  */
> +  fp = fopen (filename, "r");

xfopen?

> +
> +
> +  /* The standard requires the first ungetc to always work.  */
> +  fail = true;
> +  TEST_COMPARE (ungetc('y', fp), 'y');

Missing space after ungetc.

> +  /* Now let the buffers get allocated to allow for subsequent tests.  */
> +  fail = false;
> +  TEST_COMPARE (fgetc (fp), 'y');
> +  TEST_COMPARE (ungetc('y', fp), 'y');
> +  TEST_COMPARE (fgetc (fp), 'y');

This doesn't exercise the case where we switch from a 1-byte buffer to a
larger buffer.

Thanks,
Florian

Re: [PATCH 3/3] ungetwc: Guarantee single char pushback

[Florian Weimer]

* 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

Re: [PATCH 2/3] ungetc: Guarantee single char pushback

[Siddhesh Poyarekar]

On 2024-11-28 08:45, Florian Weimer wrote:
> * Siddhesh Poyarekar:
> 
>> The C standard requires that ungetc guarantees at least one pushback, so
>> put a single byte pushback buffer in the FILE struct to enable that.
>>
>> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
>> ---
>>   libio/bits/types/struct_FILE.h  |  3 +-
>>   libio/fileops.c                 | 21 +++-----
>>   libio/genops.c                  | 27 +++++-----
>>   stdio-common/Makefile           |  1 +
>>   stdio-common/tst-ungetc-nomem.c | 94 +++++++++++++++++++++++++++++++++
>>   5 files changed, 118 insertions(+), 28 deletions(-)
>>   create mode 100644 stdio-common/tst-ungetc-nomem.c
>>
>> diff --git a/libio/bits/types/struct_FILE.h b/libio/bits/types/struct_FILE.h
>> index d8d26639d1..6b5295fce5 100644
>> --- a/libio/bits/types/struct_FILE.h
>> +++ b/libio/bits/types/struct_FILE.h
>> @@ -94,8 +94,9 @@ struct _IO_FILE_complete
>>     void *_freeres_buf;
>>     struct _IO_FILE **_prevchain;
>>     int _mode;
>> +  char _short_backupbuf[1];
>>     /* Make sure we don't get into trouble again.  */
>> -  char _unused2[15 * sizeof (int) - 5 * sizeof (void *)];
>> +  char _unused2[15 * sizeof (int) - 5 * sizeof (void *) - sizeof (char)];
>>   };
> 
> There's unused space after _shortbuf (even on m68k), so please use that
> instead.  It avoids issues with legacy streams not having the extended
> part.

OK.

> It avoids a conflict with a patch Tulio is working on, too.
> 
>>   /* These macros are used by bits/stdio.h and internal headers.  */
>> diff --git a/libio/fileops.c b/libio/fileops.c
>> index 4db4a76f75..67e57e0d8c 100644
>> --- a/libio/fileops.c
>> +++ b/libio/fileops.c
>> @@ -478,11 +478,8 @@ _IO_new_file_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);
>>       }
> 
> So … why is it okay to call _IO_free_backup_area here, given the many
> side effects it has?
> 
> I suggest to rename free_backup_buf to _IO_free_backup_buf, make it
> extern + attribute_hidden, and only replace the relevant free calls.

OK, that's what I did first, but it seemed more coherent to always fully 
restore state from backup to main area instead.

>> diff --git a/libio/genops.c b/libio/genops.c
>> index 6545a78ad5..02b38fbc9a 100644
>> --- a/libio/genops.c
>> +++ b/libio/genops.c
>> @@ -48,6 +48,13 @@ flush_cleanup (void *not_used)
>>   }
>>   #endif
>>   
>> +static void
>> +free_backup_buf (FILE *fp, char *ptr)
>> +{
>> +  if (fp->_short_backupbuf != ptr)
>> +    free (ptr);
>> +}
> 
> I'd prefer if ptr comes first, as the scrutinee.

OK.

> 
>> @@ -634,7 +641,7 @@ _IO_default_finish (FILE *fp, int dummy)
>>     for (mark = fp->_markers; mark != NULL; mark = mark->_next)
>>       mark->_sbuf = NULL;
>>   
>> -  if (fp->_IO_save_base)
>> +  if (fp->_IO_save_base && fp->_IO_save_base != fp->_short_backupbuf)
>>       {
>>         free (fp->_IO_save_base);
>>         fp->_IO_save_base = NULL;
> 
> This skips the NULL assignment if _IO_save_base is _short_backupbuf.
> That doesn't look right.

Uhmm, yeah.  Fixed.

>>   	  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;
>> -	      fp->_IO_save_base = bbuf;
>> -	      fp->_IO_save_end = fp->_IO_save_base + backup_size;
>> +	      /* We need to guarantee one pushback, so start with the built-in
>> +		 1-char buffer.  */
>> +	      fp->_IO_save_base = fp->_short_backupbuf;
>> +	      fp->_IO_save_end = fp->_IO_save_base + 1;
>>   	      fp->_IO_backup_base = fp->_IO_save_end;
>>   	    }
> 
> Please use the 1-character buffer only as a fallback, to avoid changing
> the behavior if malloc does not fail.

But doesn't this become a micro-optimization too for 1-char pushbacks? 
I'll admit I don't know if that's the predominant case, but it seems 
like it should be a pretty common one.

>> diff --git a/stdio-common/tst-ungetc-nomem.c b/stdio-common/tst-ungetc-nomem.c
>> new file mode 100644
>> index 0000000000..bf36229a9c
>> --- /dev/null
>> +++ b/stdio-common/tst-ungetc-nomem.c
> 
>> +static bool fail = false;
> 
> Should be volatile, to inhibit compiler optimizations around malloc.

OK.

>> +
>> +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-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.  */
>> +  bufsz = props.st_blksize + 2;
>> +
>> +  char *buf = xmalloc (bufsz);
>> +  memset (buf, 'a', bufsz);
>> +
>> +  size_t remaining = bufsz;
>> +  while (remaining > 0)
>> +    {
>> +      size_t done = fwrite (buf, sizeof (char), remaining, fp);
>> +      if (done == 0)
>> +	break;
>> +      remaining -= done;
>> +    }
> 
> Why the retry loop?  I don't think fwrite works this way.

Eek, right, sorry.

>> +  fclose (fp);
> 
> xfclose?
> 
>> +  /* Begin test.  */
>> +  fp = fopen (filename, "r");
> 
> xfopen?

OK.

> 
>> +
>> +
>> +  /* The standard requires the first ungetc to always work.  */
>> +  fail = true;
>> +  TEST_COMPARE (ungetc('y', fp), 'y');
> 
> Missing space after ungetc.

Fixed.

> 
>> +  /* Now let the buffers get allocated to allow for subsequent tests.  */
>> +  fail = false;
>> +  TEST_COMPARE (fgetc (fp), 'y');
>> +  TEST_COMPARE (ungetc('y', fp), 'y');
>> +  TEST_COMPARE (fgetc (fp), 'y');
> 
> This doesn't exercise the case where we switch from a 1-byte buffer to a
> larger buffer.

OK, I'll add this.

Thanks,
Sid

Re: [PATCH 2/3] ungetc: Guarantee single char pushback

[Florian Weimer]

* Siddhesh Poyarekar:

>> So … why is it okay to call _IO_free_backup_area here, given the many
>> side effects it has?  I suggest to rename free_backup_buf to
>> _IO_free_backup_buf, make it extern + attribute_hidden, and only
>> replace the relevant free calls.
>
> OK, that's what I did first, but it seemed more coherent to always
> fully restore state from backup to main area instead.

Maybe, but this is libio, so I think we should minimize changes.

>>>   	  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;
>>> -	      fp->_IO_save_base = bbuf;
>>> -	      fp->_IO_save_end = fp->_IO_save_base + backup_size;
>>> +	      /* We need to guarantee one pushback, so start with the built-in
>>> +		 1-char buffer.  */
>>> +	      fp->_IO_save_base = fp->_short_backupbuf;
>>> +	      fp->_IO_save_end = fp->_IO_save_base + 1;
>>>   	      fp->_IO_backup_base = fp->_IO_save_end;
>>>   	    }
>> Please use the 1-character buffer only as a fallback, to avoid
>> changing the behavior if malloc does not fail.
>
> But doesn't this become a micro-optimization too for 1-char pushbacks?
> I'll admit I don't know if that's the predominant case, but it seems
> like it should be a pretty common one.

We currently have little experience with that reallocation path, and
with the 1-character buffer, we suddenly start exercising that quite
heavily if there is more than one ungetc call.  The optimization could
be a follow-up change.

Thanks,
Florian

Re: [PATCH 2/3] ungetc: Guarantee single char pushback

[Siddhesh Poyarekar]

On 2024-11-28 12:23, Florian Weimer wrote:
> * Siddhesh Poyarekar:
> 
>>> So … why is it okay to call _IO_free_backup_area here, given the many
>>> side effects it has?  I suggest to rename free_backup_buf to
>>> _IO_free_backup_buf, make it extern + attribute_hidden, and only
>>> replace the relevant free calls.
>>
>> OK, that's what I did first, but it seemed more coherent to always
>> fully restore state from backup to main area instead.
> 
> Maybe, but this is libio, so I think we should minimize changes.

This is libio, we should sneakily rewrite all of it ;)

>>>>    	  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;
>>>> -	      fp->_IO_save_base = bbuf;
>>>> -	      fp->_IO_save_end = fp->_IO_save_base + backup_size;
>>>> +	      /* We need to guarantee one pushback, so start with the built-in
>>>> +		 1-char buffer.  */
>>>> +	      fp->_IO_save_base = fp->_short_backupbuf;
>>>> +	      fp->_IO_save_end = fp->_IO_save_base + 1;
>>>>    	      fp->_IO_backup_base = fp->_IO_save_end;
>>>>    	    }
>>> Please use the 1-character buffer only as a fallback, to avoid
>>> changing the behavior if malloc does not fail.
>>
>> But doesn't this become a micro-optimization too for 1-char pushbacks?
>> I'll admit I don't know if that's the predominant case, but it seems
>> like it should be a pretty common one.
> 
> We currently have little experience with that reallocation path, and
> with the 1-character buffer, we suddenly start exercising that quite
> heavily if there is more than one ungetc call.  The optimization could
> be a follow-up change.

OK, I'll take the more conservative route with v2 :)

Thanks,
Sid

Note to self: be even more sneaky...

Re: [PATCH 2/3] ungetc: Guarantee single char pushback

[Maciej W. Rozycki]

On Thu, 28 Nov 2024, Florian Weimer wrote:

> > diff --git a/libio/bits/types/struct_FILE.h b/libio/bits/types/struct_FILE.h
> > index d8d26639d1..6b5295fce5 100644
> > --- a/libio/bits/types/struct_FILE.h
> > +++ b/libio/bits/types/struct_FILE.h
> > @@ -94,8 +94,9 @@ struct _IO_FILE_complete
> >    void *_freeres_buf;
> >    struct _IO_FILE **_prevchain;
> >    int _mode;
> > +  char _short_backupbuf[1];
> >    /* Make sure we don't get into trouble again.  */
> > -  char _unused2[15 * sizeof (int) - 5 * sizeof (void *)];
> > +  char _unused2[15 * sizeof (int) - 5 * sizeof (void *) - sizeof (char)];
> >  };
> 
> There's unused space after _shortbuf (even on m68k), so please use that
> instead.  It avoids issues with legacy streams not having the extended
> part.

 There isn't AFAICT e.g. on o32 MIPS:

struct _IO_FILE {
	int                        _flags;               /*     0     4 */
	char *                     _IO_read_ptr;         /*     4     4 */
	char *                     _IO_read_end;         /*     8     4 */
	char *                     _IO_read_base;        /*    12     4 */
	char *                     _IO_write_base;       /*    16     4 */
	char *                     _IO_write_ptr;        /*    20     4 */
	char *                     _IO_write_end;        /*    24     4 */
	char *                     _IO_buf_base;         /*    28     4 */
	char *                     _IO_buf_end;          /*    32     4 */
	char *                     _IO_save_base;        /*    36     4 */
	char *                     _IO_backup_base;      /*    40     4 */
	char *                     _IO_save_end;         /*    44     4 */
	struct _IO_marker *        _markers;             /*    48     4 */
	struct _IO_FILE *          _chain;               /*    52     4 */
	int                        _fileno;              /*    56     4 */
	int                        _flags2;              /*    60     4 */
	__off_t                    _old_offset;          /*    64     4 */
	short unsigned int         _cur_column;          /*    68     2 */
	signed char                _vtable_offset;       /*    70     1 */
	char                       _shortbuf[1];         /*    71     1 */
	_IO_lock_t *               _lock;                /*    72     4 */

	/* size: 76, cachelines: 1, members: 21 */
	/* last cacheline: 76 bytes */
};

The structure is fully packed and there's no padding in the legacy part 
I'm afraid, and I guess it's the case for the majority of 32-bit targets, 
which don't imply alignment beyond 4 bytes for pointers.  Have I missed 
anything?

 We have bits available in _flags2 though, shall we consume 8 of them?  It 
might not end up pretty, but...  And we have an established practice of 
reusing this member.

  Maciej

Re: [PATCH 2/3] ungetc: Guarantee single char pushback

[Florian Weimer]

* Maciej W. Rozycki:

> The structure is fully packed and there's no padding in the legacy part 
> I'm afraid, and I guess it's the case for the majority of 32-bit targets, 
> which don't imply alignment beyond 4 bytes for pointers.  Have I missed 
> anything?

No, you are right.  So the extension area has to be used.  I think we
should fail ungetc if the malloc call fails and the extension area is
unavailable, instead of resorting to heroic bit-stuffing.

Thanks,
Florian

Re: [PATCH 2/3] ungetc: Guarantee single char pushback

[Siddhesh Poyarekar]

On 2024-11-29 02:02, Florian Weimer wrote:
> * Maciej W. Rozycki:
> 
>> The structure is fully packed and there's no padding in the legacy part
>> I'm afraid, and I guess it's the case for the majority of 32-bit targets,
>> which don't imply alignment beyond 4 bytes for pointers.  Have I missed
>> anything?
> 
> No, you are right.  So the extension area has to be used.  I think we
> should fail ungetc if the malloc call fails and the extension area is
> unavailable, instead of resorting to heroic bit-stuffing.

So basically the original struct layout I had proposed plus this in 
genops.c should do it?

diff --git a/libio/genops.c b/libio/genops.c
index d7e35e67d5..5b8bb52ec3 100644
--- a/libio/genops.c
+++ b/libio/genops.c
@@ -1002,7 +1002,14 @@ _IO_default_pbackfail (FILE *fp, int c)
               int backup_size = 128;
               char *bbuf = (char *) malloc (backup_size);
               if (bbuf == NULL)
-               return EOF;
+               {
+#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
+                 if (__glibc_unlikely (_IO_vtable_offset (fp) != 0))
+                   return EOF;
+#endif
+                 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;

Re: [PATCH 2/3] ungetc: Guarantee single char pushback

[Maciej W. Rozycki]

On Fri, 29 Nov 2024, Florian Weimer wrote:

> > The structure is fully packed and there's no padding in the legacy part 
> > I'm afraid, and I guess it's the case for the majority of 32-bit targets, 
> > which don't imply alignment beyond 4 bytes for pointers.  Have I missed 
> > anything?
> 
> No, you are right.  So the extension area has to be used.  I think we
> should fail ungetc if the malloc call fails and the extension area is
> unavailable, instead of resorting to heroic bit-stuffing.

 What's wrong with reusing flags2?  We have 25 contiguous bits left and at 
the rate we've been consuming them here we'll need another 40 years before 
we need the last 8.  Besides, we've released a couple already and isn't it 
internal stuff anyway we can rearrange on a whim?

  Maciej

Re: [PATCH 2/3] ungetc: Guarantee single char pushback

[Siddhesh Poyarekar]

On 2024-11-29 07:15, Maciej W. Rozycki wrote:
> On Fri, 29 Nov 2024, Florian Weimer wrote:
> 
>>> The structure is fully packed and there's no padding in the legacy part
>>> I'm afraid, and I guess it's the case for the majority of 32-bit targets,
>>> which don't imply alignment beyond 4 bytes for pointers.  Have I missed
>>> anything?
>>
>> No, you are right.  So the extension area has to be used.  I think we
>> should fail ungetc if the malloc call fails and the extension area is
>> unavailable, instead of resorting to heroic bit-stuffing.
> 
>   What's wrong with reusing flags2?  We have 25 contiguous bits left and at
> the rate we've been consuming them here we'll need another 40 years before
> we need the last 8.  Besides, we've released a couple already and isn't it
> internal stuff anyway we can rearrange on a whim?

I need to stuff a whole char in there, for which we'll have to put 
flags2 and the buf into a union { int flags2; char shortbuf[sizeof 
(int)];} and then take care only to use the bottom char in that buffer. 
That seems like too much cruft to support legacy uses IMO.

Sid

Re: [PATCH 2/3] ungetc: Guarantee single char pushback

[Florian Weimer]

* Siddhesh Poyarekar:

> On 2024-11-29 02:02, Florian Weimer wrote:
>> * Maciej W. Rozycki:
>> 
>>> The structure is fully packed and there's no padding in the legacy part
>>> I'm afraid, and I guess it's the case for the majority of 32-bit targets,
>>> which don't imply alignment beyond 4 bytes for pointers.  Have I missed
>>> anything?
>> No, you are right.  So the extension area has to be used.  I think
>> we
>> should fail ungetc if the malloc call fails and the extension area is
>> unavailable, instead of resorting to heroic bit-stuffing.
>
> So basically the original struct layout I had proposed plus this in
> genops.c should do it?
>
> diff --git a/libio/genops.c b/libio/genops.c
> index d7e35e67d5..5b8bb52ec3 100644
> --- a/libio/genops.c
> +++ b/libio/genops.c
> @@ -1002,7 +1002,14 @@ _IO_default_pbackfail (FILE *fp, int c)
>               int backup_size = 128;
>               char *bbuf = (char *) malloc (backup_size);
>               if (bbuf == NULL)
> -               return EOF;
> +               {
> +#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
> +                 if (__glibc_unlikely (_IO_vtable_offset (fp) != 0))
> +                   return EOF;
> +#endif
> +                 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;

Maybe without the SHLIB_COMPAT, I think the other checks don't have that
condition.  But yes, I think that's the way to check it.

Thanks,
Florian

Re: [PATCH 3/3] ungetwc: Guarantee single char pushback

[Siddhesh Poyarekar]

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

[PATCH v2] ungetc: Guarantee single char pushback

[Siddhesh Poyarekar]

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

Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
---
Changes from v1:

- Drop ungetwc from scope of the patchset
- Fixed nits
- Retain old behaviour for legacy applications
- Minimize changes to fileops
- Namespace-ize free_backup_buf
- Add a test to verify that the subsequent malloc failure results in ungetc
  failure too
- Add GNU Toolchain Authors copyright notice.

 libio/bits/types/struct_FILE.h  |  4 +-
 libio/fileops.c                 |  7 ++-
 libio/genops.c                  | 26 +++++++--
 libio/libioP.h                  |  3 +
 stdio-common/Makefile           |  2 +
 stdio-common/tst-ungetc-nomem.c | 98 +++++++++++++++++++++++++++++++++
 6 files changed, 130 insertions(+), 10 deletions(-)
 create mode 100644 stdio-common/tst-ungetc-nomem.c

diff --git a/libio/bits/types/struct_FILE.h b/libio/bits/types/struct_FILE.h
index d8d26639d1..5d08509078 100644
--- 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
@@ -94,8 +95,9 @@ struct _IO_FILE_complete
   void *_freeres_buf;
   struct _IO_FILE **_prevchain;
   int _mode;
+  char _short_backupbuf[1];
   /* Make sure we don't get into trouble again.  */
-  char _unused2[15 * sizeof (int) - 5 * sizeof (void *)];
+  char _unused2[15 * sizeof (int) - 5 * sizeof (void *) - sizeof (char)];
 };
 
 /* These macros are used by bits/stdio.h and internal headers.  */
diff --git a/libio/fileops.c b/libio/fileops.c
index 759d737ec7..d49e489f55 100644
--- 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 d7e35e67d5..996daf0ad8 100644
--- 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
@@ -48,6 +49,13 @@ flush_cleanup (void *not_used)
 }
 #endif
 
+void
+_IO_free_backup_buf (FILE *fp, char *ptr)
+{
+  if (ptr != fp->_short_backupbuf)
+    free (ptr);
+}
+
 /* Fields in struct _IO_FILE after the _lock field are internal to
    glibc and opaque to applications.  We can change them as long as
    the size of struct _IO_FILE is unchanged, which is checked as the
@@ -212,7 +220,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 +268,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 +644,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 +1006,17 @@ _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, except for legacy code where
+		     we don't have the extended part of FILE.  */
+		  if (__glibc_unlikely (_IO_vtable_offset (fp) != 0))
+		    return EOF;
+		  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 +1036,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 34bf91fcd8..90ef8e90be 100644
--- 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
@@ -357,6 +358,8 @@ typedef FILE *_IO_ITER;
 
 /* Generic functions */
 
+extern void _IO_free_backup_buf (FILE *, char *);
+libc_hidden_proto (_IO_free_backup_buf)
 extern void _IO_switch_to_main_get_area (FILE *) __THROW;
 extern void _IO_switch_to_backup_area (FILE *) __THROW;
 extern int _IO_switch_to_get_mode (FILE *);
diff --git a/stdio-common/Makefile b/stdio-common/Makefile
index e76e40e587..b1a04fd064 100644
--- 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
index 0000000000..73b88e2e1c
--- /dev/null
+++ b/stdio-common/tst-ungetc-nomem.c
@@ -0,0 +1,98 @@
+/* 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 <stdio.h>
+#include <string.h>
+#include <support/check.h>
+#include <support/support.h>
+#include <support/temp_file.h>
+#include <support/xstdio.h>
+
+extern void *__libc_malloc (size_t)
+     __attribute__ ((malloc)) __attribute__ ((alloc_size (1)));
+
+static volatile 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-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.  */
+  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: 5m\n");
+  xfclose (fp);
+
+  /* Begin test.  */
+  fp = xfopen (filename, "r");
+
+
+  /* The standard requires the first ungetc to always work.  */
+  fail = true;
+  TEST_COMPARE (ungetc ('y', fp), 'y');
+
+  /* Now let the buffers get allocated to allow for subsequent tests.  */
+  fail = false;
+  TEST_COMPARE (fgetc (fp), 'y');
+  TEST_COMPARE (ungetc ('y', fp), 'y');
+  TEST_COMPARE (fgetc (fp), 'y');
+
+  while (!feof (fp))
+    {
+      fail = true;
+      TEST_COMPARE (ungetc ('y', fp), 'y');
+      /* This will result in resizing, which should fail.  */
+      TEST_COMPARE (ungetc ('y', fp), EOF);
+      fail = false;
+      TEST_COMPARE (fgetc (fp), 'y');
+      /* The second fgetc should return a char from the file or EOF.  */
+      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>
-- 
2.46.0

Re: [PATCH 2/3] ungetc: Guarantee single char pushback

[Maciej W. Rozycki]

On Fri, 29 Nov 2024, Siddhesh Poyarekar wrote:

> >   What's wrong with reusing flags2?  We have 25 contiguous bits left and at
> > the rate we've been consuming them here we'll need another 40 years before
> > we need the last 8.  Besides, we've released a couple already and isn't it
> > internal stuff anyway we can rearrange on a whim?
> 
> I need to stuff a whole char in there, for which we'll have to put flags2 and
> the buf into a union { int flags2; char shortbuf[sizeof (int)];} and then take
> care only to use the bottom char in that buffer. That seems like too much
> cruft to support legacy uses IMO.

 Couldn't we just redefine flags2 as short and have two extra char members 
for free?  Is there any ABI out there that packs an int differently from a 
short and a pair of char members provided the struct requires alignment of 
at least 4 bytes already and there's no padding for the int?

  Maciej

Re: [PATCH v2] ungetc: Guarantee single char pushback

[Florian Weimer]

* Siddhesh Poyarekar:

> diff --git a/libio/genops.c b/libio/genops.c
> index d7e35e67d5..996daf0ad8 100644
> --- 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
> @@ -48,6 +49,13 @@ flush_cleanup (void *not_used)
>  }
>  #endif
>  
> +void
> +_IO_free_backup_buf (FILE *fp, char *ptr)
> +{
> +  if (ptr != fp->_short_backupbuf)
> +    free (ptr);
> +}

Shouldn't this check for the availability of the _short_backupbuf field,
too?

And with that in place, it might make sense to update oldfileops.c in
similar places, just in case something weird happens with the vtables
and we end up the old vtables code on new file handles.

For similar reasons, please convert the free calls for wide streams to
_IO_free_backup_buf, too.  We definitely have weird vtable interactions
for those.

> diff --git a/stdio-common/tst-ungetc-nomem.c b/stdio-common/tst-ungetc-nomem.c
> new file mode 100644
> index 0000000000..73b88e2e1c
> --- /dev/null
> +++ b/stdio-common/tst-ungetc-nomem.c

> +    FAIL_EXIT1 ("fwrite failed: 5m\n");

Typo: %m

> +  while (!feof (fp))
> +    {
> +      fail = true;
> +      TEST_COMPARE (ungetc ('y', fp), 'y');
> +      /* This will result in resizing, which should fail.  */
> +      TEST_COMPARE (ungetc ('y', fp), EOF);
> +      fail = false;
> +      TEST_COMPARE (fgetc (fp), 'y');

Hmm.  So ungetc doesn't set the error indicator?  POSIX doesn't mention
it, so it seems okay.

When I mentioned multiple ungetc calls in a row, I meant that first
force a single-byte buffer (with fail = true), and then re-enable malloc
and force switch to a larger buffer with multiple ungetc calls.

Thanks,
Florian

Re: [PATCH v2] ungetc: Guarantee single char pushback

[Siddhesh Poyarekar]

On 2024-12-02 16:18, Florian Weimer wrote:
> * Siddhesh Poyarekar:
> 
>> diff --git a/libio/genops.c b/libio/genops.c
>> index d7e35e67d5..996daf0ad8 100644
>> --- 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
>> @@ -48,6 +49,13 @@ flush_cleanup (void *not_used)
>>   }
>>   #endif
>>   
>> +void
>> +_IO_free_backup_buf (FILE *fp, char *ptr)
>> +{
>> +  if (ptr != fp->_short_backupbuf)
>> +    free (ptr);
>> +}
> 
> Shouldn't this check for the availability of the _short_backupbuf field,
> too?

Uhmm, I suppose so, so:

if (_IO_vtable_offset (fp) == 0 && ptr != fp->_short_backupbuf)
   ...

> And with that in place, it might make sense to update oldfileops.c in
> similar places, just in case something weird happens with the vtables
> and we end up the old vtables code on new file handles.

I'll take a look and clean up.

> For similar reasons, please convert the free calls for wide streams to
> _IO_free_backup_buf, too.  We definitely have weird vtable interactions
> for those.

Yes, I just wanted to separate it out from this change.  Would you 
rather prefer that I club the two changes together like I did the first 
time?

>> diff --git a/stdio-common/tst-ungetc-nomem.c b/stdio-common/tst-ungetc-nomem.c
>> new file mode 100644
>> index 0000000000..73b88e2e1c
>> --- /dev/null
>> +++ b/stdio-common/tst-ungetc-nomem.c
> 
>> +    FAIL_EXIT1 ("fwrite failed: 5m\n");
> 
> Typo: %m

Eep :/

>> +  while (!feof (fp))
>> +    {
>> +      fail = true;
>> +      TEST_COMPARE (ungetc ('y', fp), 'y');
>> +      /* This will result in resizing, which should fail.  */
>> +      TEST_COMPARE (ungetc ('y', fp), EOF);
>> +      fail = false;
>> +      TEST_COMPARE (fgetc (fp), 'y');
> 
> Hmm.  So ungetc doesn't set the error indicator?  POSIX doesn't mention
> it, so it seems okay.
> 
> When I mentioned multiple ungetc calls in a row, I meant that first
> force a single-byte buffer (with fail = true), and then re-enable malloc
> and force switch to a larger buffer with multiple ungetc calls.

Ah, so a *successful* switch after the transient failure; OK I'll add 
that and also see if there are any other combinations to test.

Thanks,
Sid

Re: [PATCH v2] ungetc: Guarantee single char pushback

[Florian Weimer]

* Siddhesh Poyarekar:

> On 2024-12-02 16:18, Florian Weimer wrote:
>> * Siddhesh Poyarekar:
>> 
>>> diff --git a/libio/genops.c b/libio/genops.c
>>> index d7e35e67d5..996daf0ad8 100644
>>> --- 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
>>> @@ -48,6 +49,13 @@ flush_cleanup (void *not_used)
>>>   }
>>>   #endif
>>>   +void
>>> +_IO_free_backup_buf (FILE *fp, char *ptr)
>>> +{
>>> +  if (ptr != fp->_short_backupbuf)
>>> +    free (ptr);
>>> +}
>> Shouldn't this check for the availability of the _short_backupbuf
>> field,
>> too?
>
> Uhmm, I suppose so, so:
>
> if (_IO_vtable_offset (fp) == 0 && ptr != fp->_short_backupbuf)
>   ...

Right.

>> For similar reasons, please convert the free calls for wide streams to
>> _IO_free_backup_buf, too.  We definitely have weird vtable interactions
>> for those.
>
> Yes, I just wanted to separate it out from this change.  Would you
> rather prefer that I club the two changes together like I did the
> first time?

Note that this is not about ungetwc support, it's about breaking stuff
on the wide character side due to the ungetc changes.

Thanks,
Florian

[PATCH v3] ungetc: Guarantee single char pushback

[Siddhesh Poyarekar]

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

Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
---
Changes from v2:
- Fixed nits
- Used _IO_free_backup_buf in oldfileops and wfileops as well.
- Enhanced test to try some more cases

Changes from v1:

- Drop ungetwc from scope of the patchset
- Fixed nits
- Retain old behaviour for legacy applications
- Minimize changes to fileops
- Namespace-ize free_backup_buf
- Add a test to verify that the subsequent malloc failure results in ungetc
  failure too
- Add GNU Toolchain Authors copyright notice.

 libio/bits/types/struct_FILE.h  |   4 +-
 libio/fileops.c                 |   7 +-
 libio/genops.c                  |  26 ++++++--
 libio/libioP.h                  |   3 +
 libio/oldfileops.c              |   5 +-
 libio/wfileops.c                |   3 +-
 stdio-common/Makefile           |   2 +
 stdio-common/tst-ungetc-nomem.c | 115 ++++++++++++++++++++++++++++++++
 8 files changed, 152 insertions(+), 13 deletions(-)
 create mode 100644 stdio-common/tst-ungetc-nomem.c

diff --git a/libio/bits/types/struct_FILE.h b/libio/bits/types/struct_FILE.h
index d8d26639d1..5d08509078 100644
--- 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
@@ -94,8 +95,9 @@ struct _IO_FILE_complete
   void *_freeres_buf;
   struct _IO_FILE **_prevchain;
   int _mode;
+  char _short_backupbuf[1];
   /* Make sure we don't get into trouble again.  */
-  char _unused2[15 * sizeof (int) - 5 * sizeof (void *)];
+  char _unused2[15 * sizeof (int) - 5 * sizeof (void *) - sizeof (char)];
 };
 
 /* These macros are used by bits/stdio.h and internal headers.  */
diff --git a/libio/fileops.c b/libio/fileops.c
index 759d737ec7..d49e489f55 100644
--- 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 d7e35e67d5..1a782508e4 100644
--- 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
@@ -48,6 +49,13 @@ flush_cleanup (void *not_used)
 }
 #endif
 
+void
+_IO_free_backup_buf (FILE *fp, char *ptr)
+{
+  if (_IO_vtable_offset (fp) == 0 && ptr != fp->_short_backupbuf)
+    free (ptr);
+}
+
 /* Fields in struct _IO_FILE after the _lock field are internal to
    glibc and opaque to applications.  We can change them as long as
    the size of struct _IO_FILE is unchanged, which is checked as the
@@ -212,7 +220,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 +268,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 +644,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 +1006,17 @@ _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, except for legacy code where
+		     we don't have the extended part of FILE.  */
+		  if (__glibc_unlikely (_IO_vtable_offset (fp) != 0))
+		    return EOF;
+		  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 +1036,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 34bf91fcd8..90ef8e90be 100644
--- 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
@@ -357,6 +358,8 @@ typedef FILE *_IO_ITER;
 
 /* Generic functions */
 
+extern void _IO_free_backup_buf (FILE *, char *);
+libc_hidden_proto (_IO_free_backup_buf)
 extern void _IO_switch_to_main_get_area (FILE *) __THROW;
 extern void _IO_switch_to_backup_area (FILE *) __THROW;
 extern int _IO_switch_to_get_mode (FILE *);
diff --git a/libio/oldfileops.c b/libio/oldfileops.c
index 8f775c9094..03f4d76a57 100644
--- 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 16beab1f3a..a96bfa589b 100644
--- 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 e76e40e587..b1a04fd064 100644
--- 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
index 0000000000..49db33fff3
--- /dev/null
+++ b/stdio-common/tst-ungetc-nomem.c
@@ -0,0 +1,115 @@
+/* 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 <stdio.h>
+#include <string.h>
+#include <support/check.h>
+#include <support/support.h>
+#include <support/temp_file.h>
+#include <support/xstdio.h>
+
+extern void *__libc_malloc (size_t)
+     __attribute__ ((malloc)) __attribute__ ((alloc_size (1)));
+
+static volatile 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-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.  */
+  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 ('y', fp), 'y');
+
+      /* 2: This will result in resizing, which should fail.  */
+      TEST_COMPARE (ungetc ('w', 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 ('x', fp), 'x');
+
+      /* 4: And fail again because this again forces an alloc, which fails.  */
+      fail = true;
+      TEST_COMPARE (ungetc ('x', 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 ('x', fp), 'x');
+      fail = true;
+      TEST_COMPARE (ungetc ('x', fp), 'x');
+
+      /* Drain out the x's.  */
+      TEST_COMPARE (fgetc (fp), 'x');
+      TEST_COMPARE (fgetc (fp), 'x');
+      TEST_COMPARE (fgetc (fp), 'x');
+
+      /* Finally, drain out the first char we had pushed back, followed by one more char
+	 from the stream, if present.  */
+      TEST_COMPARE (fgetc (fp), 'y');
+      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>
-- 
2.46.0

Re: [PATCH v3] ungetc: Guarantee single char pushback

[Maciej W. Rozycki]

[-- Attachment #1: Type: text/plain, Size: 1135 bytes --]

On Fri, 6 Dec 2024, Siddhesh Poyarekar wrote:

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

 So what would the problem be if we instead replaced:

  int _flags2;

with

  short int _flags2;
  char _short_backupbuf[1];
  char _unused;

or if we wanted to be super cautious and used a union to prevent any issue 
with alignment with some obscure psABI (which I doubt is needed given that 
the preceding member is of the int type), then:

  union
    {
      int _overlay;
      struct
	{
	  short int _flags2;
	  char _short_backupbuf[1];
	  char _unused;
	};
    };

?  There's no need to change existing code that uses _flags2 and there's 
no need to penalise legacy calls.  I guess saving _unused2 space is hardly 
an argument, though I note that placing a char[1] member first will ask 
for rearrangement to avoid wasting space for padding once a wider member 
follows in the future.

 I've run native `powerpc64le-linux-gnu' regression testing and saw no 
issues with either patch attached applied on top of your change.

  Maciej

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=glibc-struct-file-flags2-short-backupbuf.diff, Size: 2671 bytes --]

---
 libio/bits/types/struct_FILE.h |    7 ++++---
 libio/libioP.h                 |    8 ++++----
 2 files changed, 8 insertions(+), 7 deletions(-)

Index: glibc/libio/bits/types/struct_FILE.h
===================================================================
--- glibc.orig/libio/bits/types/struct_FILE.h
+++ glibc/libio/bits/types/struct_FILE.h
@@ -71,7 +71,9 @@ struct _IO_FILE
   struct _IO_FILE *_chain;
 
   int _fileno;
-  int _flags2;
+  short int _flags2;
+  char _short_backupbuf[1];
+  char _unused;
   __off_t _old_offset; /* This used to be _offset but it's too small.  */
 
   /* 1+column number of pbase(); 0 is unknown. */
@@ -95,9 +97,8 @@ struct _IO_FILE_complete
   void *_freeres_buf;
   struct _IO_FILE **_prevchain;
   int _mode;
-  char _short_backupbuf[1];
   /* Make sure we don't get into trouble again.  */
-  char _unused2[15 * sizeof (int) - 5 * sizeof (void *) - sizeof (char)];
+  char _unused2[15 * sizeof (int) - 5 * sizeof (void *)];
 };
 
 /* These macros are used by bits/stdio.h and internal headers.  */
Index: glibc/libio/libioP.h
===================================================================
--- glibc.orig/libio/libioP.h
+++ glibc/libio/libioP.h
@@ -913,12 +913,12 @@ extern int _IO_vscanf (const char *, va_
 # ifdef _IO_USE_OLD_IO_FILE
 #  define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \
        { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \
-	 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, \
+	 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, 0, { 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, \
-	 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, \
+	 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, 0, { 0 }, \
 	 0, _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock, _IO_pos_BAD,\
 	 NULL, WDP, 0 }
 # endif
@@ -926,12 +926,12 @@ extern int _IO_vscanf (const char *, va_
 # ifdef _IO_USE_OLD_IO_FILE
 #  define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \
        { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \
-	 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, \
+	 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, 0, { 0 }, \
 	 0, _IO_pos_BAD }
 # else
 #  define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \
        { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \
-	 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, \
+	 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, 0, { 0 }, \
 	 0, _IO_pos_BAD, 0, 0, { 0 }, 0, _IO_pos_BAD, \
 	 NULL, WDP, 0 }
 # endif

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: Type: text/x-diff; name=glibc-struct-file-flags2-short-backupbuf-union.diff, Size: 2902 bytes --]

---
 libio/bits/types/struct_FILE.h |   14 +++++++++++---
 libio/libioP.h                 |   16 ++++++++--------
 2 files changed, 19 insertions(+), 11 deletions(-)

Index: glibc/libio/bits/types/struct_FILE.h
===================================================================
--- glibc.orig/libio/bits/types/struct_FILE.h
+++ glibc/libio/bits/types/struct_FILE.h
@@ -71,7 +71,16 @@ struct _IO_FILE
   struct _IO_FILE *_chain;
 
   int _fileno;
-  int _flags2;
+  union
+    {
+      int _overlay;
+      struct
+	{
+	  short int _flags2;
+	  char _short_backupbuf[1];
+	  char _unused;
+	};
+    };
   __off_t _old_offset; /* This used to be _offset but it's too small.  */
 
   /* 1+column number of pbase(); 0 is unknown. */
@@ -95,9 +104,8 @@ struct _IO_FILE_complete
   void *_freeres_buf;
   struct _IO_FILE **_prevchain;
   int _mode;
-  char _short_backupbuf[1];
   /* Make sure we don't get into trouble again.  */
-  char _unused2[15 * sizeof (int) - 5 * sizeof (void *) - sizeof (char)];
+  char _unused2[15 * sizeof (int) - 5 * sizeof (void *)];
 };
 
 /* These macros are used by bits/stdio.h and internal headers.  */
Index: glibc/libio/libioP.h
===================================================================
--- glibc.orig/libio/libioP.h
+++ glibc/libio/libioP.h
@@ -913,26 +913,26 @@ extern int _IO_vscanf (const char *, va_
 # ifdef _IO_USE_OLD_IO_FILE
 #  define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \
        { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \
-	 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, \
-	 0, _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock }
+	 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, { 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, \
-	 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, \
-	 0, _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock, _IO_pos_BAD,\
+	 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, { 0 }, \
+	 _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock, _IO_pos_BAD,\
 	 NULL, WDP, 0 }
 # endif
 #else
 # ifdef _IO_USE_OLD_IO_FILE
 #  define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \
        { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \
-	 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, \
-	 0, _IO_pos_BAD }
+	 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, { 0 }, \
+	 _IO_pos_BAD }
 # else
 #  define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \
        { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \
-	 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, \
-	 0, _IO_pos_BAD, 0, 0, { 0 }, 0, _IO_pos_BAD, \
+	 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, { 0 }, \
+	 _IO_pos_BAD, 0, 0, { 0 }, 0, _IO_pos_BAD, \
 	 NULL, WDP, 0 }
 # endif
 #endif

Re: [PATCH v3] ungetc: Guarantee single char pushback

[Siddhesh Poyarekar]

On 2024-12-08 20:40, Maciej W. Rozycki wrote:
> On Fri, 6 Dec 2024, Siddhesh Poyarekar wrote:
> 
>> The C standard requires that ungetc guarantees at least one pushback, so
>> put a single byte pushback buffer in the FILE struct to enable that.
> 
>   So what would the problem be if we instead replaced:
> 
>    int _flags2;
> 
> with
> 
>    short int _flags2;
>    char _short_backupbuf[1];
>    char _unused;

I don't see an actual problem other than that of an application maybe 
using the upper bits of _flags2 for their own logic.  One could make the 
same argument for unused2, but the defence there is that there's 
precedent of struct expansion into unused2; the same can't be said about 
_flags2.

I personally don't care to retain such abuses, so it seems OK to shrink 
_flags2.  Florian, what do you think?

Thanks,
Sid

Re: [PATCH v3] ungetc: Guarantee single char pushback

[Florian Weimer]

* Siddhesh Poyarekar:

> On 2024-12-08 20:40, Maciej W. Rozycki wrote:
>> On Fri, 6 Dec 2024, Siddhesh Poyarekar wrote:
>> 
>>> The C standard requires that ungetc guarantees at least one pushback, so
>>> put a single byte pushback buffer in the FILE struct to enable that.
>>   So what would the problem be if we instead replaced:
>>    int _flags2;
>> with
>>    short int _flags2;
>>    char _short_backupbuf[1];
>>    char _unused;
>
> I don't see an actual problem other than that of an application maybe
> using the upper bits of _flags2 for their own logic.  One could make
> the same argument for unused2, but the defence there is that there's
> precedent of struct expansion into unused2; the same can't be said
> about _flags2.
>
> I personally don't care to retain such abuses, so it seems OK to
> shrink _flags2.  Florian, what do you think?

From my perspective, either way is fine.

Thanks,
Florian

Re: [PATCH v3] ungetc: Guarantee single char pushback

[Andreas Schwab]

On Dez 09 2024, Siddhesh Poyarekar wrote:

> On 2024-12-08 20:40, Maciej W. Rozycki wrote:
>> On Fri, 6 Dec 2024, Siddhesh Poyarekar wrote:
>> 
>>> The C standard requires that ungetc guarantees at least one pushback, so
>>> put a single byte pushback buffer in the FILE struct to enable that.
>>   So what would the problem be if we instead replaced:
>>    int _flags2;
>> with
>>    short int _flags2;
>>    char _short_backupbuf[1];
>>    char _unused;
>
> I don't see an actual problem other than that of an application maybe
> using the upper bits of _flags2 for their own logic.  One could make the
> same argument for unused2, but the defence there is that there's precedent
> of struct expansion into unused2; the same can't be said about _flags2.

There is also the question of the probability that we eventually need
more than 16 flags here.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH v3] ungetc: Guarantee single char pushback

[Siddhesh Poyarekar]

On 2024-12-09 08:14, Andreas Schwab wrote:
> On Dez 09 2024, Siddhesh Poyarekar wrote:
> 
>> On 2024-12-08 20:40, Maciej W. Rozycki wrote:
>>> On Fri, 6 Dec 2024, Siddhesh Poyarekar wrote:
>>>
>>>> The C standard requires that ungetc guarantees at least one pushback, so
>>>> put a single byte pushback buffer in the FILE struct to enable that.
>>>    So what would the problem be if we instead replaced:
>>>     int _flags2;
>>> with
>>>     short int _flags2;
>>>     char _short_backupbuf[1];
>>>     char _unused;
>>
>> I don't see an actual problem other than that of an application maybe
>> using the upper bits of _flags2 for their own logic.  One could make the
>> same argument for unused2, but the defence there is that there's precedent
>> of struct expansion into unused2; the same can't be said about _flags2.
> 
> There is also the question of the probability that we eventually need
> more than 16 flags here.

Maciej's argument (in a previous thread) was that the probability of 
that is low enough that we need not care.  Do you disagree?

Sid

[PATCH v4] ungetc: Guarantee single char pushback

[Siddhesh Poyarekar]

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

Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
---
Changes from v3:
- Shrunk _flags2 and moved _short_backupbuf to the old FILE struct.

Changes from v2:
- Fixed nits
- Used _IO_free_backup_buf in oldfileops and wfileops as well.
- Enhanced test to try some more cases

Changes from v1:

- Drop ungetwc from scope of the patchset
- Fixed nits
- Retain old behaviour for legacy applications
- Minimize changes to fileops
- Namespace-ize free_backup_buf
- Add a test to verify that the subsequent malloc failure results in ungetc
  failure too
- Add GNU Toolchain Authors copyright notice.

 libio/bits/types/struct_FILE.h  |   4 +-
 libio/fileops.c                 |   7 +-
 libio/genops.c                  |  23 +++++--
 libio/libioP.h                  |  13 ++--
 libio/oldfileops.c              |   5 +-
 libio/wfileops.c                |   3 +-
 stdio-common/Makefile           |   2 +
 stdio-common/tst-ungetc-nomem.c | 115 ++++++++++++++++++++++++++++++++
 8 files changed, 154 insertions(+), 18 deletions(-)
 create mode 100644 stdio-common/tst-ungetc-nomem.c

diff --git a/libio/bits/types/struct_FILE.h b/libio/bits/types/struct_FILE.h
index d8d26639d1..a5e0679de3 100644
--- 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,8 @@ struct _IO_FILE
   struct _IO_FILE *_chain;
 
   int _fileno;
-  int _flags2;
+  int _flags2:24;
+  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 759d737ec7..d49e489f55 100644
--- 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 d7e35e67d5..dddd420ee2 100644
--- 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
@@ -48,6 +49,13 @@ flush_cleanup (void *not_used)
 }
 #endif
 
+void
+_IO_free_backup_buf (FILE *fp, char *ptr)
+{
+  if (ptr != fp->_short_backupbuf)
+    free (ptr);
+}
+
 /* Fields in struct _IO_FILE after the _lock field are internal to
    glibc and opaque to applications.  We can change them as long as
    the size of struct _IO_FILE is unchanged, which is checked as the
@@ -212,7 +220,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 +268,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 +644,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 +1006,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 +1033,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 34bf91fcd8..287caf8664 100644
--- 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
@@ -357,6 +358,8 @@ typedef FILE *_IO_ITER;
 
 /* Generic functions */
 
+extern void _IO_free_backup_buf (FILE *, char *);
+libc_hidden_proto (_IO_free_backup_buf)
 extern void _IO_switch_to_main_get_area (FILE *) __THROW;
 extern void _IO_switch_to_backup_area (FILE *) __THROW;
 extern int _IO_switch_to_get_mode (FILE *);
@@ -911,13 +914,13 @@ extern int _IO_vscanf (const char *, va_list) __THROW;
 #  define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \
        { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \
 	 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, \
-	 0, _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock }
+	 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, NULL, (FILE *) CHAIN, FD, 0, { 0 }, \
+	 _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock, _IO_pos_BAD, \
 	 NULL, WDP, NULL }
 # endif
 #else
@@ -925,12 +928,12 @@ extern int _IO_vscanf (const char *, va_list) __THROW;
 #  define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \
        { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \
 	 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, \
-	 0, _IO_pos_BAD }
+	 0, { 0 }, _IO_pos_BAD }
 # else
 #  define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \
        { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \
 	 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, \
-	 0, _IO_pos_BAD, 0, 0, { 0 }, 0, _IO_pos_BAD, \
+	 0, { 0 }, _IO_pos_BAD, 0, 0, { 0 }, 0, _IO_pos_BAD, \
 	 NULL, WDP, 0 }
 # endif
 #endif
diff --git a/libio/oldfileops.c b/libio/oldfileops.c
index 8f775c9094..03f4d76a57 100644
--- 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 16beab1f3a..a96bfa589b 100644
--- 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 e76e40e587..b1a04fd064 100644
--- 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
index 0000000000..49db33fff3
--- /dev/null
+++ b/stdio-common/tst-ungetc-nomem.c
@@ -0,0 +1,115 @@
+/* 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 <stdio.h>
+#include <string.h>
+#include <support/check.h>
+#include <support/support.h>
+#include <support/temp_file.h>
+#include <support/xstdio.h>
+
+extern void *__libc_malloc (size_t)
+     __attribute__ ((malloc)) __attribute__ ((alloc_size (1)));
+
+static volatile 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-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.  */
+  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 ('y', fp), 'y');
+
+      /* 2: This will result in resizing, which should fail.  */
+      TEST_COMPARE (ungetc ('w', 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 ('x', fp), 'x');
+
+      /* 4: And fail again because this again forces an alloc, which fails.  */
+      fail = true;
+      TEST_COMPARE (ungetc ('x', 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 ('x', fp), 'x');
+      fail = true;
+      TEST_COMPARE (ungetc ('x', fp), 'x');
+
+      /* Drain out the x's.  */
+      TEST_COMPARE (fgetc (fp), 'x');
+      TEST_COMPARE (fgetc (fp), 'x');
+      TEST_COMPARE (fgetc (fp), 'x');
+
+      /* Finally, drain out the first char we had pushed back, followed by one more char
+	 from the stream, if present.  */
+      TEST_COMPARE (fgetc (fp), 'y');
+      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>
-- 
2.47.1

Re: [PATCH v3] ungetc: Guarantee single char pushback

[Maciej W. Rozycki]

On Mon, 9 Dec 2024, Siddhesh Poyarekar wrote:

> > > >    So what would the problem be if we instead replaced:
> > > >     int _flags2;
> > > > with
> > > >     short int _flags2;
> > > >     char _short_backupbuf[1];
> > > >     char _unused;
> > > 
> > > I don't see an actual problem other than that of an application maybe
> > > using the upper bits of _flags2 for their own logic.  One could make the

 I don't thing an app is allowed to poke at FILE in the first place, it's 
supposed to be an opaque data type.  We have a note at the top to this 
effect (in the first sentence):

/* Caution: The contents of this file are not part of the official
   stdio.h API.  However, much of it is part of the official *binary*
   interface, and therefore cannot be changed.  */

It's not clear to me what "much of it" refers to in the second, except 
(obviously) for the size of the structure.

> > > same argument for unused2, but the defence there is that there's precedent
> > > of struct expansion into unused2; the same can't be said about _flags2.
> > 
> > There is also the question of the probability that we eventually need
> > more than 16 flags here.
> 
> Maciej's argument (in a previous thread) was that the probability of that is
> low enough that we need not care.  Do you disagree?

 Also I don't think the flags need to be contiguous.  It's not clear to me 
even why we're using plain integers and explicit masks in the first place 
rather than bit-fields, which I think would make code just a little bit 
cleaner.

 My only concern would be if we ever needed to consume a FILE data object 
produced by a different version of glibc, in which case we'd have to vary 
the new member ordering by the endianness.  I don't think we need, though; 
the only case I can think of would be pulling a different version of libc 
via dlopen from a static binary and passing a FILE reference to it, but we 
don't support such a scenario AFAIK.

 Have I missed anything here?

  Maciej

Re: [PATCH v3] ungetc: Guarantee single char pushback

[Andreas Schwab]

On Dez 10 2024, Maciej W. Rozycki wrote:

>  I don't thing an app is allowed to poke at FILE in the first place, it's 
> supposed to be an opaque data type.

Right, except through the macros provided by <stdio.h>, the only reason
why the details are visible.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH v4] ungetc: Guarantee single char pushback

[Richard Henderson]

On 12/10/24 06:30, Siddhesh Poyarekar wrote:
> -  int _flags2;
> +  int _flags2:24;
> +  char _short_backupbuf[1];

This doesn't create a 3-byte flags2, if that's what you're trying.
The underlying storage will still be int-sized, now with 8 unallocated bits.


r~

Re: [PATCH v3] ungetc: Guarantee single char pushback

[Maciej W. Rozycki]

On Tue, 10 Dec 2024, Andreas Schwab wrote:

> >  I don't thing an app is allowed to poke at FILE in the first place, it's 
> > supposed to be an opaque data type.
> 
> Right, except through the macros provided by <stdio.h>, the only reason
> why the details are visible.

 Indeed, so these parts have become a part of the ABI.  This has fixed 
`_IO_read_ptr', `_IO_read_end', `_IO_write_ptr', `_IO_write_end', and 
`_flags' AFAICT, but not `_flags2' or anything else.

  Maciej

Re: [PATCH v4] ungetc: Guarantee single char pushback

[Siddhesh Poyarekar]

On 2024-12-10 09:03, Richard Henderson wrote:
> On 12/10/24 06:30, Siddhesh Poyarekar wrote:
>> -  int _flags2;
>> +  int _flags2:24;
>> +  char _short_backupbuf[1];
> 
> This doesn't create a 3-byte flags2, if that's what you're trying.
> The underlying storage will still be int-sized, now with 8 unallocated 
> bits.

gcc and clang both appear to pack in the struct just fine, which 
effectively gives flags2 3-bytes storage and _short_backupbuf right next 
to it.  Maybe I've misunderstood the issue you're trying to point out, 
could you please elaborate?

Thanks,
Sid

Re: [PATCH v4] ungetc: Guarantee single char pushback

[Maciej W. Rozycki]

On Wed, 11 Dec 2024, Siddhesh Poyarekar wrote:

> > > -  int _flags2;
> > > +  int _flags2:24;
> > > +  char _short_backupbuf[1];
> > 
> > This doesn't create a 3-byte flags2, if that's what you're trying.
> > The underlying storage will still be int-sized, now with 8 unallocated bits.
> 
> gcc and clang both appear to pack in the struct just fine, which effectively
> gives flags2 3-bytes storage and _short_backupbuf right next to it.  Maybe
> I've misunderstood the issue you're trying to point out, could you please
> elaborate?

 Indeed a bit-field does get packed with an eligible adjacent non-bitfield 
member of the structure, although ISO C defers storage unit allocation to 
the implementation and therefore I do believe it is down to the individual 
platform-specific ABI.

 Then e.g. the o32 MIPS ABI has this clause[1]:

"* Bit-fields can share a storage unit with other struct/union members,
   including members that are not bit-fields.  Of course, struct members
   occupy different parts of the storage unit."

and the updated structure is laid out accordingly:

struct _IO_FILE {
	int                        _flags;               /*     0     4 */
	char *                     _IO_read_ptr;         /*     4     4 */
	char *                     _IO_read_end;         /*     8     4 */
	char *                     _IO_read_base;        /*    12     4 */
	char *                     _IO_write_base;       /*    16     4 */
	char *                     _IO_write_ptr;        /*    20     4 */
	char *                     _IO_write_end;        /*    24     4 */
	char *                     _IO_buf_base;         /*    28     4 */
	char *                     _IO_buf_end;          /*    32     4 */
	char *                     _IO_save_base;        /*    36     4 */
	char *                     _IO_backup_base;      /*    40     4 */
	char *                     _IO_save_end;         /*    44     4 */
	struct _IO_marker *        _markers;             /*    48     4 */
	struct _IO_FILE *          _chain;               /*    52     4 */
	int                        _fileno;              /*    56     4 */

	/* XXX 3 bytes hole, try to pack */
	/* Bitfield combined with previous fields */

	static int                        _flags2        /*     0: 0  0 */
	char                       _short_backupbuf[1];  /*    63     1 */
	__off_t                    _old_offset;          /*    64     4 */
	short unsigned int         _cur_column;          /*    68     2 */
	signed char                _vtable_offset;       /*    70     1 */
	char                       _shortbuf[1];         /*    71     1 */
	_IO_lock_t *               _lock;                /*    72     4 */

	/* size: 76, cachelines: 1, members: 21, static members: 1 */
	/* sum members: 85, holes: 1, sum holes: 3 */
	/* last cacheline: 76 bytes */

	/* BRAIN FART ALERT! 76 != 85 + 3(holes), diff = -12 */

};

(although the tool does seem confused a little here).

 AFAICT from `place_field' in gcc/stor-layout.cc it is the same across all 
the !TARGET_MS_BITFIELD_LAYOUT_P ABIs GCC supports, there's no provision 
for a per-target variation.

  Maciej

Re: [PATCH v4] ungetc: Guarantee single char pushback

[Siddhesh Poyarekar]

On 2024-12-12 07:16, Maciej W. Rozycki wrote:
> On Wed, 11 Dec 2024, Siddhesh Poyarekar wrote:
> 
>>>> -  int _flags2;
>>>> +  int _flags2:24;
>>>> +  char _short_backupbuf[1];
>>>
>>> This doesn't create a 3-byte flags2, if that's what you're trying.
>>> The underlying storage will still be int-sized, now with 8 unallocated bits.
>>
>> gcc and clang both appear to pack in the struct just fine, which effectively
>> gives flags2 3-bytes storage and _short_backupbuf right next to it.  Maybe
>> I've misunderstood the issue you're trying to point out, could you please
>> elaborate?
> 
>   Indeed a bit-field does get packed with an eligible adjacent non-bitfield
> member of the structure, although ISO C defers storage unit allocation to
> the implementation and therefore I do believe it is down to the individual
> platform-specific ABI.
> 
>   Then e.g. the o32 MIPS ABI has this clause[1]:
> 
> "* Bit-fields can share a storage unit with other struct/union members,
>     including members that are not bit-fields.  Of course, struct members
>     occupy different parts of the storage unit."

That sounds like it's saying that distinct members cannot share the 
smallest addressable unit, i.e. a byte, which is fine.

> and the updated structure is laid out accordingly:
> 
> struct _IO_FILE {
> 	int                        _flags;               /*     0     4 */
> 	char *                     _IO_read_ptr;         /*     4     4 */
> 	char *                     _IO_read_end;         /*     8     4 */
> 	char *                     _IO_read_base;        /*    12     4 */
> 	char *                     _IO_write_base;       /*    16     4 */
> 	char *                     _IO_write_ptr;        /*    20     4 */
> 	char *                     _IO_write_end;        /*    24     4 */
> 	char *                     _IO_buf_base;         /*    28     4 */
> 	char *                     _IO_buf_end;          /*    32     4 */
> 	char *                     _IO_save_base;        /*    36     4 */
> 	char *                     _IO_backup_base;      /*    40     4 */
> 	char *                     _IO_save_end;         /*    44     4 */
> 	struct _IO_marker *        _markers;             /*    48     4 */
> 	struct _IO_FILE *          _chain;               /*    52     4 */
> 	int                        _fileno;              /*    56     4 */
> 
> 	/* XXX 3 bytes hole, try to pack */
> 	/* Bitfield combined with previous fields */
> 
> 	static int                        _flags2        /*     0: 0  0 */

Is the "static" a typo that may have confused the tool?

Sid

Re: [PATCH v4] ungetc: Guarantee single char pushback

[Maciej W. Rozycki]

On Thu, 12 Dec 2024, Siddhesh Poyarekar wrote:

> > > gcc and clang both appear to pack in the struct just fine, which
> > > effectively
> > > gives flags2 3-bytes storage and _short_backupbuf right next to it.  Maybe
> > > I've misunderstood the issue you're trying to point out, could you please
> > > elaborate?
> > 
> >   Indeed a bit-field does get packed with an eligible adjacent non-bitfield
> > member of the structure, although ISO C defers storage unit allocation to
> > the implementation and therefore I do believe it is down to the individual
> > platform-specific ABI.
> > 
> >   Then e.g. the o32 MIPS ABI has this clause[1]:
> > 
> > "* Bit-fields can share a storage unit with other struct/union members,
> >     including members that are not bit-fields.  Of course, struct members
> >     occupy different parts of the storage unit."
> 
> That sounds like it's saying that distinct members cannot share the smallest
> addressable unit, i.e. a byte, which is fine.

 Obviously struct members cannot overlap, but more importantly it does say 
non-bitfield members can share a storage unit with bit-fields, e.g. one 
used for the "int" type, just as bit-field members do between themselves.

 NB below there is the document reference I forgot to add.

> > and the updated structure is laid out accordingly:
> > 
> > struct _IO_FILE {
> > 	int                        _flags;               /*     0     4 */
> > 	char *                     _IO_read_ptr;         /*     4     4 */
> > 	char *                     _IO_read_end;         /*     8     4 */
> > 	char *                     _IO_read_base;        /*    12     4 */
> > 	char *                     _IO_write_base;       /*    16     4 */
> > 	char *                     _IO_write_ptr;        /*    20     4 */
> > 	char *                     _IO_write_end;        /*    24     4 */
> > 	char *                     _IO_buf_base;         /*    28     4 */
> > 	char *                     _IO_buf_end;          /*    32     4 */
> > 	char *                     _IO_save_base;        /*    36     4 */
> > 	char *                     _IO_backup_base;      /*    40     4 */
> > 	char *                     _IO_save_end;         /*    44     4 */
> > 	struct _IO_marker *        _markers;             /*    48     4 */
> > 	struct _IO_FILE *          _chain;               /*    52     4 */
> > 	int                        _fileno;              /*    56     4 */
> > 
> > 	/* XXX 3 bytes hole, try to pack */
> > 	/* Bitfield combined with previous fields */
> > 
> > 	static int                        _flags2        /*     0: 0  0 */
> 
> Is the "static" a typo that may have confused the tool?

 This is all output from the tool used over a glibc object compiled with 
your patch applied and it's not clear to me why the took has added the 
"static" keyword to the declaration.  It seems related to the offset/width 
reported on the right hand side all being zero.

 The 64-bit POWER/LE variant is similar except for another hole reported 
at the top and also further down, but then for the Alpha there's no hole 
reported here, the offset/width are all nonzero and there's no "static" 
keyword:

struct _IO_FILE {
	int                        _flags;               /*     0     4 */

	/* XXX 4 bytes hole, try to pack */

	char *                     _IO_read_ptr;         /*     8     8 */
	char *                     _IO_read_end;         /*    16     8 */
	char *                     _IO_read_base;        /*    24     8 */
	char *                     _IO_write_base;       /*    32     8 */
	char *                     _IO_write_ptr;        /*    40     8 */
	char *                     _IO_write_end;        /*    48     8 */
	char *                     _IO_buf_base;         /*    56     8 */
	char *                     _IO_buf_end;          /*    64     8 */
	char *                     _IO_save_base;        /*    72     8 */
	char *                     _IO_backup_base;      /*    80     8 */
	char *                     _IO_save_end;         /*    88     8 */
	struct _IO_marker *        _markers;             /*    96     8 */
	struct _IO_FILE *          _chain;               /*   104     8 */
	int                        _fileno;              /*   112     4 */
	int                        _flags2:24;           /*   116: 8  4 */

	/* Bitfield combined with next fields */

	char                       _short_backupbuf[1];  /*   119     1 */
	__off_t                    _old_offset;          /*   120     8 */
	/* --- cacheline 1 boundary (128 bytes) --- */
	short unsigned int         _cur_column;          /*   128     2 */
	signed char                _vtable_offset;       /*   130     1 */
	char                       _shortbuf[1];         /*   131     1 */

	/* XXX 4 bytes hole, try to pack */

	_IO_lock_t *               _lock;                /*   136     8 */

	/* size: 144, cachelines: 2, members: 22 */
	/* sum members: 136, holes: 2, sum holes: 8 */
	/* last cacheline: 16 bytes */
};

 I take it it comes from slight variations between DWARF information 
produced -- I have DWARFv2 forced for my Alpha configuration, to satisfy 
ancient native GDB I have installed on the target machine (there's no 
`gdbserver' port available yet for Alpha/Linux, so I cannot use a modern 
cross-debugger instead).  I suspect it is a bug/limitation in the tool, 
which seems a bit picky as well and refuses to produce output for some 
objects.

References:

[1] "SYSTEM V APPLICATION BINARY INTERFACE, MIPS RISC Processor
    Supplement, 3rd Edition", Section "Bit-Fields", p. 3-8

  Maciej

Re: [PATCH v4] ungetc: Guarantee single char pushback

[Siddhesh Poyarekar]

On 2024-12-12 08:00, Maciej W. Rozycki wrote:
> On Thu, 12 Dec 2024, Siddhesh Poyarekar wrote:
> 
>>>> gcc and clang both appear to pack in the struct just fine, which
>>>> effectively
>>>> gives flags2 3-bytes storage and _short_backupbuf right next to it.  Maybe
>>>> I've misunderstood the issue you're trying to point out, could you please
>>>> elaborate?
>>>
>>>    Indeed a bit-field does get packed with an eligible adjacent non-bitfield
>>> member of the structure, although ISO C defers storage unit allocation to
>>> the implementation and therefore I do believe it is down to the individual
>>> platform-specific ABI.
>>>
>>>    Then e.g. the o32 MIPS ABI has this clause[1]:
>>>
>>> "* Bit-fields can share a storage unit with other struct/union members,
>>>      including members that are not bit-fields.  Of course, struct members
>>>      occupy different parts of the storage unit."
>>
>> That sounds like it's saying that distinct members cannot share the smallest
>> addressable unit, i.e. a byte, which is fine.
> 
>   Obviously struct members cannot overlap, but more importantly it does say
> non-bitfield members can share a storage unit with bit-fields, e.g. one
> used for the "int" type, just as bit-field members do between themselves.

Right, which is why I reckon the layout I'm proposing is OK; would you 
agree?

Thanks,
Sid

Re: [PATCH v4] ungetc: Guarantee single char pushback

[Maciej W. Rozycki]

On Thu, 12 Dec 2024, Siddhesh Poyarekar wrote:

> > > > "* Bit-fields can share a storage unit with other struct/union members,
> > > >      including members that are not bit-fields.  Of course, struct
> > > > members
> > > >      occupy different parts of the storage unit."
> > > 
> > > That sounds like it's saying that distinct members cannot share the
> > > smallest
> > > addressable unit, i.e. a byte, which is fine.
> > 
> >   Obviously struct members cannot overlap, but more importantly it does say
> > non-bitfield members can share a storage unit with bit-fields, e.g. one
> > used for the "int" type, just as bit-field members do between themselves.
> 
> Right, which is why I reckon the layout I'm proposing is OK; would you agree?

 I do and actually I like it; I'll go through your patch in detail soon.

  Maciej

Re: [PATCH v4] ungetc: Guarantee single char pushback

[Maciej W. Rozycki]

Hi Sid,

 Thanks for your effort, this is looking mostly good to me.

 My main concern is `_IO_free_backup_buf', which I think will perform 
better as a static inline function.  I have some questions as well as to 
the test functions including a request to add introductory comments for 
them with the answers.  Plus a couple of small nits, as all detailed 
below.

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

 Please mention here why this single byte pushback buffer is needed to 
fulfil the C standard's requirement (i.e. that we fail to fulfil it now 
because we use `malloc', which can fail).

> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>

 Hmm, please clarify your copyright status.

> diff --git a/libio/bits/types/struct_FILE.h b/libio/bits/types/struct_FILE.h
> index d8d26639d1..a5e0679de3 100644
> --- 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.

 OK.  Consistent with DCO (but see above).

> @@ -70,7 +71,8 @@ struct _IO_FILE
>    struct _IO_FILE *_chain;
>  
>    int _fileno;
> -  int _flags2;
> +  int _flags2:24;
> +  char _short_backupbuf[1];

 OK.  Taking advantage of a char member sharing the storage unit with the 
preceding bit-field, so there's no change in the size of the structure or 
member offsets.  Please add a short description of the new member, just as 
with most of the existing ones (all should have one IMO).

> diff --git a/libio/fileops.c b/libio/fileops.c
> index 759d737ec7..d49e489f55 100644
> --- 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.

 OK.  Consistent with DCO (but see above).

> @@ -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);

 OK.  Mechanical update.

> @@ -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);

 OK.  Mechanical update.

> @@ -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);

 OK.  Mechanical update.

> diff --git a/libio/genops.c b/libio/genops.c
> index d7e35e67d5..dddd420ee2 100644
> --- 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.

 OK.  Consistent with DCO (but see above).

> @@ -48,6 +49,13 @@ flush_cleanup (void *not_used)
>  }
>  #endif
>  
> +void
> +_IO_free_backup_buf (FILE *fp, char *ptr)
> +{
> +  if (ptr != fp->_short_backupbuf)
> +    free (ptr);
> +}
> +

 OK, this replaces explicit calls to `free', taking care of the special 
case of the backup buffer.

 But is there a need for this to be an external function?

 ISTM there could be a performance benefit from making it static inline: 
an arrangement for making calls here is likely not to be cheaper in terms 
of instruction size/count or execution time than making the comparison and 
branching around `free', even for simplistic predictors that predict all 
forward branches untaken.

 From the look of the code I infer we normally expect `ptr' not to point 
at the backup buffer as that will only happen in the case of a `malloc' 
failure, so firstly such a forward branch usually won't indeed be taken, 
making it virtually free for the fall-through case, and secondly please 
annotate the expression with `__glibc_unlikely' accordingly.

> @@ -212,7 +220,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);

 OK.  Mechanical update.

> @@ -260,7 +268,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);

 OK.  Mechanical update.

> @@ -636,7 +644,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);

 OK.  Mechanical update.

> @@ -998,11 +1006,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;
> +		}

 OK.  In the unlikely case of a `malloc' failure we'll resort to the 
single-character backup buffer, avoiding an unsuccessful return.

 Thanks for discarding a comment that's no longer relevant.  From 
observation such bits are too easily missed.

> @@ -1022,7 +1033,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);

 OK.  Mechanical update.

> diff --git a/libio/libioP.h b/libio/libioP.h
> index 34bf91fcd8..287caf8664 100644
> --- 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.

 OK.  Consistent with DCO (but see above).

> @@ -357,6 +358,8 @@ typedef FILE *_IO_ITER;
>  
>  /* Generic functions */
>  
> +extern void _IO_free_backup_buf (FILE *, char *);
> +libc_hidden_proto (_IO_free_backup_buf)

 OK.  But this won't be needed with a static inline function.

> @@ -911,13 +914,13 @@ extern int _IO_vscanf (const char *, va_list) __THROW;
>  #  define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \
>         { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \
>  	 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, \
> -	 0, _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock }
> +	 0, { 0 }, _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock }

 OK.  New member initialised.

>  # 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, NULL, (FILE *) CHAIN, FD, 0, { 0 }, \
> +	 _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock, _IO_pos_BAD, \

 OK.  New member initialised.

 I think it will make sense to keep the line breaks between the same 
members across all the four FILEBUF_LITERAL definitions so as to make it 
easier to people to match the variants against each other.

 Please coordinate with Alejandro Colomar (CC'd) on cleaning up these 
definitions, which went out of sync; cf. 
<https://inbox.sourceware.org/libc-alpha/042e25d3-1b02-c448-1f8c-84f52549f5b5@redhat.com/>.

> @@ -925,12 +928,12 @@ extern int _IO_vscanf (const char *, va_list) __THROW;
>  #  define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \
>         { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \
>  	 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, \
> -	 0, _IO_pos_BAD }
> +	 0, { 0 }, _IO_pos_BAD }

 OK.  New member initialised.

>  # else
>  #  define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \
>         { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \
>  	 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, \
> -	 0, _IO_pos_BAD, 0, 0, { 0 }, 0, _IO_pos_BAD, \
> +	 0, { 0 }, _IO_pos_BAD, 0, 0, { 0 }, 0, _IO_pos_BAD, \

 OK.  New member initialised.

> diff --git a/libio/oldfileops.c b/libio/oldfileops.c
> index 8f775c9094..03f4d76a57 100644
> --- 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.

 OK.  Consistent with DCO (but see above).

> @@ -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);

 OK.  Mechanical update.

> @@ -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);

 OK.  Mechanical update.

> diff --git a/libio/wfileops.c b/libio/wfileops.c
> index 16beab1f3a..a96bfa589b 100644
> --- 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.

 OK.  Consistent with DCO (but see above).

> @@ -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);

 OK.  Mechanical update.

> diff --git a/stdio-common/Makefile b/stdio-common/Makefile
> index e76e40e587..b1a04fd064 100644
> --- 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.

 OK.  Consistent with DCO (but see above).

> @@ -303,6 +304,7 @@ tests := \
>    tst-tmpnam \
>    tst-ungetc \
>    tst-ungetc-leak \
> +  tst-ungetc-nomem \

 OK.  New test added.

> diff --git a/stdio-common/tst-ungetc-nomem.c b/stdio-common/tst-ungetc-nomem.c
> new file mode 100644
> index 0000000000..49db33fff3
> --- /dev/null
> +++ b/stdio-common/tst-ungetc-nomem.c
> @@ -0,0 +1,115 @@
> +/* 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 <stdio.h>
> +#include <string.h>
> +#include <support/check.h>
> +#include <support/support.h>
> +#include <support/temp_file.h>
> +#include <support/xstdio.h>

 OK, alphabetic order.

> +
> +extern void *__libc_malloc (size_t)
> +     __attribute__ ((malloc)) __attribute__ ((alloc_size (1)));

 Please list both attributes together, also avoiding the question as to 
how much to indent here (as the prototype will fit in one line then).

> +
> +static volatile bool fail = false;

 OK, marked as `volatile' to prevent the compiler from interfering, as 
previously advised.  I note this could skip the initialiser so as to place 
it in BSS, but I'm fine with the current arrangement if you consider it 
desirable to make it explicit.

> +
> +void *
> +malloc (size_t sz)
> +{
> +  if (fail)
> +    return NULL;
> +
> +  return __libc_malloc (sz);
> +}

 OK, this interposes `malloc' so as to conditionally induce a failure and 
refers to `__libc_malloc' if the condition does not stand.  A bit hackish 
IMO, but we're in control here, so let it be.

 I think this function deserves an introductory comment, even if a single 
terse line.

> +
> +static int
> +do_test (void)
> +{
> +  char *filename = NULL;
> +  struct stat props = {};

 As nice as empty initialisers are they're a C23-ism, so please rewrite 
using older syntax.

> +  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.  */
> +  bufsz = props.st_blksize + 2;

 Why do we want the file to be the size of the libio buffer plus 2?  The 
answer seems like a good candidate for the function's introductory 
comment.

> +
> +  char *buf = xmalloc (bufsz);
> +  memset (buf, 'a', bufsz);
> +
> +  if (fwrite (buf, sizeof (char), bufsz, fp) != bufsz)
> +    FAIL_EXIT1 ("fwrite failed: %m\n");
> +  xfclose (fp);

 OK, we make a test file made up of "a" letters.

> +
> +  /* 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 ('y', fp), 'y');
> +
> +      /* 2: This will result in resizing, which should fail.  */
> +      TEST_COMPARE (ungetc ('w', 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 ('x', fp), 'x');

 This does verify new semantics, and I take it it's intentional that after 
a `malloc' failure for the initial buffer we don't go back to the minimum 
of 128 bytes for the buffer, but instead start from 2 up in a hope for a 
smaller allocation to succeed where a somewhat larger one might not.  But 
I think this new semantics should be mentioned in the change description.

> +
> +      /* 4: And fail again because this again forces an alloc, which fails.  */
> +      fail = true;
> +      TEST_COMPARE (ungetc ('x', 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 ('x', fp), 'x');
> +      fail = true;
> +      TEST_COMPARE (ungetc ('x', fp), 'x');
> +
> +      /* Drain out the x's.  */
> +      TEST_COMPARE (fgetc (fp), 'x');
> +      TEST_COMPARE (fgetc (fp), 'x');
> +      TEST_COMPARE (fgetc (fp), 'x');

 Shouldn't the `ungetc' calls use different characters each, so that we 
have an additional check that rejected characters do not come back and 
that the accepted ones come back in the correct order?

> +
> +      /* Finally, drain out the first char we had pushed back, followed by one more char
> +	 from the stream, if present.  */

 Please wrap the comment, cf:
<https://sourceware.org/glibc/wiki/Style_and_Conventions#A79-Column_Lines>.

> +      TEST_COMPARE (fgetc (fp), 'y');
> +      char c = fgetc (fp);
> +      if (!feof (fp))
> +	TEST_COMPARE (c, 'a');
> +    }

 So this loop, if successful, runs libio buffer size plus 2 times.  Please 
state in the function's introductory comment why this specific iteration 
count has been chosen.

> +
> +  /* Final sanity check before we're done.  */
> +  TEST_COMPARE (ferror (fp), 0);
> +  xfclose (fp);

 OK.  Checking for no error and closing the test file.  File removed 
automagically by test support clean-up.

> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>

 OK.

 Please resend with the updates applied (but I note further clarification 
might be needed first).

  Maciej

Re: [PATCH v4] ungetc: Guarantee single char pushback

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 1438 bytes --]

Hi Maciej,

On Mon, Dec 16, 2024 at 02:52:55AM +0000, Maciej W. Rozycki wrote:
> >  # 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, NULL, (FILE *) CHAIN, FD, 0, { 0 }, \
> > +	 _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock, _IO_pos_BAD, \
> 
>  OK.  New member initialised.
> 
>  I think it will make sense to keep the line breaks between the same 
> members across all the four FILEBUF_LITERAL definitions so as to make it 
> easier to people to match the variants against each other.
> 
>  Please coordinate with Alejandro Colomar (CC'd) on cleaning up these 
> definitions, which went out of sync; cf. 
> <https://inbox.sourceware.org/libc-alpha/042e25d3-1b02-c448-1f8c-84f52549f5b5@redhat.com/>.

Ack.  (I've also checked the other email.)

> > +
> > +static int
> > +do_test (void)
> > +{
> > +  char *filename = NULL;
> > +  struct stat props = {};
> 
>  As nice as empty initialisers are they're a C23-ism, so please rewrite 
> using older syntax.

They are also an older GNU extension, which I think is OK in .c files
(unlike in headers).  No?

Have a lovely day!
Alex

-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4] ungetc: Guarantee single char pushback

[Siddhesh Poyarekar]

On 2024-12-16 05:05, Alejandro Colomar wrote:
> Hi Maciej,
> 
> On Mon, Dec 16, 2024 at 02:52:55AM +0000, Maciej W. Rozycki wrote:
>>>   # 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, NULL, (FILE *) CHAIN, FD, 0, { 0 }, \
>>> +	 _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock, _IO_pos_BAD, \
>>
>>   OK.  New member initialised.
>>
>>   I think it will make sense to keep the line breaks between the same
>> members across all the four FILEBUF_LITERAL definitions so as to make it
>> easier to people to match the variants against each other.
>>
>>   Please coordinate with Alejandro Colomar (CC'd) on cleaning up these
>> definitions, which went out of sync; cf.
>> <https://inbox.sourceware.org/libc-alpha/042e25d3-1b02-c448-1f8c-84f52549f5b5@redhat.com/>.
> 
> Ack.  (I've also checked the other email.)

Do you want to send a patch isolated to this file, which I can rebase on 
top of and push whenever my patch is acked?

Thanks,
Sid

Re: [PATCH v4] ungetc: Guarantee single char pushback

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 1465 bytes --]

Hi Sid,

On Mon, Dec 16, 2024 at 07:38:24AM -0500, Siddhesh Poyarekar wrote:
> On 2024-12-16 05:05, Alejandro Colomar wrote:
> > Hi Maciej,
> > 
> > On Mon, Dec 16, 2024 at 02:52:55AM +0000, Maciej W. Rozycki wrote:
> > > >   # 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, NULL, (FILE *) CHAIN, FD, 0, { 0 }, \
> > > > +	 _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock, _IO_pos_BAD, \
> > > 
> > >   OK.  New member initialised.
> > > 
> > >   I think it will make sense to keep the line breaks between the same
> > > members across all the four FILEBUF_LITERAL definitions so as to make it
> > > easier to people to match the variants against each other.
> > > 
> > >   Please coordinate with Alejandro Colomar (CC'd) on cleaning up these
> > > definitions, which went out of sync; cf.
> > > <https://inbox.sourceware.org/libc-alpha/042e25d3-1b02-c448-1f8c-84f52549f5b5@redhat.com/>.
> > 
> > Ack.  (I've also checked the other email.)
> 
> Do you want to send a patch isolated to this file, which I can rebase on top
> of and push whenever my patch is acked?

Yep.

Cheers,
Alex

-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4] ungetc: Guarantee single char pushback

[Siddhesh Poyarekar]

On 2024-12-15 21:52, Maciej W. Rozycki wrote:
> Hi Sid,
> 
>   Thanks for your effort, this is looking mostly good to me.
> 
>   My main concern is `_IO_free_backup_buf', which I think will perform
> better as a static inline function.  I have some questions as well as to
> the test functions including a request to add introductory comments for
> them with the answers.  Plus a couple of small nits, as all detailed
> below.
> 
>> The C standard requires that ungetc guarantees at least one pushback, so
>> put a single byte pushback buffer in the FILE struct to enable that.
> 
>   Please mention here why this single byte pushback buffer is needed to
> fulfil the C standard's requirement (i.e. that we fail to fulfil it now
> because we use `malloc', which can fail).

Ack, I'll update the comment.

> 
>> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
> 
>   Hmm, please clarify your copyright status.

I had my personal copyright assignment to the FSF revoked in ~2019.  My 
employer (Red Hat) too has disclaimed copyright to my contributions to 
GNU projects.  As author, I am the owner of the copyright to code I 
write for GNU projects, hence the Signed-off-by.

>> diff --git a/libio/bits/types/struct_FILE.h b/libio/bits/types/struct_FILE.h
>> index d8d26639d1..a5e0679de3 100644
>> --- 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.
> 
>   OK.  Consistent with DCO (but see above).
> 
>> @@ -70,7 +71,8 @@ struct _IO_FILE
>>     struct _IO_FILE *_chain;
>>   
>>     int _fileno;
>> -  int _flags2;
>> +  int _flags2:24;
>> +  char _short_backupbuf[1];
> 
>   OK.  Taking advantage of a char member sharing the storage unit with the
> preceding bit-field, so there's no change in the size of the structure or
> member offsets.  Please add a short description of the new member, just as
> with most of the existing ones (all should have one IMO).

OK.

>> diff --git a/libio/fileops.c b/libio/fileops.c
>> index 759d737ec7..d49e489f55 100644
>> --- 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.
> 
>   OK.  Consistent with DCO (but see above).
> 
>> @@ -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);
> 
>   OK.  Mechanical update.
> 
>> @@ -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);
> 
>   OK.  Mechanical update.
> 
>> @@ -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);
> 
>   OK.  Mechanical update.
> 
>> diff --git a/libio/genops.c b/libio/genops.c
>> index d7e35e67d5..dddd420ee2 100644
>> --- 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.
> 
>   OK.  Consistent with DCO (but see above).
> 
>> @@ -48,6 +49,13 @@ flush_cleanup (void *not_used)
>>   }
>>   #endif
>>   
>> +void
>> +_IO_free_backup_buf (FILE *fp, char *ptr)
>> +{
>> +  if (ptr != fp->_short_backupbuf)
>> +    free (ptr);
>> +}
>> +
> 
>   OK, this replaces explicit calls to `free', taking care of the special
> case of the backup buffer.
> 
>   But is there a need for this to be an external function?
> 
>   ISTM there could be a performance benefit from making it static inline:
> an arrangement for making calls here is likely not to be cheaper in terms
> of instruction size/count or execution time than making the comparison and
> branching around `free', even for simplistic predictors that predict all
> forward branches untaken.
> 
>   From the look of the code I infer we normally expect `ptr' not to point
> at the backup buffer as that will only happen in the case of a `malloc'
> failure, so firstly such a forward branch usually won't indeed be taken,
> making it virtually free for the fall-through case, and secondly please
> annotate the expression with `__glibc_unlikely' accordingly.

That's a good point, I'll move it to libioP.h.

>> @@ -212,7 +220,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);
> 
>   OK.  Mechanical update.
> 
>> @@ -260,7 +268,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);
> 
>   OK.  Mechanical update.
> 
>> @@ -636,7 +644,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);
> 
>   OK.  Mechanical update.
> 
>> @@ -998,11 +1006,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;
>> +		}
> 
>   OK.  In the unlikely case of a `malloc' failure we'll resort to the
> single-character backup buffer, avoiding an unsuccessful return.
> 
>   Thanks for discarding a comment that's no longer relevant.  From
> observation such bits are too easily missed.
> 
>> @@ -1022,7 +1033,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);
> 
>   OK.  Mechanical update.
> 
>> diff --git a/libio/libioP.h b/libio/libioP.h
>> index 34bf91fcd8..287caf8664 100644
>> --- 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.
> 
>   OK.  Consistent with DCO (but see above).
> 
>> @@ -357,6 +358,8 @@ typedef FILE *_IO_ITER;
>>   
>>   /* Generic functions */
>>   
>> +extern void _IO_free_backup_buf (FILE *, char *);
>> +libc_hidden_proto (_IO_free_backup_buf)
> 
>   OK.  But this won't be needed with a static inline function.
> 
>> @@ -911,13 +914,13 @@ extern int _IO_vscanf (const char *, va_list) __THROW;
>>   #  define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \
>>          { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \
>>   	 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, \
>> -	 0, _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock }
>> +	 0, { 0 }, _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock }
> 
>   OK.  New member initialised.
> 
>>   # 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, NULL, (FILE *) CHAIN, FD, 0, { 0 }, \
>> +	 _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock, _IO_pos_BAD, \
> 
>   OK.  New member initialised.
> 
>   I think it will make sense to keep the line breaks between the same
> members across all the four FILEBUF_LITERAL definitions so as to make it
> easier to people to match the variants against each other.
> 
>   Please coordinate with Alejandro Colomar (CC'd) on cleaning up these
> definitions, which went out of sync; cf.
> <https://inbox.sourceware.org/libc-alpha/042e25d3-1b02-c448-1f8c-84f52549f5b5@redhat.com/>.

I'll rebase on top of whatever he pushes, or alternatively I've asked if 
he could send a separate patch for this file which I can include in my 
series and push whenever my patch is ready.

>> @@ -925,12 +928,12 @@ extern int _IO_vscanf (const char *, va_list) __THROW;
>>   #  define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \
>>          { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \
>>   	 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, \
>> -	 0, _IO_pos_BAD }
>> +	 0, { 0 }, _IO_pos_BAD }
> 
>   OK.  New member initialised.
> 
>>   # else
>>   #  define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \
>>          { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \
>>   	 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, \
>> -	 0, _IO_pos_BAD, 0, 0, { 0 }, 0, _IO_pos_BAD, \
>> +	 0, { 0 }, _IO_pos_BAD, 0, 0, { 0 }, 0, _IO_pos_BAD, \
> 
>   OK.  New member initialised.
> 
>> diff --git a/libio/oldfileops.c b/libio/oldfileops.c
>> index 8f775c9094..03f4d76a57 100644
>> --- 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.
> 
>   OK.  Consistent with DCO (but see above).
> 
>> @@ -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);
> 
>   OK.  Mechanical update.
> 
>> @@ -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);
> 
>   OK.  Mechanical update.
> 
>> diff --git a/libio/wfileops.c b/libio/wfileops.c
>> index 16beab1f3a..a96bfa589b 100644
>> --- 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.
> 
>   OK.  Consistent with DCO (but see above).
> 
>> @@ -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);
> 
>   OK.  Mechanical update.
> 
>> diff --git a/stdio-common/Makefile b/stdio-common/Makefile
>> index e76e40e587..b1a04fd064 100644
>> --- 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.
> 
>   OK.  Consistent with DCO (but see above).
> 
>> @@ -303,6 +304,7 @@ tests := \
>>     tst-tmpnam \
>>     tst-ungetc \
>>     tst-ungetc-leak \
>> +  tst-ungetc-nomem \
> 
>   OK.  New test added.
> 
>> diff --git a/stdio-common/tst-ungetc-nomem.c b/stdio-common/tst-ungetc-nomem.c
>> new file mode 100644
>> index 0000000000..49db33fff3
>> --- /dev/null
>> +++ b/stdio-common/tst-ungetc-nomem.c
>> @@ -0,0 +1,115 @@
>> +/* 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 <stdio.h>
>> +#include <string.h>
>> +#include <support/check.h>
>> +#include <support/support.h>
>> +#include <support/temp_file.h>
>> +#include <support/xstdio.h>
> 
>   OK, alphabetic order.
> 
>> +
>> +extern void *__libc_malloc (size_t)
>> +     __attribute__ ((malloc)) __attribute__ ((alloc_size (1)));
> 
>   Please list both attributes together, also avoiding the question as to
> how much to indent here (as the prototype will fit in one line then).

OK.

>> +
>> +static volatile bool fail = false;
> 
>   OK, marked as `volatile' to prevent the compiler from interfering, as
> previously advised.  I note this could skip the initialiser so as to place
> it in BSS, but I'm fine with the current arrangement if you consider it
> desirable to make it explicit.
> 
>> +
>> +void *
>> +malloc (size_t sz)
>> +{
>> +  if (fail)
>> +    return NULL;
>> +
>> +  return __libc_malloc (sz);
>> +}
> 
>   OK, this interposes `malloc' so as to conditionally induce a failure and
> refers to `__libc_malloc' if the condition does not stand.  A bit hackish
> IMO, but we're in control here, so let it be.
> 
>   I think this function deserves an introductory comment, even if a single
> terse line.

OK.

>> +
>> +static int
>> +do_test (void)
>> +{
>> +  char *filename = NULL;
>> +  struct stat props = {};
> 
>   As nice as empty initialisers are they're a C23-ism, so please rewrite
> using older syntax.

We use this quite extensively across glibc, so I'm inclined to keep this 
unless you have a strong objection; it just looks so clean :)  Besides, 
as Alejandro pointed out, it's been a GNU extension since before that.

>> +  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.  */
>> +  bufsz = props.st_blksize + 2;
> 
>   Why do we want the file to be the size of the libio buffer plus 2?  The
> answer seems like a good candidate for the function's introductory
> comment.

Ack, it's basically to test to the point of running out of buffer space 
so that we test at the read underflow border.

>> +
>> +  char *buf = xmalloc (bufsz);
>> +  memset (buf, 'a', bufsz);
>> +
>> +  if (fwrite (buf, sizeof (char), bufsz, fp) != bufsz)
>> +    FAIL_EXIT1 ("fwrite failed: %m\n");
>> +  xfclose (fp);
> 
>   OK, we make a test file made up of "a" letters.
> 
>> +
>> +  /* 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 ('y', fp), 'y');
>> +
>> +      /* 2: This will result in resizing, which should fail.  */
>> +      TEST_COMPARE (ungetc ('w', 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 ('x', fp), 'x');
> 
>   This does verify new semantics, and I take it it's intentional that after
> a `malloc' failure for the initial buffer we don't go back to the minimum
> of 128 bytes for the buffer, but instead start from 2 up in a hope for a
> smaller allocation to succeed where a somewhat larger one might not.  But
> I think this new semantics should be mentioned in the change description.

OK, I'll add a comment in the pushback code change.

> 
>> +
>> +      /* 4: And fail again because this again forces an alloc, which fails.  */
>> +      fail = true;
>> +      TEST_COMPARE (ungetc ('x', 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 ('x', fp), 'x');
>> +      fail = true;
>> +      TEST_COMPARE (ungetc ('x', fp), 'x');
>> +
>> +      /* Drain out the x's.  */
>> +      TEST_COMPARE (fgetc (fp), 'x');
>> +      TEST_COMPARE (fgetc (fp), 'x');
>> +      TEST_COMPARE (fgetc (fp), 'x');
> 
>   Shouldn't the `ungetc' calls use different characters each, so that we
> have an additional check that rejected characters do not come back and
> that the accepted ones come back in the correct order?

I've grouped them, but sure, I could make each call unget/get a distinct 
char.

>> +
>> +      /* Finally, drain out the first char we had pushed back, followed by one more char
>> +	 from the stream, if present.  */
> 
>   Please wrap the comment, cf:
> <https://sourceware.org/glibc/wiki/Style_and_Conventions#A79-Column_Lines>.

Oops.

>> +      TEST_COMPARE (fgetc (fp), 'y');
>> +      char c = fgetc (fp);
>> +      if (!feof (fp))
>> +	TEST_COMPARE (c, 'a');
>> +    }
> 
>   So this loop, if successful, runs libio buffer size plus 2 times.  Please
> state in the function's introductory comment why this specific iteration
> count has been chosen.

Same as mentioned above, to test at the read underflow boundary, which 
happens at libio buffer size.

>> +
>> +  /* Final sanity check before we're done.  */
>> +  TEST_COMPARE (ferror (fp), 0);
>> +  xfclose (fp);
> 
>   OK.  Checking for no error and closing the test file.  File removed
> automagically by test support clean-up.
> 
>> +
>> +  return 0;
>> +}
>> +
>> +#include <support/test-driver.c>
> 
>   OK.
> 
>   Please resend with the updates applied (but I note further clarification
> might be needed first).

Thanks,
Sid

Re: [PATCH v4] ungetc: Guarantee single char pushback

[Siddhesh Poyarekar]

On 2024-12-16 07:58, Siddhesh Poyarekar wrote:
>>> +
>>> +void *
>>> +malloc (size_t sz)
>>> +{
>>> +  if (fail)
>>> +    return NULL;
>>> +
>>> +  return __libc_malloc (sz);
>>> +}
>>
>>   OK, this interposes `malloc' so as to conditionally induce a failure 
>> and
>> refers to `__libc_malloc' if the condition does not stand.  A bit hackish
>> IMO, but we're in control here, so let it be.
>>
>>   I think this function deserves an introductory comment, even if a 
>> single
>> terse line.
> 
> OK.
> 

I just realized while adding the comment that the reason why I was using 
__libc_malloc (which was to avoid dlsym) was a flimsy one and will in 
fact end up skipping any interposed malloc implementations.  It's not an 
immediate problem since I don't think anybody runs the testsuite with 
interposed malloc today, but I'll use dlsym anyway to be future-proof.

Sid

[PATCH v5] ungetc: Guarantee single char pushback

[Siddhesh Poyarekar]

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>
---

I don't see Alejandro's patch for only struct FILE in git or on the ML
yet so I guess I'm winning this race ;)

Changes from v4:
- Moved _IO_free_backup_buf into header as a static inline.
- Use RTLD_NEXT in test instead of __libc_malloc
- Push back distinct characters in test
- Fix other formatting and commenting nits.

Changes from v3:
- Shrunk _flags2 and moved _short_backupbuf to the old FILE struct.

Changes from v2:
- Fixed nits
- Used _IO_free_backup_buf in oldfileops and wfileops as well.
- Enhanced test to try some more cases

Changes from v1:

- Drop ungetwc from scope of the patchset
- Fixed nits
- Retain old behaviour for legacy applications
- Minimize changes to fileops
- Namespace-ize free_backup_buf
- Add a test to verify that the subsequent malloc failure results in ungetc
  failure too
- Add GNU Toolchain Authors copyright notice.

 libio/bits/types/struct_FILE.h  |   5 +-
 libio/fileops.c                 |   7 +-
 libio/genops.c                  |  16 +++--
 libio/libioP.h                  |  20 ++++--
 libio/oldfileops.c              |   5 +-
 libio/wfileops.c                |   3 +-
 stdio-common/Makefile           |   2 +
 stdio-common/tst-ungetc-nomem.c | 123 ++++++++++++++++++++++++++++++++
 8 files changed, 163 insertions(+), 18 deletions(-)
 create mode 100644 stdio-common/tst-ungetc-nomem.c

diff --git a/libio/bits/types/struct_FILE.h b/libio/bits/types/struct_FILE.h
index d8d26639d1..87197a328c 100644
--- 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 759d737ec7..d49e489f55 100644
--- 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 d7e35e67d5..02e159d6a8 100644
--- 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 34bf91fcd8..bad9c2e4e9 100644
--- 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,13 +912,13 @@ extern int _IO_vscanf (const char *, va_list) __THROW;
 #  define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \
        { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \
 	 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, \
-	 0, _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock }
+	 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, NULL, (FILE *) CHAIN, FD, 0, { 0 }, \
+	 _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock, _IO_pos_BAD, \
 	 NULL, WDP, NULL }
 # endif
 #else
@@ -925,12 +926,12 @@ extern int _IO_vscanf (const char *, va_list) __THROW;
 #  define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \
        { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \
 	 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, \
-	 0, _IO_pos_BAD }
+	 0, { 0 }, _IO_pos_BAD }
 # else
 #  define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \
        { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \
 	 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, \
-	 0, _IO_pos_BAD, 0, 0, { 0 }, 0, _IO_pos_BAD, \
+	 0, { 0 }, _IO_pos_BAD, 0, 0, { 0 }, 0, _IO_pos_BAD, \
 	 NULL, WDP, 0 }
 # endif
 #endif
@@ -1037,6 +1038,15 @@ IO_validate_vtable (const struct _IO_jump_t *vtable)
   return vtable;
 }
 
+/* Free PTR if it was allocated dynamically, i.e. it does not point to the
+   fallback _SHORT_BACKUPBUF.  */
+static 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 8f775c9094..03f4d76a57 100644
--- 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 16beab1f3a..a96bfa589b 100644
--- 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 e76e40e587..b1a04fd064 100644
--- 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
index 0000000000..b334357065
--- /dev/null
+++ b/stdio-common/tst-ungetc-nomem.c
@@ -0,0 +1,123 @@
+/* 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>
+
+extern void *__libc_malloc (size_t) __attribute__ ((malloc, alloc_size (1)));
+
+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>
-- 
2.47.1

Re: [PATCH v5] ungetc: Guarantee single char pushback

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 498 bytes --]

Hi Sid,

On Mon, Dec 16, 2024 at 10:08:07AM GMT, Siddhesh Poyarekar wrote:
> I don't see Alejandro's patch for only struct FILE in git or on the ML
> yet so I guess I'm winning this race ;)

Yep, I'm a bit busy today dealing with a boot problem in one of my
systems.  I probably won't be able to post the patch today.

Feel free to apply your patch, and I can apply mine afterwards.  I don't
think the order is important.  :)

Cheers,
Alex

-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4] ungetc: Guarantee single char pushback

[Maciej W. Rozycki]

On Mon, 16 Dec 2024, Siddhesh Poyarekar wrote:

> > > > +
> > > > +void *
> > > > +malloc (size_t sz)
> > > > +{
> > > > +  if (fail)
> > > > +    return NULL;
> > > > +
> > > > +  return __libc_malloc (sz);
> > > > +}
> > > 
> > >   OK, this interposes `malloc' so as to conditionally induce a failure and
> > > refers to `__libc_malloc' if the condition does not stand.  A bit hackish
> > > IMO, but we're in control here, so let it be.
> > > 
> > >   I think this function deserves an introductory comment, even if a single
> > > terse line.
> > 
> > OK.
> > 
> 
> I just realized while adding the comment that the reason why I was using
> __libc_malloc (which was to avoid dlsym) was a flimsy one and will in fact end
> up skipping any interposed malloc implementations.  It's not an immediate
> problem since I don't think anybody runs the testsuite with interposed malloc
> today, but I'll use dlsym anyway to be future-proof.

 But is using `dlsym' going to actually work?

 AFAIK a symbol in the main executable pre-empts any other ones of the 
same name coming from the loader's initial namespace (i.e. including any 
pulled early via LD_PRELOAD, but not those loaded via `dlopen' into a new 
namespace).

 NB we have a preexisting use of `__libc_malloc' in another test case, so 
if coming up with an alternative arrangement, I suggest to cover it too.

  Maciej

Re: [PATCH v4] ungetc: Guarantee single char pushback

[Siddhesh Poyarekar]

On 2024-12-16 11:16, Maciej W. Rozycki wrote:
>> I just realized while adding the comment that the reason why I was using
>> __libc_malloc (which was to avoid dlsym) was a flimsy one and will in fact end
>> up skipping any interposed malloc implementations.  It's not an immediate
>> problem since I don't think anybody runs the testsuite with interposed malloc
>> today, but I'll use dlsym anyway to be future-proof.
> 
>   But is using `dlsym' going to actually work?
> 
>   AFAIK a symbol in the main executable pre-empts any other ones of the
> same name coming from the loader's initial namespace (i.e. including any
> pulled early via LD_PRELOAD, but not those loaded via `dlopen' into a new
> namespace).

The idea is to use RTLD_NEXT from within the malloc in the executable to 
get the next available 'malloc'.  This would find the glibc malloc under 
normal circumstances, but also be able to use any interposed malloc if 
that's a future need.  I've submitted v5 with that change.

Thanks,
Sid

Re: [PATCH v5] ungetc: Guarantee single char pushback

[Maciej W. Rozycki]

On Mon, 16 Dec 2024, Alejandro Colomar wrote:

> Feel free to apply your patch, and I can apply mine afterwards.  I don't
> think the order is important.  :)

 Conversely I think getting the order right will help with backporting 
downstream.

  Maciej

[PATCH v6] ungetc: Guarantee single char pushback

[Siddhesh Poyarekar]

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>
---
Changes from v5:
- Rebased on top of Alejandro's libioP.h change

Changes from v4:
- Moved _IO_free_backup_buf into header as a static inline.
- Use RTLD_NEXT in test instead of __libc_malloc
- Push back distinct characters in test
- Fix other formatting and commenting nits.

Changes from v3:
- Shrunk _flags2 and moved _short_backupbuf to the old FILE struct.

Changes from v2:
- Fixed nits
- Used _IO_free_backup_buf in oldfileops and wfileops as well.
- Enhanced test to try some more cases

Changes from v1:

- Drop ungetwc from scope of the patchset
- Fixed nits
- Retain old behaviour for legacy applications
- Minimize changes to fileops
- Namespace-ize free_backup_buf
- Add a test to verify that the subsequent malloc failure results in ungetc
  failure too
- Add GNU Toolchain Authors copyright notice.

 libio/bits/types/struct_FILE.h  |   5 +-
 libio/fileops.c                 |   7 +-
 libio/genops.c                  |  16 +++--
 libio/libioP.h                  |  20 ++++--
 libio/oldfileops.c              |   5 +-
 libio/wfileops.c                |   3 +-
 stdio-common/Makefile           |   2 +
 stdio-common/tst-ungetc-nomem.c | 123 ++++++++++++++++++++++++++++++++
 8 files changed, 163 insertions(+), 18 deletions(-)
 create mode 100644 stdio-common/tst-ungetc-nomem.c

diff --git a/libio/bits/types/struct_FILE.h b/libio/bits/types/struct_FILE.h
index d8d26639d1..87197a328c 100644
--- 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 759d737ec7..d49e489f55 100644
--- 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 d7e35e67d5..02e159d6a8 100644
--- 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 70e2bdfc9d..e50177114b 100644
--- 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
@@ -912,13 +913,13 @@ extern int _IO_vscanf (const char *, va_list) __THROW;
        { _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 }
+	 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, NULL, (FILE *) CHAIN, FD, 0, { 0 }, \
+	 _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock, _IO_pos_BAD, \
 	 NULL, WDP, NULL }
 # endif
 #else
@@ -927,13 +928,13 @@ extern int _IO_vscanf (const char *, va_list) __THROW;
        { _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 }, _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 }, 0, _IO_pos_BAD, \
+	 0, { 0 }, _IO_pos_BAD, 0, 0, { 0 }, 0, _IO_pos_BAD, \
 	 NULL, WDP, NULL }
 # endif
 #endif
@@ -1040,6 +1041,15 @@ IO_validate_vtable (const struct _IO_jump_t *vtable)
   return vtable;
 }
 
+/* Free PTR if it was allocated dynamically, i.e. it does not point to the
+   fallback _SHORT_BACKUPBUF.  */
+static 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 8f775c9094..03f4d76a57 100644
--- 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 16beab1f3a..a96bfa589b 100644
--- 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 e76e40e587..b1a04fd064 100644
--- 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
index 0000000000..b334357065
--- /dev/null
+++ b/stdio-common/tst-ungetc-nomem.c
@@ -0,0 +1,123 @@
+/* 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>
+
+extern void *__libc_malloc (size_t) __attribute__ ((malloc, alloc_size (1)));
+
+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>
-- 
2.47.1

Re: [PATCH v5] ungetc: Guarantee single char pushback

[Siddhesh Poyarekar]

On 2024-12-16 22:34, Maciej W. Rozycki wrote:
> On Mon, 16 Dec 2024, Alejandro Colomar wrote:
> 
>> Feel free to apply your patch, and I can apply mine afterwards.  I don't
>> think the order is important.  :)
> 
>   Conversely I think getting the order right will help with backporting
> downstream.

I sent v6 rebased on top of Alejandro's patch.

Sid

Re: [PATCH v6] ungetc: Guarantee single char pushback

[Maciej W. Rozycki]

On Tue, 17 Dec 2024, Siddhesh Poyarekar wrote:

> 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.

 OK.  Thanks, I find this description much better now.

> diff --git a/libio/libioP.h b/libio/libioP.h
> index 70e2bdfc9d..e50177114b 100644
> --- a/libio/libioP.h
> +++ b/libio/libioP.h
[...]
> @@ -912,13 +913,13 @@ extern int _IO_vscanf (const char *, va_list) __THROW;
>         { _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 }
> +	 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, NULL, (FILE *) CHAIN, FD, 0, { 0 }, \
> +	 _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock, _IO_pos_BAD, \

 As noted in v4 can you please reformat the variants of the initialiser 
such that the line breaks are still between the same member pairs across 
all the four FILEBUF_LITERAL definitions?  This will continue making it 
easier to verify visually that the definitions are consistent.

> @@ -927,13 +928,13 @@ extern int _IO_vscanf (const char *, va_list) __THROW;
>         { _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 }, _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 }, 0, _IO_pos_BAD, \
> +	 0, { 0 }, _IO_pos_BAD, 0, 0, { 0 }, 0, _IO_pos_BAD, \

 Same here if needed after the change above.

> @@ -1040,6 +1041,15 @@ IO_validate_vtable (const struct _IO_jump_t *vtable)
>    return vtable;
>  }
>  
> +/* Free PTR if it was allocated dynamically, i.e. it does not point to the
> +   fallback _SHORT_BACKUPBUF.  */

 Hmm, this description just repeats what the code does.  How about:

/* In the case of an allocation failure we resort to using a fixed buffer.  
   Free PTR unless pointing to that buffer.  */

or suchlike?

> +static inline void
> +_IO_free_backup_buf (FILE *fp, char *ptr)
> +{
> +  if (ptr != fp->_short_backupbuf)
> +    free (ptr);
> +}
> +

 Please make it __always_inline too so as to prevent the definition from 
going missing should the function fail to get inlined otherwise.  Sorry 
not to be clear about it with v4 (my Linux kernel background shows here).

> diff --git a/stdio-common/tst-ungetc-nomem.c b/stdio-common/tst-ungetc-nomem.c
> new file mode 100644
> index 0000000000..b334357065
> --- /dev/null
> +++ b/stdio-common/tst-ungetc-nomem.c
[...]
> +
> +extern void *__libc_malloc (size_t) __attribute__ ((malloc, alloc_size (1)));

 This is not needed anymore.

> +
> +/* 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);
                  ^
 Do we want a space here?  I think not.

> +
> +  if (real_malloc == NULL)
> +    real_malloc = dlsym (RTLD_NEXT, "malloc");

 OK.  Using a GNU feature to get at the original symbol.

> +
> +  return real_malloc (sz);
> +}

 OK.

> +
> +  /* 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');

 OK.  Thanks for making the characters distinct.

 I look forward to v7; hopefully the final one.

  Maciej

Re: [PATCH v6] ungetc: Guarantee single char pushback

[Siddhesh Poyarekar]

On 2024-12-17 09:44, Maciej W. Rozycki wrote:
>> @@ -912,13 +913,13 @@ extern int _IO_vscanf (const char *, va_list) __THROW;
>>          { _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 }
>> +	 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, NULL, (FILE *) CHAIN, FD, 0, { 0 }, \
>> +	 _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock, _IO_pos_BAD, \
> 
>   As noted in v4 can you please reformat the variants of the initialiser
> such that the line breaks are still between the same member pairs across
> all the four FILEBUF_LITERAL definitions?  This will continue making it
> easier to verify visually that the definitions are consistent.
> 

Oops, sorry I missed that; I've made this change in v7, including 
converting an instance of 0 -> NULL that was missed in Alejandro's 
patch.  Sending out shortly.

Thanks,
Sid

[PATCH v7] ungetc: Guarantee single char pushback

[Siddhesh Poyarekar]

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>
---
Changes from v6:
- Fixed formatting nits
- Make _IO_free_backup_buf __always_inline

Changes from v5:
- Rebased on top of Alejandro's libioP.h change

Changes from v4:
- Moved _IO_free_backup_buf into header as a static inline.
- Use RTLD_NEXT in test instead of __libc_malloc
- Push back distinct characters in test
- Fix other formatting and commenting nits.

Changes from v3:
- Shrunk _flags2 and moved _short_backupbuf to the old FILE struct.

Changes from v2:
- Fixed nits
- Used _IO_free_backup_buf in oldfileops and wfileops as well.
- Enhanced test to try some more cases

Changes from v1:

- Drop ungetwc from scope of the patchset
- Fixed nits
- Retain old behaviour for legacy applications
- Minimize changes to fileops
- Namespace-ize free_backup_buf
- Add a test to verify that the subsequent malloc failure results in ungetc
  failure too
- Add GNU Toolchain Authors copyright notice.

 libio/bits/types/struct_FILE.h  |   5 +-
 libio/fileops.c                 |   7 +-
 libio/genops.c                  |  16 +++--
 libio/libioP.h                  |  30 +++++---
 libio/oldfileops.c              |   5 +-
 libio/wfileops.c                |   3 +-
 stdio-common/Makefile           |   2 +
 stdio-common/tst-ungetc-nomem.c | 121 ++++++++++++++++++++++++++++++++
 8 files changed, 166 insertions(+), 23 deletions(-)
 create mode 100644 stdio-common/tst-ungetc-nomem.c

diff --git a/libio/bits/types/struct_FILE.h b/libio/bits/types/struct_FILE.h
index d8d26639d1..87197a328c 100644
--- 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 759d737ec7..d49e489f55 100644
--- 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 d7e35e67d5..02e159d6a8 100644
--- 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 70e2bdfc9d..714abbd549 100644
--- 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 }, 0, _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 8f775c9094..03f4d76a57 100644
--- 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 16beab1f3a..a96bfa589b 100644
--- 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 e76e40e587..b1a04fd064 100644
--- 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
index 0000000000..0872de6050
--- /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>
-- 
2.47.1

Re: [PATCH v6] ungetc: Guarantee single char pushback

[Maciej W. Rozycki]

On Tue, 17 Dec 2024, Siddhesh Poyarekar wrote:

> >   As noted in v4 can you please reformat the variants of the initialiser
> > such that the line breaks are still between the same member pairs across
> > all the four FILEBUF_LITERAL definitions?  This will continue making it
> > easier to verify visually that the definitions are consistent.
> > 
> 
> Oops, sorry I missed that; I've made this change in v7, including converting
> an instance of 0 -> NULL that was missed in Alejandro's patch.  Sending out
> shortly.

 Sigh, that instance was well hidden, thanks for catching it and sorry to 
miss it in the review.

 Would you please split this piece off though and apply beforehand so that 
it's not completely obscured by your ungetc change?  I've pasted the hunk 
extracted below for your convenience (lines might be off); please feel 
free to add my:

Reviewed-by: Maciej W. Rozycki <macro@redhat.com>

for it.

  Maciej

---
 libio/libioP.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: glibc/libio/libioP.h
===================================================================
--- glibc.orig/libio/libioP.h
+++ glibc/libio/libioP.h
@@ -933,7 +933,7 @@ extern int _IO_vscanf (const char *, va_
        { _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 }, 0, _IO_pos_BAD, \
+	 0, _IO_pos_BAD, 0, 0, { 0 }, NULL, _IO_pos_BAD, \
 	 NULL, WDP, NULL }
 # endif
 #endif

Re: [PATCH v7] ungetc: Guarantee single char pushback

[Maciej W. Rozycki]

On Tue, 17 Dec 2024, Siddhesh Poyarekar wrote:

> diff --git a/libio/libioP.h b/libio/libioP.h
> index 70e2bdfc9d..714abbd549 100644
> --- a/libio/libioP.h
> +++ b/libio/libioP.h
[...]
> @@ -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 }, 0, _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 }

 OK, line breaks now aligned, thank you.  As mentioned for v6 please split 
the final NULL fix off from this change.

> @@ -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.  */

 OK.

> +static __always_inline void
> +_IO_free_backup_buf (FILE *fp, char *ptr)

 OK.

 No need to repost as far as I'm concerned just for the NULL fix update 
mentioned above; this is:

Reviewed-by: Maciej W. Rozycki <macro@redhat.com>

with said update applied.  Thank you for your contribution.

  Maciej

[committed 1/2] libio: Fix last NULL-as-0 issue in libioP.h

[Siddhesh Poyarekar]

Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
Reviewed-by: Maciej W. Rozycki <macro@redhat.com>
---
 libio/libioP.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libio/libioP.h b/libio/libioP.h
index 70e2bdfc9d..ad45579e13 100644
--- a/libio/libioP.h
+++ b/libio/libioP.h
@@ -933,7 +933,7 @@ extern int _IO_vscanf (const char *, va_list) __THROW;
        { _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 }, 0, _IO_pos_BAD, \
+	 0, _IO_pos_BAD, 0, 0, { 0 }, NULL, _IO_pos_BAD, \
 	 NULL, WDP, NULL }
 # endif
 #endif
-- 
2.47.1

[committed 2/2] ungetc: Guarantee single char pushback

[Siddhesh Poyarekar]

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>
---
 libio/bits/types/struct_FILE.h  |   5 +-
 libio/fileops.c                 |   7 +-
 libio/genops.c                  |  16 +++--
 libio/libioP.h                  |  30 +++++---
 libio/oldfileops.c              |   5 +-
 libio/wfileops.c                |   3 +-
 stdio-common/Makefile           |   2 +
 stdio-common/tst-ungetc-nomem.c | 121 ++++++++++++++++++++++++++++++++
 8 files changed, 166 insertions(+), 23 deletions(-)
 create mode 100644 stdio-common/tst-ungetc-nomem.c

diff --git a/libio/bits/types/struct_FILE.h b/libio/bits/types/struct_FILE.h
index d8d26639d1..87197a328c 100644
--- 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 759d737ec7..d49e489f55 100644
--- 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 d7e35e67d5..02e159d6a8 100644
--- 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 ad45579e13..714abbd549 100644
--- 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 8f775c9094..03f4d76a57 100644
--- 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 16beab1f3a..a96bfa589b 100644
--- 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 e76e40e587..b1a04fd064 100644
--- 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
index 0000000000..0872de6050
--- /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>
-- 
2.47.1

[PATCH] tests: Verify inheritance of cpu affinity

[Siddhesh Poyarekar]

Add a couple of tests to verify that CPU affinity set using
sched_setaffinity and pthread_setaffinity_np are inherited by a child
process and child thread.

Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
---
 nptl/Makefile                                 |   1 +
 nptl/tst-pthread-affinity-inheritance.c       |  71 ++++++++
 nptl/tst-skeleton-affinity-inheritance.c      | 152 ++++++++++++++++++
 sysdeps/unix/sysv/linux/Makefile              |   1 +
 .../linux/tst-sched-affinity-inheritance.c    |  71 ++++++++
 5 files changed, 296 insertions(+)
 create mode 100644 nptl/tst-pthread-affinity-inheritance.c
 create mode 100644 nptl/tst-skeleton-affinity-inheritance.c
 create mode 100644 sysdeps/unix/sysv/linux/tst-sched-affinity-inheritance.c

diff --git a/nptl/Makefile b/nptl/Makefile
index 88077e27bb..429b8e3c33 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -313,6 +313,7 @@ tests = \
   tst-mutexpi11 \
   tst-mutexpi12 \
   tst-once5 \
+  tst-pthread-affinity-inheritance \
   tst-pthread-attr-affinity \
   tst-pthread-attr-affinity-fail \
   tst-pthread-attr-sigmask \
diff --git a/nptl/tst-pthread-affinity-inheritance.c b/nptl/tst-pthread-affinity-inheritance.c
new file mode 100644
index 0000000000..c020530dd9
--- /dev/null
+++ b/nptl/tst-pthread-affinity-inheritance.c
@@ -0,0 +1,71 @@
+/* CPU Affinity inheritance test - pthread_{gs}etaffinity_np.
+   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/>.  */
+
+/* See top level comment in nptl/tst-skeleton-affinity-inheritance.c for a
+   description of this test.  */
+#include <pthread.h>
+#include <sched.h>
+#include <stdio.h>
+#include <string.h>
+#include <support/check.h>
+
+static void
+set_my_affinity (size_t size, const cpu_set_t *set)
+{
+  int ret = pthread_setaffinity_np (pthread_self (), size, set);
+
+  if (ret != 0)
+    FAIL ("pthread_setaffinity_np returned %d (%s)", ret, strerror (ret));
+}
+
+static void
+verify_my_affinity (int nproc, size_t size, const cpu_set_t *expected_set)
+{
+  cpu_set_t *set = CPU_ALLOC (nproc);
+  cpu_set_t *xor_set = CPU_ALLOC (nproc);
+
+  if (set == NULL || xor_set== NULL)
+    FAIL_EXIT1 ("verify_my_affinity: Failed to allocate cpuset: %m\n");
+
+  int ret = pthread_getaffinity_np (pthread_self (), size, set);
+  if (ret != 0)
+    FAIL ("pthread_getaffinity_np returned %d (%s)", ret, strerror (ret));
+
+  CPU_XOR_S (size, xor_set, expected_set, set);
+
+  int cpucount = CPU_COUNT_S (size, xor_set);
+
+  if (cpucount > 0)
+    {
+      FAIL ("Affinity mask not inherited, "
+	    "following %d CPUs mismatched in the expected and actual sets: ",
+	    cpucount);
+      for (int cur = 0; cur < nproc && cpucount >= 0; cur++)
+	if (CPU_ISSET_S (size, cur, xor_set))
+	  {
+	    printf ("%d ", cur);
+	    cpucount--;
+	  }
+      printf ("\n");
+    }
+
+  CPU_FREE (set);
+  CPU_FREE (xor_set);
+}
+
+#include "tst-skeleton-affinity-inheritance.c"
diff --git a/nptl/tst-skeleton-affinity-inheritance.c b/nptl/tst-skeleton-affinity-inheritance.c
new file mode 100644
index 0000000000..6de6d9c942
--- /dev/null
+++ b/nptl/tst-skeleton-affinity-inheritance.c
@@ -0,0 +1,152 @@
+/* CPU Affinity inheritance test - common infrastructure.
+   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/>.  */
+
+/* The general idea of this test is to verify that the set of CPUs assigned to
+   a task gets inherited by a child (thread or process) of that task.  This is
+   a framework that is included by specific APIs for the test, e.g.
+   sched_getaffinity/sched_setaffinity and
+   pthread_setaffinity_np/pthread_getaffinity_np.  This is a framework, actual
+   tests entry points are in nptl/tst-pthread-affinity-inheritance.c and
+   sysdeps/unix/sysv/linux/tst-sched-affinity-inheritance.c.
+
+   There are two levels to the test with two different CPU masks.  The first
+   level verifies that the affinity set on the main process is inherited by its
+   children subprocess or thread.  The second level verifies that a subprocess
+   or subthread passes on its affinity to their respective subprocess or
+   subthread.  We set a slightly different mask in both levels to ensure that
+   they're both inherited.  */
+
+#include <errno.h>
+#include <stdio.h>
+#include <support/test-driver.h>
+#include <support/xthread.h>
+#include <support/xunistd.h>
+#include <sys/sysinfo.h>
+#include <sys/wait.h>
+
+struct test_param
+{
+  int nproc;
+  cpu_set_t *set;
+  size_t size;
+  bool entry;
+};
+
+void __attribute__((noinline))
+set_cpu_mask (struct test_param *param, bool entry)
+{
+  int cpus = param->nproc;
+
+  /* Less CPUS for the first level, if that's possible.  */
+  if (entry && cpus > 1)
+    cpus--;
+
+  CPU_ZERO_S (param->size, param->set);
+  while (cpus > 0)
+    CPU_SET_S (--cpus, param->size, param->set);
+
+  if (CPU_COUNT_S (param->size, param->set) == 0)
+    FAIL_EXIT1 ("Failed to add any CPUs to the affinity set\n");
+}
+
+static void *
+child_test (void *arg)
+{
+  struct test_param *param = arg;
+
+  printf ("%d:%d        child\n", getpid (), gettid ());
+  verify_my_affinity (param->nproc, param->size, param->set);
+  return NULL;
+}
+
+void *
+do_one_test (void *arg)
+{
+  void *(*child) (void *) = NULL;
+  struct test_param *param = arg;
+  bool entry = param->entry;
+
+  if (entry)
+    {
+      printf ("%d:%d Start test run\n", getpid (), gettid ());
+      /* First level: Reenter as a subprocess and then as a subthread.  */
+      child = do_one_test;
+      set_cpu_mask (param, true);
+      set_my_affinity (param->size, param->set);
+      param->entry = false;
+    }
+  else
+    {
+      /* Verification for the first level.  */
+      verify_my_affinity (param->nproc, param->size, param->set);
+
+      /* Launch the second level test, launching CHILD_TEST as a subprocess and
+	 then as a subthread.  Use a different mask to see if it gets
+	 inherited.  */
+      child = child_test;
+      set_cpu_mask (param, false);
+      set_my_affinity (param->size, param->set);
+    }
+
+  /* Verify that a child of a thread/process inherits the affinity mask.  */
+  printf ("%d:%d%sdo_one_test: fork\n", getpid (), gettid (),
+	  entry ? " " : "    ");
+  int pid = xfork ();
+
+  if (pid == 0)
+    {
+      child (param);
+      return NULL;
+    }
+
+  xwaitpid (pid, NULL, 0);
+
+  /* Verify that a subthread of a thread/process inherits the affinity
+     mask.  */
+  printf ("%d:%d%sdo_one_test: thread\n", getpid (), gettid (),
+	  entry ? " " : "    ");
+  pthread_t t = xpthread_create (NULL, child, param);
+  xpthread_join (t);
+
+  return NULL;
+}
+
+static int
+do_test (void)
+{
+  int num_cpus = get_nprocs ();
+
+  struct test_param param =
+    {
+      .nproc = num_cpus,
+      .set = CPU_ALLOC (num_cpus),
+      .size = CPU_ALLOC_SIZE (num_cpus),
+      .entry = true,
+    };
+
+  if (param.set == NULL)
+    FAIL_EXIT1 ("error: CPU_ALLOC (%d) failed\n", num_cpus);
+
+  do_one_test (&param);
+
+  CPU_FREE (param.set);
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
index eb9c697ce5..1be3195ecf 100644
--- a/sysdeps/unix/sysv/linux/Makefile
+++ b/sysdeps/unix/sysv/linux/Makefile
@@ -226,6 +226,7 @@ tests += \
   tst-process_mrelease \
   tst-quota \
   tst-rlimit-infinity \
+  tst-sched-affinity-inheritance \
   tst-sched_setattr \
   tst-scm_rights \
   tst-sigtimedwait \
diff --git a/sysdeps/unix/sysv/linux/tst-sched-affinity-inheritance.c b/sysdeps/unix/sysv/linux/tst-sched-affinity-inheritance.c
new file mode 100644
index 0000000000..fe0297f743
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/tst-sched-affinity-inheritance.c
@@ -0,0 +1,71 @@
+/* CPU Affinity inheritance test - sched_{gs}etaffinity.
+   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/>.  */
+
+/* See top level comment in nptl/tst-skeleton-affinity-inheritance.c for a
+   description of this test.  */
+
+#include <sched.h>
+#include <string.h>
+#include <stdio.h>
+#include <support/check.h>
+
+static void
+set_my_affinity (size_t size, const cpu_set_t *set)
+{
+  int ret = sched_setaffinity (0, size, set);
+
+  if (ret != 0)
+    FAIL ("sched_setaffinity returned %d (%s)", ret, strerror (ret));
+}
+
+static void
+verify_my_affinity (int nproc, size_t size, const cpu_set_t *expected_set)
+{
+  cpu_set_t *set = CPU_ALLOC (nproc);
+  cpu_set_t *xor_set = CPU_ALLOC (nproc);
+
+  if (set == NULL || xor_set== NULL)
+    FAIL_EXIT1 ("verify_my_affinity: Failed to allocate cpuset: %m\n");
+
+  int ret = sched_getaffinity (0, size, set);
+  if (ret != 0)
+    FAIL ("sched_getaffinity returned %d (%s)", ret, strerror (ret));
+
+  CPU_XOR_S (size, xor_set, expected_set, set);
+
+  int cpucount = CPU_COUNT_S (size, xor_set);
+
+  if (cpucount > 0)
+    {
+      FAIL ("Affinity mask not inherited, "
+	    "following %d CPUs mismatched in the expected and actual sets:\n",
+	    cpucount);
+      for (int cur = 0; cur < nproc && cpucount >= 0; cur++)
+	if (CPU_ISSET_S (size, cur, xor_set))
+	  {
+	    printf ("%d ", cur);
+	    cpucount--;
+	  }
+      printf ("\n");
+    }
+
+  CPU_FREE (set);
+  CPU_FREE (xor_set);
+}
+
+#include <nptl/tst-skeleton-affinity-inheritance.c>
-- 
2.47.1