[PATCH v2 05/14] stdio-common: Introduce buffers for implementing printf

[Florian Weimer]

* Adhemerval Zanella:

> Some comments below.  I am not very found on this approach, it is *another*
> layer on our already convoluted libio/stdio code; although I don't have a
> better solution.  I *really* wish we could move away from libio...

On the plus side, it completely eliminates libio for sprintf, strfromd,
strfmon.

>> +/* <printf_buffer_as_file.h> introduces a way to use struct
>> +   __printf_buffer objects from FILE * streams.  To avoid storing a
>> +   function pointer (or vtable pointer) in struct __printf_buffer
>> +   (which would defeat libio vtable hardening), a switch statement
>> +   over the different flush implementations is used to implement
>> +   __printf_buffer_flush.
>> +
>> +   __printf_buffer_mode_failed is special: it is the sticky failure
>> +   indicator.  Unlike struct alloc_buffer, this is not folded into
>> +   write_ptr, so that snprintf and other string-writing functions can
>> +   discover the end of the string even in the error case, to be able
>> +   to add the null terminator.  */
>> +enum __printf_buffer_mode
>> +  {
>> +    __printf_buffer_mode_failed,
>> +    __printf_buffer_mode_to_file,
>> +  };
>
> Do we really need to use namespace safe names for types as well? I usually
> think it is usually clear to use otherwise.

It's needed to enable the type-generic implementations.
stdio-common/Xprintf_buffer_flush.c uses Xprintf (buffer_mode_failed).
I used to have two macros (Xprintf and __Xprintf), but I moved away from
this because using __printf_ everywhere seemed more consistent.

>> +/* Marks the buffer as failed, so that __printf_buffer_has_failed
>> +   returns true and future flush operations are no-ops.  */
>> +static inline void
>> +__printf_buffer_mark_failed (struct __printf_buffer *buf)
>
> For other internal interfaces it seems that current pratice is to use
> non double underscore static inline function wrappers over the namespace
> safe symbols.  Maybe we should use the same strategy here.

Same reason as above.
>
>> +/* Write COUNT bytes starting at S to BUF.  S must not overlap with
>> +   the internal buffer.  */
>
> Maybe add a overlpa check on Xprintf_buffer_write.c ?  Not sure if it is
> worth though.

We actually can't because for sprintf, the target buffer (as specified)
extends to the end of the address space, it's unbounded.

>> +int
>> +Xprintf_buffer_done (struct Xprintf_buffer *buf)
>> +{
>> +  if (Xprintf_buffer_has_failed (buf))
>> +    return -1;
>> +
>> +  /* Use uintptr_t to deal with ptrdiff_t overflows (to some degree).  */
>
> I am not sure using uintptr_t yield any gain in fact, if 
> 'buf->write_ptr - buf->write_base' can not fit in ptrdiff_t it means the
> code already trigger UB in write_ptr update.

I should probably remove that.

>> +  uintptr_t written_current = buf->write_ptr - buf->write_base;
>> +  unsigned int written_total;
>> +  if (__builtin_add_overflow (buf->written, written_current, &written_total)
>
> I think it should use INT_ADD_OVERFLOW since we still support GCC 6.5 and
> __builtin_{add,sub}_overflow is not reliable with GCC 6.X [1].
>
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98269

We require GCC 7.1 on s390x, see 844b4d8b4b937fe6943d2c0c80ce7d871cdb1eb5.

I'm not sure if INT_ADD_OVERFLOW deals correctly with mixed types.

>> +void
>> +Xprintf (buffer_puts_1) (struct Xprintf_buffer *buf, const CHAR_T *s)
>> +{
>> +  if (__glibc_unlikely (Xprintf_buffer_has_failed (buf)))
>> +    return;
>> +
>> +  while (*s != 0)
>> +    {
>> +      if (buf->write_ptr == buf->write_end && !Xprintf_buffer_flush (buf))
>> +        return;
>> +      assert (buf->write_ptr != buf->write_end);
>> +      size_t to_copy = STRNLEN (s, buf->write_end - buf->write_ptr);
>> +      buf->write_ptr = MEMPCPY (buf->write_ptr, s, to_copy);
>> +      s += to_copy;
>> +    }
>> +}
>
> Does it really need a loop here?

Yes, we might reach the end of the buffer with data left to write, so we
have to flush it.  This can happen with asprintf and of course in the
FILE * case.

Thanks,
Florian