Re: fprintf() crashes on wide-oriented stream.

[Takashi Yano <takashi.yano@nifty.ne.jp>]

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

On Thu, 5 Oct 2023 19:18:14 +0900
Takashi Yano wrote:
> Hi Jeff,
> 
> Thanks for reviewing and the comment.
> 
> On Wed, 4 Oct 2023 16:16:13 -0400
> Jeff Johnston wrote:
> > I finally took a look at this.  The issue is whether POSIX compliance is
> > desired.
> 
> IIUC, POSIX states that width setting is once decided, it cannot be
> changed until the stream is closed. However, nothing is stated what
> should happen when different width data is output into the stream.
> 
> > Corinna would have
> > strong opinions that it is desired and thus, I think she should have her
> > say when she gets back.  I personally believe that
> > newlib should have behaved like glibc.  I also think the test snippet is
> > invalid and should have performed an fwide call on stdout
> > to reset the wide-orientation and have the code work properly in all cases.
> 
> Currently, fputs and fputc works even for wide-oriended stream, so to
> be consistent with that, fprintf also might be better to work.
> 
> I wouldn't necessarily expect fprintf to work on wide-oriented streams,
> but buffer overruns should not happen anyway.
> 
> So, newlib should be fixed either way.

As a test, I made a patch attached to make it behave like glibc.
What do you think?

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

