Re: [PATCH v3 next 07/17] tools/nolibc/printf: Move snprintf length check to callback

[Thomas Weißschuh]

On 2026-02-25 23:12:21+0000, David Laight wrote:
> On Wed, 25 Feb 2026 23:37:42 +0100
> Thomas Weißschuh <linux@xxxxxxxxxxxxxx> wrote:
> 
> > On 2026-02-23 10:17:25+0000, david.laight.linux@xxxxxxxxx wrote:
> > 
> > (...)
> > 
> > > @@ -425,18 +430,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_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';
> > > +	state->space = space - size;
> > > +	state->buf = tgt + size;
> > > +	memcpy(tgt, buf, size);  
> > 
> > This trips UBSAN for me when 'buf == NULL'.
> > 
> > if (cb(state, NULL, 0) != 0)
> > 	return -1;
> > 
> > It can be fixed by adding a NULL check around memcpy(),
> > but I'd rather not do this as a random fixup.
> 
> Blame Willy, he made me remove the 'if (size)' check to reduce
> the code size.

Done. But we still can't hard-break our own testsuite.
If we can detect UBSAN at build-time, that could work.
But I would prefer to just add the check.

> The '*tgt = 0' line is (only) needed when size is zero, the other lines
> are clearly pointless.
> So the 'random fixup' is adding 'if (size)' rather than a NULL
> pointer check
> printf("%s", "") will give a zero size with non-NULL buf.

Can you roll this into the next revision?

> IIRC the C standard make memcpy(tgt, NULL, 0) UB because some old system
> no one has used for 40+ years would trap when NULL was loaded into an
> 'address register' and they wanted it to be compliant.

Fair enough. But how would this work for functions where NULL is an
allowed argument value like 'free()'? Anyways, we use UBSAN in the
testsuite and it actually found a bunch of issues, so I'd like to keep
it.

> > >  
> > > -	memcpy(*state, buf, size);
> > > -	*state += size;
> > >  	return 0;
> > >  }  
> > 
> > (...)