Re: [PATCH 11/11] syslog: Use a printf buffer directly to construct the entire packet

[Florian Weimer <fweimer@redhat.com>]

* Adhemerval Zanella Netto:

> On 09/02/24 12:26, Florian Weimer wrote:
>> This defers buffer management largely to the asprintf implementation.
>> It is quite close to the original implementation around
>> open_memstream, except that an on-stack buffer is used for shorter
>> messages, and that strftime no longer writes directly into the
>> buffer.
>> 
>> The new version no longer uses the (slow) %n format specifier.
>> It also fixes an issue in the localtime_r failure path, where
>> the message is prefixed with ": " due to an incorrect placement
>> of the %n specifier.
>
> What I am not sure if this is really the direction we want for
> internal FILE usage.  I had the impression that the printf buffer
> internal API was mainly meant to improve the old FILE implementation
> and its historical drawnbacks and limitations.  For internal usage we
> would continue to use standard FILE API, should we move to always use
> printf buffers instead?

There are currently no internal uses (that I can see) of _IO_strfile for
writing purposes.  The psiginfo function uses __fmemopen, but that seems
excessive.  The syslog implementation was the only user of
open_memstream.  The reason for the open_memstream removal was
allocation removal, which makes sense for a logging function.

We must have manual constructs that use some printf function variant to
write to temporary buffers elsewhere.  One example is
stdio-common/psignal.c.  I can't find others outside sunrpc/ right now,
but they likely exist.  Switching those to fmemopen/open_memstream would
introduce the allocation issue as well, and dynamically sized printf
buffers could be a replacement.

I know that musl uses its _IO_strfile equivalent in such cases, but our
libio implementation is much more heavyweight, and it's hard to review
if new direct low-level uses of the libio internals are actually
correct.

> I am asking because it is a lot of code and refactoring for a specific
> code that I would like to avoid change due the recent issues.  Most of 
> complication is the static buffer optimization, so maybe we should just
> remove it?

Pretty much all buffer management code is deleted.  We now have even
fewer code of that than in the previous open_memstream-based function.

> Also, since the motivation for this change is just to remove the %n
> requirement, maybe we can just not enable it on syslog instead (since
> we now that the internal calls should not act as a gadget)?  

That doesn't remove the complicated buffer management.

Thanks,
Florian