Re: [musl] swprintf: count returned by %n is wrong after conversion error

[Rich Felker <dalias@libc.org>]

On Mon, Mar 20, 2023 at 08:15:59AM -0400, Rich Felker wrote:
> On Mon, Mar 20, 2023 at 12:48:59AM +0100, Bruno Haible wrote:
> > Hi,
> > 
> > On musl-1.2.3 I see this violation of the POSIX specification of swprintf [1]:
> > 
> > ==================================== foo1.c ====================================
> > #include <stdio.h>
> > #include <wchar.h>
> > 
> > int main ()
> > {
> >   static const wchar_t input[] = { (wchar_t) 1702057263, 114, 0 };
> >   wchar_t buf[12] = { 0xDEADBEEF, 0xDEADBEEF, 0xDEADBEEF, 0xDEADBEEF };
> >   int count = -1;
> >   int ret = swprintf (buf, 12, L"%ls%n", input, &count);
> >   printf ("ret = %d, count = %d, buf[0] = 0x%x, buf[1] = 0x%x, buf[2] = 0x%x\n",
> >           ret, count,
> >           (unsigned int) buf[0], (unsigned int) buf[1], (unsigned int) buf[2]);
> >   return 0;
> > }
> > /*
> > glibc:      ret = 2, count = 2, buf[0] = 0x6573552f, buf[1] = 0x72, buf[2] = 0x0
> > musl libc:  ret = -1, count = 2, buf[0] = 0x0, buf[1] = 0xdeadbeef, buf[2] = 0xdeadbeef
> > FreeBSD 13: ret = -1, count = -1, buf[0] = 0x0, buf[1] = 0xdeadbeef, buf[2] = 0xdeadbeef
> > Solaris OI: ret = 2, count = 2, buf[0] = 0x6573552f, buf[1] = 0x72, buf[2] = 0x0
> > */
> > ================================================================================
> > 
> > $ gcc -Wall foo1.c
> > $ ./a.out
> > ret = -1, count = 2, buf[0] = 0x0, buf[1] = 0xdeadbeef, buf[2] = 0xdeadbeef
> > 
> > The POSIX specification says:
> >   "The application shall ensure that the argument is a pointer to an integer
> >    into which is written the number of wide characters written to the output
> >    so far by this call to one of the fwprintf() functions."
> > 
> > From the values of buf[0], buf[1], buf[2] it can be seen that the number
> > of wide characters written after the %ls directive is 0, not 2. Therefore
> > the value of count should be 0 or — if the processing of the format string
> > stops right after the %ls directive, like it does on FreeBSD 13 — -1.
> > 
> > It is OK for the %ls directive to fail, because of the invalid wide characters
> > in the input[] arrary. What is not OK is for the %n directive to report 2
> > written wide characters, when in fact 0 wide characters have been written.
> 
> Thanks. It looks like the wide printf core is just missing any
> validation logic for the wchar_t[] array. Probably it was assumed
> writing it that any wchar_t would be accepted so that no error
> handling would be needed, but the backend doesn't actually accept
> them.
> 
> I'm not sure what the best fix is, but the simplest one is to iterate
> and validate the entire range to be printed before printing anything
> and error out on EILSEQ. But I'm not sure if this is actually
> conforming either.
> 
> Since fwprintf is supposed to behave as if by repeated fputwc, and the
> allowance (rather mandate) for EILSEQ comes from fputwc, the error
> should only happen at the point the invalid character is written, with
> all output up to that point being visible. So I guess we should drop
> use of the out() helper function here and call fputc explicitly in a
> loop where we can process the error. And in fact out() is only used
> two places (also for the format string, where not being a valid wchar
> string is UB so it doesn't really matter) so maybe we should just drop
> the helper altogether and open-code it both places with proper error
> handling...

OK, it looks like we have some high-level wrong error handling logic
in vfwprintf.c. The top-level vfwprintf function saves and clears the
error flag for the stream, then tests it at the end to determine
whether it needs to replace the return value with -1. And the out()
helper function tests the flag to avoid producing further output when
an error has already been hit, but this neither prevents the side
effects of processing %n, nor does it prevent writes via pass thru to
fprintf for numeric formats, or writes in the %s case.

I think the logic to check the error flag should just be moved into
the core function to check it before processing each format specifier,
and bail out of it's set.

There is probably also a similar issue in non-wide printf, but only
for write errors, not for encoding errors, and only visibly wrong if
the write error is transient (e.g. EAGAIN).

Rich