linux-kernel - Re: [PATCH v2 next 03/11] tools/nolibc/printf: Add buffering to vfprintf() callback.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20260207233651.577f0fcb@pumpkin>
Date: Sat, 7 Feb 2026 23:36:51 +0000
From: David Laight <david.laight.linux@...il.com>
To: Willy Tarreau <w@....eu>
Cc: Thomas Weißschuh <linux@...ssschuh.net>,
 linux-kernel@...r.kernel.org, Cheng Li <lechain@...il.com>
Subject: Re: [PATCH v2 next 03/11] tools/nolibc/printf: Add buffering to
 vfprintf() callback.

On Sat, 7 Feb 2026 20:29:34 +0100
Willy Tarreau <w@....eu> wrote:

> On Fri, Feb 06, 2026 at 07:11:13PM +0000, david.laight.linux@...il.com wrote:
> > From: David Laight <david.laight.linux@...il.com>
> > 
> > Add per-call buffering to the vprintf() callback.
> > While this adds some extra code it will speed things up and
> > makes a massive difference to anyone looking at strace output.  
> 
> This patch alone adds more than 200 extra bytes to the smallest binary
> for something that was never expressed as a need by users:
> 
>  $ size hello-patch*
>    text    data     bss     dec     hex filename
>    1859      48      24    1931     78b hello-patch1
>    2071      48      24    2143     85f hello-patch2
> 
> I doubt it would make sense to have a build option to choose this.
> Or alternately one could decide do disable it when __OPTIMIZE_SIZE__
> is defined. I just tried quickly and it does the job:

That probably makes sense.
For anything non-trivial the extra size is noise.
Actually is would be easier to read with a single #if covering the
whole lot.

> 
>   @@ -390,6 +390,7 @@ struct __nolibc_fprintf_cb_state {
>    
>    static int __nolibc_fprintf_cb(void *v_state, const char *buf, size_t size)
>    {
>   +#if !defined(__OPTIMIZE_SIZE__)
>           struct __nolibc_fprintf_cb_state *state = v_state;
>           unsigned int off = state->buf_offset;
>    
>   @@ -407,16 +408,24 @@ static int __nolibc_fprintf_cb(void *v_state, const char *buf, size_t size)
>                   memcpy(state->buf + off, buf, size);
>           }
>           return 0;
>   +#else
>   +       /* v_state is the stream */
>   +       return size ? _fwrite(buf, size, v_state) : 0;
>   +#endif
>    }
>    
>    static __attribute__((unused, format(printf, 2, 0)))
>    int vfprintf(FILE *stream, const char *fmt, va_list args)
>    {
>   +#if !defined(__OPTIMIZE_SIZE__)
>           struct __nolibc_fprintf_cb_state state;
>    
>           state.stream = stream;
>           state.buf_offset = 0;
>           return __nolibc_printf(__nolibc_fprintf_cb, &state, fmt, args);
>   +#else
>   +       return __nolibc_printf(__nolibc_fprintf_cb, stream, fmt, args);
>   +#endif
>    }
>    
>    static __attribute__((unused, format(printf, 1, 0)))
> 
> > Signed-off-by: David Laight <david.laight.linux@...il.com>
> > ---
> > 
> > Changes for v2:
> > Formally patch 2, unchanged.
> > 
> >  tools/include/nolibc/stdio.h | 32 +++++++++++++++++++++++++++++---
> >  1 file changed, 29 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/include/nolibc/stdio.h b/tools/include/nolibc/stdio.h
> > index 36733ecd4261..552f09d51d82 100644
> > --- a/tools/include/nolibc/stdio.h
> > +++ b/tools/include/nolibc/stdio.h
> > @@ -382,15 +382,41 @@ int __nolibc_printf(__nolibc_printf_cb cb, void *state, const char *fmt, va_list
> >  	return written;
> >  }
> >  
> > -static int __nolibc_fprintf_cb(void *stream, const char *buf, size_t size)
> > +struct __nolibc_fprintf_cb_state {
> > +	FILE *stream;
> > +	unsigned int buf_offset;
> > +	char buf[128];
> > +};  
> 
> So that's the other state I was wondering if we could merge with the first
> one.

The snprintf state is the 'address to write the next character to'
and 'the amount of space remaining'.
No real overlap at all.

> 
> > +static int __nolibc_fprintf_cb(void *v_state, const char *buf, size_t size)
> >  {
> > -	return size ? _fwrite(buf, size, stream) : 0;
> > +	struct __nolibc_fprintf_cb_state *state = v_state;
> > +	unsigned int off = state->buf_offset;
> > +
> > +	if (off + size > sizeof(state->buf) || buf == NULL) {  
> 
> Please mention that special case of buf==NULL in a comment above the
> function. That's an internal API choice.

ok - it might just go above this line though.

	David

> 
> > +		state->buf_offset = 0;
> > +		if (off && _fwrite(state->buf, off, state->stream))
> > +			return -1;
> > +		if (size > sizeof(state->buf))
> > +			return _fwrite(buf, size, state->stream);
> > +		off = 0;
> > +	}
> > +
> > +	if (size) {
> > +		state->buf_offset = off + size;
> > +		memcpy(state->buf + off, buf, size);
> > +	}
> > +	return 0;
> >  }
> >  
> >  static __attribute__((unused, format(printf, 2, 0)))
> >  int vfprintf(FILE *stream, const char *fmt, va_list args)
> >  {
> > -	return __nolibc_printf(__nolibc_fprintf_cb, stream, fmt, args);
> > +	struct __nolibc_fprintf_cb_state state;
> > +
> > +	state.stream = stream;
> > +	state.buf_offset = 0;
> > +	return __nolibc_printf(__nolibc_fprintf_cb, &state, fmt, args);
> >  }
> >  
> >  static __attribute__((unused, format(printf, 1, 0)))  
> 
> Thanks,
> Willy