[PATCH] fix fgetwc when decoding a character that crosses buffer boundary

[PATCH] fix fgetwc when decoding a character that crosses buffer boundary

[Szabolcs Nagy]

Update the buffer position according to the bytes consumed into st when
decoding an incomplete character at the end of the buffer.
---
 src/stdio/fgetwc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/stdio/fgetwc.c b/src/stdio/fgetwc.c
index e455cfec..a00c1a86 100644
--- a/src/stdio/fgetwc.c
+++ b/src/stdio/fgetwc.c
@@ -15,20 +15,21 @@ static wint_t __fgetwc_unlocked_internal(FILE *f)
 	if (f->rpos < f->rend) {
 		l = mbrtowc(&wc, (void *)f->rpos, f->rend - f->rpos, &st);
 		if (l+2 >= 2) {
 			f->rpos += l + !l; /* l==0 means 1 byte, null */
 			return wc;
 		}
 		if (l == -1) {
 			f->rpos++;
 			return WEOF;
 		}
+		f->rpos = f->rend;
 	} else l = -2;
 
 	/* Convert character byte-by-byte */
 	while (l == -2) {
 		b = c = getc_unlocked(f);
 		if (c < 0) {
 			if (!mbsinit(&st)) errno = EILSEQ;
 			return WEOF;
 		}
 		l = mbrtowc(&wc, (void *)&b, 1, &st);
-- 
2.15.0

Re: [PATCH] fix fgetwc when decoding a character that crosses buffer boundary

[Rich Felker]

[-- Attachment #1: Type: text/plain, Size: 1010 bytes --]

On Sat, Nov 18, 2017 at 05:51:48PM +0100, Szabolcs Nagy wrote:
> Update the buffer position according to the bytes consumed into st when
> decoding an incomplete character at the end of the buffer.
> ---
>  src/stdio/fgetwc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/stdio/fgetwc.c b/src/stdio/fgetwc.c
> index e455cfec..a00c1a86 100644
> --- a/src/stdio/fgetwc.c
> +++ b/src/stdio/fgetwc.c
> @@ -15,20 +15,21 @@ static wint_t __fgetwc_unlocked_internal(FILE *f)
>  	if (f->rpos < f->rend) {
>  		l = mbrtowc(&wc, (void *)f->rpos, f->rend - f->rpos, &st);
>  		if (l+2 >= 2) {
>  			f->rpos += l + !l; /* l==0 means 1 byte, null */
>  			return wc;
>  		}
>  		if (l == -1) {
>  			f->rpos++;
>  			return WEOF;
>  		}
> +		f->rpos = f->rend;
>  	} else l = -2;

Thanks, applying! Here is a test case that demonstrates the bug
reproducibly; feel free to adapt (it should probably use the framework
functions for getting a utf-8 locale and error reporting) & include it
in libc-test.

Rich

[-- Attachment #2: fgetwc2.c --]
[-- Type: text/plain, Size: 324 bytes --]

#include <stdio.h>
#include <poll.h>
#include <locale.h>
#include <wchar.h>
#include <unistd.h>

int main()
{
	setlocale(LC_CTYPE, "");
	int p[2];
	pipe(p);
	write(p[1], "x\340\240", 3);
	dup2(p[0], 0);
	wchar_t wc;
	wc = fgetwc(stdin);
	write(p[1], "\200", 1);
	close(p[1]);
	wc = fgetwc(stdin);
	printf("got %x\n", wc);
}

Re: [PATCH] fix fgetwc when decoding a character that crosses buffer boundary

[Rich Felker]

On Sat, Nov 18, 2017 at 08:12:57PM -0500, Rich Felker wrote:
> On Sat, Nov 18, 2017 at 05:51:48PM +0100, Szabolcs Nagy wrote:
> > Update the buffer position according to the bytes consumed into st when
> > decoding an incomplete character at the end of the buffer.
> > ---
> >  src/stdio/fgetwc.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/src/stdio/fgetwc.c b/src/stdio/fgetwc.c
> > index e455cfec..a00c1a86 100644
> > --- a/src/stdio/fgetwc.c
> > +++ b/src/stdio/fgetwc.c
> > @@ -15,20 +15,21 @@ static wint_t __fgetwc_unlocked_internal(FILE *f)
> >  	if (f->rpos < f->rend) {
> >  		l = mbrtowc(&wc, (void *)f->rpos, f->rend - f->rpos, &st);
> >  		if (l+2 >= 2) {
> >  			f->rpos += l + !l; /* l==0 means 1 byte, null */
> >  			return wc;
> >  		}
> >  		if (l == -1) {
> >  			f->rpos++;
> >  			return WEOF;
> >  		}
> > +		f->rpos = f->rend;
> >  	} else l = -2;
> 
> Thanks, applying! Here is a test case that demonstrates the bug
> reproducibly; feel free to adapt (it should probably use the framework
> functions for getting a utf-8 locale and error reporting) & include it
> in libc-test.
> 
> Rich

> #include <stdio.h>
> #include <poll.h>
           ^^^^^^^^

Ooops, this is spurious/leftover from when I thought I was going to
need to do something fancier to reliably trigger it.

> #include <locale.h>
> #include <wchar.h>
> #include <unistd.h>
> 
> int main()
> {
> 	setlocale(LC_CTYPE, "");
> 	int p[2];
> 	pipe(p);
> 	write(p[1], "x\340\240", 3);
> 	dup2(p[0], 0);
> 	wchar_t wc;
> 	wc = fgetwc(stdin);
> 	write(p[1], "\200", 1);
> 	close(p[1]);
> 	wc = fgetwc(stdin);
> 	printf("got %x\n", wc);
> }

Further wide stdio issues [Was: Re: [PATCH] fix fgetwc when decoding a character that crosses buffer boundary]

[Rich Felker]

On Sat, Nov 18, 2017 at 05:51:48PM +0100, Szabolcs Nagy wrote:
> Update the buffer position according to the bytes consumed into st when
> decoding an incomplete character at the end of the buffer.

Further related problems:

1. fgetws is unable to accurately determine whether fgetwc failed due
   to hitting EOF on a clean stream vs hitting EOF in the middle of a
   truncated character. The latter is an EILSEQ condition and should
   cause fgetws to return a null pointer rather than a line not ending
   in newline.

2. fgetwc attempts to leave all but the first byte unread on EILSEQ so
   that calling fgetwc again will resync, but doesn't do this
   consistently when it has to call getc_unlocked to get new bytes
   rather than reading from the buffer.

I think (1) can be fixed in an ugly way by setting errno to something
other than EILSEQ before calling fgetwc, then checking if EILSEQ upon
failure.

As for (2), it's not fixable without some pushback (ungetc). I'm not
sure if pushback here on an unbuffered stream is even conforming; if
not, no fix is possible, which is quite unfortunate because it makes
it impossible to resync after error. Assuming pushback is acceptable,
however, we'd still need up to MB_LEN_MAX-1 bytes of pushback to give
the same behavior as the buffered case now. It might be better to have
both the buffered and unbuffered cases consume as many bytes as yield
(size_t)-2 from mbrtowc (i.e. as many as would be a prefix of a valid
character) and only leave unread (pushed back) the first byte that
determines failure (which could be invalid itself, or the first byte
of the next valid character). This would require slightly more work in
the buffered case, but it could instead be handled by just falling
through to the slow unbuffered-case loop, with st reset to {0} and
without advancing the buffer position, when mbrtowc fails. In fact
then we could even use mbtowc instead of mbrtowc in the fast path,
making it slightly faster.

Thoughts?

Rich

Re: Further wide stdio issues [Was: Re: [PATCH] fix fgetwc when decoding a character that crosses buffer boundary]

[Rich Felker]

[-- Attachment #1: Type: text/plain, Size: 1066 bytes --]

On Sun, Nov 19, 2017 at 08:48:53PM -0500, Rich Felker wrote:
> On Sat, Nov 18, 2017 at 05:51:48PM +0100, Szabolcs Nagy wrote:
> > Update the buffer position according to the bytes consumed into st when
> > decoding an incomplete character at the end of the buffer.
> 
> Further related problems:
> 
> 1. fgetws is unable to accurately determine whether fgetwc failed due
>    to hitting EOF on a clean stream vs hitting EOF in the middle of a
>    truncated character. The latter is an EILSEQ condition and should
>    cause fgetws to return a null pointer rather than a line not ending
>    in newline.
> 
> 2. fgetwc attempts to leave all but the first byte unread on EILSEQ so
>    that calling fgetwc again will resync, but doesn't do this
>    consistently when it has to call getc_unlocked to get new bytes
>    rather than reading from the buffer.
> 
> I think (1) can be fixed in an ugly way by setting errno to something
> other than EILSEQ before calling fgetwc, then checking if EILSEQ upon
> failure.

Patching this; here's a test case (attached).

Rich

[-- Attachment #2: fgetws.c --]
[-- Type: text/plain, Size: 319 bytes --]

#include <stdio.h>
#include <poll.h>
#include <locale.h>
#include <wchar.h>
#include <unistd.h>

int main()
{
	setlocale(LC_CTYPE, "");
	int p[2];
	pipe(p);
	write(p[1], "x\340\240xxx", 3);
	close(p[1]);
	dup2(p[0], 0);
	wchar_t buf[100] = {0};
	wchar_t *ws = fgetws(buf, 100, stdin);
	printf("%p [%ls]\n", ws, buf);
}