Re: [RFC v1 1/3] vsprintf: Add [v]seprintf(), [v]stprintf()

[Alejandro Colomar]

Hi Alexander,

On Mon, Jul 07, 2025 at 11:47:43AM +0200, Alexander Potapenko wrote:
> > +/**
> > + * vseprintf - Format a string and place it in a buffer
> > + * @p: The buffer to place the result into
> > + * @end: A pointer to one past the last character in the buffer
> > + * @fmt: The format string to use
> > + * @args: Arguments for the format string
> > + *
> > + * The return value is a pointer to the trailing '\0'.
> > + * If @p is NULL, the function returns NULL.
> > + * If the string is truncated, the function returns NULL.
> > + *
> > + * If you're not already dealing with a va_list consider using seprintf().
> > + *
> > + * See the vsnprintf() documentation for format string extensions over C99.
> > + */
> > +char *vseprintf(char *p, const char end[0], const char *fmt, va_list args)
> > +{
> > +       int len;
> > +
> > +       if (unlikely(p == NULL))
> > +               return NULL;
> > +
> > +       len = vstprintf(p, end - p, fmt, args);
> 
> It's easy to imagine a situation in which `end` is calculated from the
> user input and may overflow.
> Maybe we can add a check for `end > p` to be on the safe side?

That would technically be already UB at the moment you hold the 'end'
pointer, so the verification should in theory happen much earlier.

However, if we've arrived here with an overflown 'end', the safety is in
vsnprintf(), which has

        /* Reject out-of-range values early.  Large positive sizes are
           used for unknown buffer sizes. */
        if (WARN_ON_ONCE(size > INT_MAX))
                return 0;

The sequence is:

-  vseprintf() calls vstprintf() where end-p => size.
-  vstprintf() calls vsnprintf() with size.
-  vsnprintf() would return 0, and the contents of the string are
   undefined, as we haven't written anything.  It's not even truncated.

Which, indeed, doesn't sound like a safety.  We've reported a successful
copy of 0 bytes, but we actually failed.

Which BTW is a reminder that this implementation of vsnprintf() seems
dangerous to me, and not conforming to the standard vsnprintf(3).

Maybe we should do the check in vstprintf() and report an error as
-E2BIG (which is later translated into NULL by vseprintf()).  This is
what sized_strscpy() does, so sounds reasonable.  I'll add this test.

Thanks!


Have a lovely day!
Alex

-- 
<https://www.alejandro-colomar.es/>

Attachment: signature.asc
Description: PGP signature