[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