Hi,
more is broken in printf(3).
Ingo Schwarze wrote on Fri, Dec 25, 2015 at 12:30:29AM +0100:
> Fourth file, fourth broken file.
> This is the worst bug found so far.
[...]
> When fprintf(fp, "...%ls...", ...) encounters an encoding error,
> it trashes fp BEYOND REPAIR, clearing all FILE flags, making the
> file unreadable, unwriteable, and probably breaking even more than
> that. Calling clearerr(3) is by far insufficient to undo the
> devastating damage done.
In addition to that, the format string is parsed with mbrtowc(3),
even if PRINTF_WIDE_CHAR is *not* defined, and the failure is *not*
detected. That is, invalid bytes in the format string (for example,
0xff in an UTF-8 locale or ISO-Latin bytes in the C locale) are
silently treated as the end of the format string, without reporting
an error, only clobbering errno.
In addition to that, __find_arguments() also returns successfully
in this case, and its return value - which can already be -1 on
malloc failure - is never checked. This can result in access to
invalid memory.
Fix this by always checking the mbrtowc(3) return value against -1,
by failing at once as soon as that happens, and by always checking
__find_arguments() for success.
The diff below includes my previous diff, both are indeed related.
The new chunks are marked with ***NEW***.
OK?
Ingo
Index: stdio/vfprintf.c
===================================================================
RCS file: /cvs/src/lib/libc/stdio/vfprintf.c,v
retrieving revision 1.69
diff -u -p -r1.69 vfprintf.c
--- stdio/vfprintf.c 29 Sep 2015 03:19:24 -0000 1.69
+++ stdio/vfprintf.c 25 Dec 2015 13:33:57 -0000
@@ -165,10 +165,8 @@ __wcsconv(wchar_t *wcsarg, int prec)
memset(&mbs, 0, sizeof(mbs));
p = wcsarg;
nbytes = wcsrtombs(NULL, (const wchar_t **)&p, 0, &mbs);
- if (nbytes == (size_t)-1) {
- errno = EILSEQ;
+ if (nbytes == (size_t)-1)
return (NULL);
- }
} else {
/*
* Optimisation: if the output precision is small enough,
@@ -188,10 +186,8 @@ __wcsconv(wchar_t *wcsarg, int prec)
break;
nbytes += clen;
}
- if (clen == (size_t)-1) {
- errno = EILSEQ;
+ if (clen == (size_t)-1)
return (NULL);
- }
}
}
if ((convbuf = malloc(nbytes + 1)) == NULL)
@@ -203,7 +199,6 @@ __wcsconv(wchar_t *wcsarg, int prec)
if ((nbytes = wcsrtombs(convbuf, (const wchar_t **)&p,
nbytes, &mbs)) == (size_t)-1) {
free(convbuf);
- errno = EILSEQ;
return (NULL);
}
convbuf[nbytes] = '\0';
@@ -438,7 +433,11 @@ __vfprintf(FILE *fp, const char *fmt0, *** NEW ***
int hold = nextarg; \
if (argtable == NULL) { \
argtable = statargtable; \
- __find_arguments(fmt0, orgap, &argtable, &argtablesiz);
\
+ if (__find_arguments(fmt0, orgap, &argtable, \
+ &argtablesiz) == -1) { \
+ ret = -1; \
+ goto error; \
+ } \
} \
nextarg = n2; \
val = GETARG(int); \
@@ -494,6 +493,10 @@ __vfprintf(FILE *fp, const char *fmt0, *** NEW ***
break;
}
}
+ if (n < 0) {
+ ret = -1;
+ goto error;
+ }
if (fmt != cp) {
ptrdiff_t m = fmt - cp;
if (m < 0 || m > INT_MAX - ret)
@@ -501,7 +504,7 @@ __vfprintf(FILE *fp, const char *fmt0, *** NEW ***
PRINT(cp, m);
ret += m;
}
- if (n <= 0)
+ if (n == 0)
goto done;
fmt++; /* skip over '%' */
@@ -564,8 +567,11 @@ reswitch: switch (ch) { *** NEW ***
nextarg = n;
if (argtable == NULL) {
argtable = statargtable;
- __find_arguments(fmt0, orgap,
- &argtable, &argtablesiz);
+ if (__find_arguments(fmt0, orgap,
+ &argtable, &argtablesiz) == -1) {
+ ret = -1;
+ goto error;
+ }
}
goto rflag;
}
@@ -590,8 +596,11 @@ reswitch: switch (ch) { *** NEW ***
nextarg = n;
if (argtable == NULL) {
argtable = statargtable;
- __find_arguments(fmt0, orgap,
- &argtable, &argtablesiz);
+ if (__find_arguments(fmt0, orgap,
+ &argtable, &argtablesiz) == -1) {
+ ret = -1;
+ goto error;
+ }
}
goto rflag;
}
@@ -640,8 +649,7 @@ reswitch: switch (ch) {
mbseqlen = wcrtomb(buf,
(wchar_t)GETARG(wint_t), &mbs);
if (mbseqlen == (size_t)-1) {
- fp->_flags |= __SERR;
- errno = EILSEQ;
+ ret = -1;
goto error;
}
cp = buf;
@@ -855,7 +863,7 @@ fp_common:
} else {
convbuf = __wcsconv(wcp, prec);
if (convbuf == NULL) {
- fp->_flags = __SERR;
+ ret = -1;
goto error;
}
cp = convbuf;
@@ -1214,7 +1222,9 @@ __find_arguments(const char *fmt0, *** NEW ***
break;
}
}
- if (n <= 0)
+ if (n < 0)
+ return (-1);
+ if (n == 0)
goto done;
fmt++; /* skip over '%' */
