[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