diff mbox series

[2/3] ungetc: Guarantee single char pushback

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

Commit Message

Siddhesh Poyarekar Nov. 8, 2024, 5:14 p.m. UTC
The C standard requires that 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

Comments

Florian Weimer Nov. 28, 2024, 1:45 p.m. UTC | #1
* 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
Siddhesh Poyarekar Nov. 28, 2024, 5:05 p.m. UTC | #2
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
Florian Weimer Nov. 28, 2024, 5:23 p.m. UTC | #3
* 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
Siddhesh Poyarekar Nov. 28, 2024, 5:26 p.m. UTC | #4
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...
Maciej W. Rozycki Nov. 29, 2024, 5:47 a.m. UTC | #5
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
Florian Weimer Nov. 29, 2024, 7:02 a.m. UTC | #6
* 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
Siddhesh Poyarekar Nov. 29, 2024, 11:44 a.m. UTC | #7
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;
Maciej W. Rozycki Nov. 29, 2024, 12:15 p.m. UTC | #8
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
Siddhesh Poyarekar Nov. 29, 2024, 12:20 p.m. UTC | #9
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
Florian Weimer Nov. 29, 2024, 12:28 p.m. UTC | #10
* 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
Maciej W. Rozycki Nov. 29, 2024, 7:05 p.m. UTC | #11
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
diff mbox series

Patch

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>