[-- Attachment #2: v3-0001-newlib-libc-Fix-crash-on-fprintf-to-a-wide-orient.patch --]
[-- Type: text/plain, Size: 11655 bytes --]

From 043b6981f748f332224bd3cb23ac8f1868f60f52 Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.yano@nifty.ne.jp>
Date: Fri, 6 Oct 2023 00:04:49 +0900
Subject: [PATCH v3] newlib: libc: Fix crash on fprintf to a wide-oriented
 stream.

Previously, fprintf() on a wide-oriented stream crashes or outputs
garbage. This is because a narrow char string which can be odd bytes
in length is cast into a wide char string which should be even
bytes in length in __sprint_r/__sfputs_r based on the __SWID flag.
As a result, if the length is odd bytes, the reading buffer runs over
the buffer length, which causes a crash. If the length is even bytes,
garbage is printed.

With this patch, any output to the stream which is set to different
orientation fails with error just like glibc. Note that it behaves
differently from other libc implementations such as BSD, musl and
Solaris.

Reviewed-by:
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
---
 newlib/libc/stdio/fgetwc.c    | 5 ++++-
 newlib/libc/stdio/fgetwc_u.c  | 2 ++
 newlib/libc/stdio/fgetws.c    | 2 ++
 newlib/libc/stdio/fputs.c     | 7 ++++++-
 newlib/libc/stdio/fputwc.c    | 5 ++++-
 newlib/libc/stdio/fputwc_u.c  | 2 ++
 newlib/libc/stdio/fputws.c    | 4 ++++
 newlib/libc/stdio/fread.c     | 6 ++++++
 newlib/libc/stdio/fwrite.c    | 7 +++++++
 newlib/libc/stdio/local.h     | 2 ++
 newlib/libc/stdio/putc.c      | 6 ++++++
 newlib/libc/stdio/puts.c      | 7 ++++++-
 newlib/libc/stdio/refill.c    | 2 ++
 newlib/libc/stdio/ungetc.c    | 5 +++++
 newlib/libc/stdio/ungetwc.c   | 4 +++-
 newlib/libc/stdio/vfprintf.c  | 4 ++++
 newlib/libc/stdio/vfscanf.c   | 5 +++++
 newlib/libc/stdio/vfwprintf.c | 4 ++++
 newlib/libc/stdio/vfwscanf.c  | 5 +++++
 19 files changed, 79 insertions(+), 5 deletions(-)

diff --git a/newlib/libc/stdio/fgetwc.c b/newlib/libc/stdio/fgetwc.c
index 522697e9b..ce4f007d8 100644
--- a/newlib/libc/stdio/fgetwc.c
+++ b/newlib/libc/stdio/fgetwc.c
@@ -178,7 +178,10 @@ _fgetwc_r (struct _reent *ptr,
 
   _newlib_flockfile_start (fp);
   ORIENT(fp, 1);
-  r = __fgetwc (ptr, fp);
+  if (QUERY_ORIENT(fp) != 1)
+    r = WEOF;
+  else
+    r = __fgetwc (ptr, fp);
   _newlib_flockfile_end (fp);
   return r;
 }
diff --git a/newlib/libc/stdio/fgetwc_u.c b/newlib/libc/stdio/fgetwc_u.c
index cbb97360b..a0ffbcba1 100644
--- a/newlib/libc/stdio/fgetwc_u.c
+++ b/newlib/libc/stdio/fgetwc_u.c
@@ -34,6 +34,8 @@ _fgetwc_unlocked_r (struct _reent *ptr,
 	register FILE *fp)
 {
   ORIENT(fp, 1);
+  if (QUERY_ORIENT(fp) != 1)
+    return WEOF;
   return __fgetwc (ptr, fp);
 }
 
diff --git a/newlib/libc/stdio/fgetws.c b/newlib/libc/stdio/fgetws.c
index c942806c6..9b1304fdc 100644
--- a/newlib/libc/stdio/fgetws.c
+++ b/newlib/libc/stdio/fgetws.c
@@ -111,6 +111,8 @@ _fgetws_r (struct _reent *ptr,
 
   _newlib_flockfile_start (fp);
   ORIENT (fp, 1);
+  if (QUERY_ORIENT (fp) != 1)
+    goto error;
 
   if (n <= 0)
     {
diff --git a/newlib/libc/stdio/fputs.c b/newlib/libc/stdio/fputs.c
index 4e9cb7547..8f1639076 100644
--- a/newlib/libc/stdio/fputs.c
+++ b/newlib/libc/stdio/fputs.c
@@ -104,7 +104,10 @@ _fputs_r (struct _reent * ptr,
 
   _newlib_flockfile_start (fp);
   ORIENT (fp, -1);
-  result = __sfvwrite_r (ptr, fp, &uio);
+  if (QUERY_ORIENT (fp) != -1)
+    result = EOF;
+  else
+    result = __sfvwrite_r (ptr, fp, &uio);
   _newlib_flockfile_end (fp);
   return result;
 #else
@@ -114,6 +117,8 @@ _fputs_r (struct _reent * ptr,
 
   _newlib_flockfile_start (fp);
   ORIENT (fp, -1);
+  if (QUERY_ORIENT (fp) != -1)
+    goto error;
   /* Make sure we can write.  */
   if (cantwrite (ptr, fp))
     goto error;
diff --git a/newlib/libc/stdio/fputwc.c b/newlib/libc/stdio/fputwc.c
index 9a4e80d52..0242dca77 100644
--- a/newlib/libc/stdio/fputwc.c
+++ b/newlib/libc/stdio/fputwc.c
@@ -170,7 +170,10 @@ _fputwc_r (struct _reent *ptr,
 
   _newlib_flockfile_start (fp);
   ORIENT(fp, 1);
-  r = __fputwc(ptr, wc, fp);
+  if (QUERY_ORIENT(fp) != 1)
+    r = WEOF;
+  else
+    r = __fputwc(ptr, wc, fp);
   _newlib_flockfile_end (fp);
   return r;
 }
diff --git a/newlib/libc/stdio/fputwc_u.c b/newlib/libc/stdio/fputwc_u.c
index d4e51532a..79136c3d8 100644
--- a/newlib/libc/stdio/fputwc_u.c
+++ b/newlib/libc/stdio/fputwc_u.c
@@ -35,6 +35,8 @@ _fputwc_unlocked_r (struct _reent *ptr,
 	FILE *fp)
 {
   ORIENT(fp, 1);
+  if (QUERY_ORIENT(fp) != 1)
+    return WEOF;
   return __fputwc(ptr, wc, fp);
 }
 
diff --git a/newlib/libc/stdio/fputws.c b/newlib/libc/stdio/fputws.c
index 92f2cbf6a..57e93193e 100644
--- a/newlib/libc/stdio/fputws.c
+++ b/newlib/libc/stdio/fputws.c
@@ -106,6 +106,8 @@ _fputws_r (struct _reent *ptr,
 
   _newlib_flockfile_start (fp);
   ORIENT (fp, 1);
+  if (QUERY_ORIENT (fp) != 1)
+    goto error;
   if (cantwrite (ptr, fp) != 0)
     goto error;
   uio.uio_iov = &iov;
@@ -130,6 +132,8 @@ error:
 #else
   _newlib_flockfile_start (fp);
   ORIENT (fp, 1);
+  if (QUERY_ORIENT (fp) != 1)
+    goto error;
   if (cantwrite (ptr, fp) != 0)
     goto error;
 
diff --git a/newlib/libc/stdio/fread.c b/newlib/libc/stdio/fread.c
index df8321461..dbb13130f 100644
--- a/newlib/libc/stdio/fread.c
+++ b/newlib/libc/stdio/fread.c
@@ -159,6 +159,11 @@ _fread_r (struct _reent * ptr,
 
   _newlib_flockfile_start (fp);
   ORIENT (fp, -1);
+  if (QUERY_ORIENT (fp) != -1)
+    {
+      count = 0;
+      goto ret;
+    }
   if (fp->_r < 0)
     fp->_r = 0;
   total = resid;
@@ -252,6 +257,7 @@ _fread_r (struct _reent * ptr,
       return crlf_r(ptr, fp, buf, total, 0) / size;
     }
 #endif
+ret:
   _newlib_flockfile_end (fp);
   return count;
 }
diff --git a/newlib/libc/stdio/fwrite.c b/newlib/libc/stdio/fwrite.c
index d499f6f59..4f0c2982c 100644
--- a/newlib/libc/stdio/fwrite.c
+++ b/newlib/libc/stdio/fwrite.c
@@ -134,6 +134,11 @@ _fwrite_r (struct _reent * ptr,
 
   _newlib_flockfile_start (fp);
   ORIENT (fp, -1);
+  if (QUERY_ORIENT (fp) != -1)
+    {
+      _newlib_flockfile_exit (fp);
+      return 0;
+    }
   if (__sfvwrite_r (ptr, fp, &uio) == 0)
     {
       _newlib_flockfile_exit (fp);
@@ -149,6 +154,8 @@ _fwrite_r (struct _reent * ptr,
 
   _newlib_flockfile_start (fp);
   ORIENT (fp, -1);
+  if (QUERY_ORIENT (fp) != -1)
+    goto ret;
   /* Make sure we can write.  */
   if (cantwrite (ptr, fp))
     goto ret;
diff --git a/newlib/libc/stdio/local.h b/newlib/libc/stdio/local.h
index b34c7c9d8..e3e7d7f09 100644
--- a/newlib/libc/stdio/local.h
+++ b/newlib/libc/stdio/local.h
@@ -244,8 +244,10 @@ extern _READ_WRITE_RETURN_TYPE __swrite64 (struct _reent *, void *,
 	}							\
     }								\
   while (0)
+#define QUERY_ORIENT(fp) (((fp)->_flags2 & __SWID) ? 1 : -1)
 #else
 #define ORIENT(fp,ori)
+#define QUERY_ORIENT(fp) (-1)
 #endif
 
 /* WARNING: _dcvt is defined in the stdlib directory, not here!  */
diff --git a/newlib/libc/stdio/putc.c b/newlib/libc/stdio/putc.c
index 6a410e216..dbc633d10 100644
--- a/newlib/libc/stdio/putc.c
+++ b/newlib/libc/stdio/putc.c
@@ -84,6 +84,9 @@ _putc_r (struct _reent *ptr,
 {
   int result;
   CHECK_INIT (ptr, fp);
+  ORIENT(fp, -1);
+  if (QUERY_ORIENT(fp) != -1)
+    return EOF;
   _newlib_flockfile_start (fp);
   result = __sputc_r (ptr, c, fp);
   _newlib_flockfile_end (fp);
@@ -100,6 +103,9 @@ putc (int c,
   struct _reent *reent = _REENT;
 
   CHECK_INIT (reent, fp);
+  ORIENT(fp, -1);
+  if (QUERY_ORIENT(fp) != -1)
+    return EOF;
   _newlib_flockfile_start (fp);
   result = __sputc_r (reent, c, fp);
   _newlib_flockfile_end (fp);
diff --git a/newlib/libc/stdio/puts.c b/newlib/libc/stdio/puts.c
index d4d7b7ffb..03d231499 100644
--- a/newlib/libc/stdio/puts.c
+++ b/newlib/libc/stdio/puts.c
@@ -88,7 +88,10 @@ _puts_r (struct _reent *ptr,
   CHECK_INIT (ptr, fp);
   _newlib_flockfile_start (fp);
   ORIENT (fp, -1);
-  result = (__sfvwrite_r (ptr, fp, &uio) ? EOF : '\n');
+  if (QUERY_ORIENT (fp) != -1)
+    result = EOF;
+  else
+    result = (__sfvwrite_r (ptr, fp, &uio) ? EOF : '\n');
   _newlib_flockfile_end (fp);
   return result;
 #else
@@ -101,6 +104,8 @@ _puts_r (struct _reent *ptr,
   CHECK_INIT (ptr, fp);
   _newlib_flockfile_start (fp);
   ORIENT (fp, -1);
+  if (QUERY_ORIENT (fp) != -1)
+    goto err;
   /* Make sure we can write.  */
   if (cantwrite (ptr, fp))
     goto err;
diff --git a/newlib/libc/stdio/refill.c b/newlib/libc/stdio/refill.c
index 7bd38806c..5d9fb198c 100644
--- a/newlib/libc/stdio/refill.c
+++ b/newlib/libc/stdio/refill.c
@@ -44,6 +44,8 @@ __srefill_r (struct _reent * ptr,
   CHECK_INIT (ptr, fp);
 
   ORIENT (fp, -1);
+  if (QUERY_ORIENT (fp) != -1)
+    return EOF;
 
   fp->_r = 0;			/* largely a convenience for callers */
 
diff --git a/newlib/libc/stdio/ungetc.c b/newlib/libc/stdio/ungetc.c
index 533c28eac..ae859b4f8 100644
--- a/newlib/libc/stdio/ungetc.c
+++ b/newlib/libc/stdio/ungetc.c
@@ -126,6 +126,11 @@ _ungetc_r (struct _reent *rptr,
   _newlib_flockfile_start (fp);
 
   ORIENT (fp, -1);
+  if (QUERY_ORIENT (fp) != -1)
+    {
+      _newlib_flockfile_exit (fp);
+      return EOF;
+    }
 
   /* After ungetc, we won't be at eof anymore */
   fp->_flags &= ~__SEOF;
diff --git a/newlib/libc/stdio/ungetwc.c b/newlib/libc/stdio/ungetwc.c
index 16d37f2d1..c9437c0fa 100644
--- a/newlib/libc/stdio/ungetwc.c
+++ b/newlib/libc/stdio/ungetwc.c
@@ -83,7 +83,9 @@ _ungetwc_r (struct _reent *ptr,
 
   _newlib_flockfile_start (fp);
   ORIENT (fp, 1);
-  if (wc == WEOF)
+  if (QUERY_ORIENT (fp) != 1)
+    wc = WEOF;
+  else if (wc == WEOF)
     wc = WEOF;
   else if ((len = _wcrtomb_r(ptr, buf, wc, &fp->_mbstate)) == (size_t)-1)
     {
diff --git a/newlib/libc/stdio/vfprintf.c b/newlib/libc/stdio/vfprintf.c
index 6a198e2c6..4c13dee51 100644
--- a/newlib/libc/stdio/vfprintf.c
+++ b/newlib/libc/stdio/vfprintf.c
@@ -846,6 +846,10 @@ _VFPRINTF_R (struct _reent *data,
 	_newlib_flockfile_start (fp);
 
 	ORIENT(fp, -1);
+	if (QUERY_ORIENT(fp) != -1) {
+		_newlib_flockfile_exit (fp);
+		return (EOF);
+	}
 
 	/* sorry, fprintf(read_only_file, "") returns EOF, not 0 */
 	if (cantwrite (data, fp)) {
diff --git a/newlib/libc/stdio/vfscanf.c b/newlib/libc/stdio/vfscanf.c
index cfeea9876..673ed8b42 100644
--- a/newlib/libc/stdio/vfscanf.c
+++ b/newlib/libc/stdio/vfscanf.c
@@ -590,6 +590,11 @@ __SVFSCANF_R (struct _reent *rptr,
   _newlib_flockfile_start (fp);
 
   ORIENT (fp, -1);
+  if (QUERY_ORIENT (fp) != -1)
+    {
+      nassigned = EOF;
+      goto all_done;
+    }
 
   nassigned = 0;
   nread = 0;
diff --git a/newlib/libc/stdio/vfwprintf.c b/newlib/libc/stdio/vfwprintf.c
index 7807a1229..0bb147d1d 100644
--- a/newlib/libc/stdio/vfwprintf.c
+++ b/newlib/libc/stdio/vfwprintf.c
@@ -589,6 +589,10 @@ _VFWPRINTF_R (struct _reent *data,
 	_newlib_flockfile_start (fp);
 
 	ORIENT(fp, 1);
+	if (QUERY_ORIENT(fp) != 1) {
+		_newlib_flockfile_exit (fp);
+		return (EOF);
+	}
 
 	/* sorry, fwprintf(read_only_file, "") returns EOF, not 0 */
 	if (cantwrite (data, fp)) {
diff --git a/newlib/libc/stdio/vfwscanf.c b/newlib/libc/stdio/vfwscanf.c
index df966f929..4356694a6 100644
--- a/newlib/libc/stdio/vfwscanf.c
+++ b/newlib/libc/stdio/vfwscanf.c
@@ -517,6 +517,11 @@ __SVFWSCANF_R (struct _reent *rptr,
   _newlib_flockfile_start (fp);
 
   ORIENT (fp, 1);
+  if (QUERY_ORIENT (fp) != 1)
+    {
+      nassigned = EOF;
+      goto all_done;
+    }
 
   nassigned = 0;
   nread = 0;
-- 
2.39.0