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

[David Laight <david.laight.linux@gmail.com>]

On Thu, 26 Feb 2026 22:29:07 +0100
Thomas Weißschuh <linux@weissschuh.net> wrote:

> On 2026-02-25 23:12:21+0000, David Laight wrote:
> > On Wed, 25 Feb 2026 23:37:42 +0100
> > Thomas Weißschuh <linux@weissschuh.net> wrote:
> >   
> > > On 2026-02-23 10:17:25+0000, david.laight.linux@gmail.com 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?

I'll add the zero check back.
Probably get around to it tomorrow.
At least I don't have to completely re-order the patches again.
I've got about 15 old branches from failed attempts lurking.

	David

> 
> > 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;
> > > >  }    
> > > 
> > > (...)