Re: [PATCH v4 next 02/23] tools/nolibc/printf: Move snprintf length check to callback

[Willy Tarreau]

On Mon, Mar 02, 2026 at 10:17:54AM +0000, david.laight.linux@xxxxxxxxx wrote:
> From: David Laight <david.laight.linux@xxxxxxxxx>
> 
> Move output truncation to the snprintf() callback.
> This simplifies the main code and fixes truncation of padded fields.
> 
> Add a zero length callback to 'finalise' the buffer rather than
> doing it in snprintf() itself.
> 
> Fixes e90ce42e81381 ("tools/nolibc: implement width padding in printf()")
> Signed-off-by: David Laight <david.laight.linux@xxxxxxxxx>

Acked-by: Willy Tarreau <w@xxxxxx>

Willy

> ---
> 
> For v4:
> - Avoid memcpy(ptr, NULL, 0)
> 
> For v3:
> - Patch 2 in v2, patch 1 in v1.
> - Note that it fixes e90ce42e81381.
> - Update comments (again).
> - Rename size => space in snprintf 'state.
> - Copy state->space to a local rather than relying on the compiler
>   doing CSE, changes the code slightly for x86 (but not the size).
> - Unconditionally write a '\0' to terminate the old data before
>   overwriting it with new data.
>   Saves a few bytes of object code.
> 
>  tools/include/nolibc/stdio.h | 94 +++++++++++++++++++++++++-----------
>  1 file changed, 67 insertions(+), 27 deletions(-)
> 
> diff --git a/tools/include/nolibc/stdio.h b/tools/include/nolibc/stdio.h
> index 77d7669cdb80..a4df72d9a2d3 100644
> --- a/tools/include/nolibc/stdio.h
> +++ b/tools/include/nolibc/stdio.h
> @@ -295,16 +295,25 @@ int fseek(FILE *stream, long offset, int whence)
>   *  - %[l*]{d,u,c,x,p}
>   *  - %s
>   *  - unknown modifiers are ignored.
> + *
> + * Called by vfprintf() and snprintf() to do the actual formatting.
> + * The callers provide a callback function to save the formatted data.
> + * The callback function is called multiple times:
> + *  - for each group of literal characters in the format string.
> + *  - for field padding.
> + *  - for each conversion specifier.
> + *  - with (NULL, 0) at the end of the __nolibc_printf.
> + * If the callback returns non-zero __nolibc_printf() immediately returns -1.
>   */
> -typedef int (*__nolibc_printf_cb)(intptr_t state, const char *buf, size_t size);
> +typedef int (*__nolibc_printf_cb)(void *state, const char *buf, size_t size);
>  
> -static __attribute__((unused, format(printf, 4, 0)))
> -int __nolibc_printf(__nolibc_printf_cb cb, intptr_t state, size_t n, const char *fmt, va_list args)
> +static __attribute__((unused, format(printf, 3, 0)))
> +int __nolibc_printf(__nolibc_printf_cb cb, void *state, const char *fmt, va_list args)
>  {
>  	char escape, lpref, ch;
>  	unsigned long long v;
>  	unsigned int written, width;
> -	size_t len, ofs, w;
> +	size_t len, ofs;
>  	char outbuf[21];
>  	const char *outstr;
>  
> @@ -406,17 +415,13 @@ int __nolibc_printf(__nolibc_printf_cb cb, intptr_t state, size_t n, const char
>  			outstr = fmt;
>  			len = ofs - 1;
>  		flush_str:
> -			if (n) {
> -				w = len < n ? len : n;
> -				n -= w;
> -				while (width-- > w) {
> -					if (cb(state, " ", 1) != 0)
> -						return -1;
> -					written += 1;
> -				}
> -				if (cb(state, outstr, w) != 0)
> +			while (width-- > len) {
> +				if (cb(state, " ", 1) != 0)
>  					return -1;
> +				written += 1;
>  			}
> +			if (cb(state, outstr, len) != 0)
> +				return -1;
>  
>  			written += len;
>  		do_escape:
> @@ -429,18 +434,25 @@ int __nolibc_printf(__nolibc_printf_cb cb, intptr_t state, size_t n, const char
>  
>  		/* literal char, just queue it */
>  	}
> +
> +	/* Request a final '\0' be added to the snprintf() output.
> +	 * This may be the only call of the cb() function.
> +	 */
> +	if (cb(state, NULL, 0) != 0)
> +		return -1;
> +
>  	return written;
>  }
>  
> -static int __nolibc_fprintf_cb(intptr_t state, const char *buf, size_t size)
> +static int __nolibc_fprintf_cb(void *stream, const char *buf, size_t size)
>  {
> -	return _fwrite(buf, size, (FILE *)state);
> +	return _fwrite(buf, size, stream);
>  }
>  
>  static __attribute__((unused, format(printf, 2, 0)))
>  int vfprintf(FILE *stream, const char *fmt, va_list args)
>  {
> -	return __nolibc_printf(__nolibc_fprintf_cb, (intptr_t)stream, SIZE_MAX, fmt, args);
> +	return __nolibc_printf(__nolibc_fprintf_cb, stream, fmt, args);
>  }
>  
>  static __attribute__((unused, format(printf, 1, 0)))
> @@ -498,26 +510,54 @@ int dprintf(int fd, const char *fmt, ...)
>  	return ret;
>  }
>  
> -static int __nolibc_sprintf_cb(intptr_t _state, const char *buf, size_t size)
> +struct __nolibc_sprintf_cb_state {
> +	char *buf;
> +	size_t space;
> +};
> +
> +static int __nolibc_sprintf_cb(void *v_state, const char *buf, size_t size)
>  {
> -	char **state = (char **)_state;
> +	struct __nolibc_sprintf_cb_state *state = v_state;
> +	size_t space = state->space;
> +	char *tgt;
> +
> +	/* Truncate the request to fit in the output buffer space.
> +	 * The last byte is reserved for the terminating '\0'.
> +	 * state->space can only be zero for snprintf(NULL, 0, fmt, args)
> +	 * so this normally lets through calls with 'size == 0'.
> +	 */
> +	if (size >= space) {
> +		if (space <= 1)
> +			return 0;
> +		size = space - 1;
> +	}
> +	tgt = state->buf;
> +
> +	/* __nolibc_printf() ends with cb(state, NULL, 0) to request the output
> +	 * buffer be '\0' terminated.
> +	 * That will be the only cb() call for, eg, snprintf(buf, sz, "").
> +	 * Zero lengths can occur at other times (eg "%s" for an empty string).
> +	 * Unconditionally write the '\0' byte to reduce code size, it is
> +	 * normally overwritten by the data being output.
> +	 * There is no point adding a '\0' after copied data - there is always
> +	 * another call.
> +	 */
> +	*tgt = '\0';
> +	if (size) {
> +		state->space = space - size;
> +		state->buf = tgt + size;
> +		memcpy(tgt, buf, size);
> +	}
>  
> -	memcpy(*state, buf, size);
> -	*state += size;
>  	return 0;
>  }
>  
>  static __attribute__((unused, format(printf, 3, 0)))
>  int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
>  {
> -	char *state = buf;
> -	int ret;
> +	struct __nolibc_sprintf_cb_state state = { .buf = buf, .space = size };
>  
> -	ret = __nolibc_printf(__nolibc_sprintf_cb, (intptr_t)&state, size, fmt, args);
> -	if (ret < 0)
> -		return ret;
> -	buf[(size_t)ret < size ? (size_t)ret : size - 1] = '\0';
> -	return ret;
> +	return __nolibc_printf(__nolibc_sprintf_cb, &state, fmt, args);
>  }
>  
>  static __attribute__((unused, format(printf, 3, 4)))
> -- 
> 2.39.5