fprintf() crashes on wide-oriented stream.

fprintf() crashes on wide-oriented stream.

[Takashi Yano]

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

Hi,

I noticed that the following test case crashes at printf() with current
newlib.


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

int main()
{
	setlocale(LC_ALL, "C.UTF-8");
	wprintf(L"%ls\n", L"aaaa"); /* or fwide(stdout, 1); */
	printf("%ls\n", L"bbbb");  /* <--- crash here */
	return 0;
}


I looked into this problem and found the cause.

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, crash does not happen, but garbage
is printed. This hapens if printf("%ls\r\n", L"bbbb"); is used instead.
                                      ^^

The same issue seemed to be reported ten years ago.
https://sourceware.org/pipermail/newlib/2013/010831.html

I have built a patch attached for this issue.

With this patch, __sfputs_r/__sprint_r is split into two versions, one
is for vfprintf which does not handle wide string, and the other (newly
introduced __sfputws_r/__swprin_r) is for vfwprintf which handles wide
string. Please note that fprintf gets working for wide orient stream
just like BSD libc, which behaves differently from GNU libc.

This patch also fixes nano-vfprintf.c as well as vfprintf.c/vfwprintf.c
in the same manner.

Could someone please review the patch?
Thanks in advance.

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

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

From 1bafca861ed191444f16dedb4b1987404ee7830e Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.yano@nifty.ne.jp>
Date: Tue, 26 Sep 2023 08:19:07 +0900
Subject: [PATCH] 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, __sfputs_r/__sprint_r is split into two versions;
one is for vfprintf which does not handle wide string, and the other
(newly introduced __sfputws_r/__swprin_r) is for vfwprintf which
handles wide string. Please note that fprintf() gets working for
wide-oriented stream just like BSD libc, which behaves differently
from GNU libc.

Revewed-by:
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
---
 newlib/libc/stdio/nano-vfprintf.c | 51 ++-----------------
 newlib/libc/stdio/vfprintf.c      | 44 ++---------------
 newlib/libc/stdio/vfwprintf.c     | 81 ++++++++++++++++++++++++++++++-
 3 files changed, 89 insertions(+), 87 deletions(-)

diff --git a/newlib/libc/stdio/nano-vfprintf.c b/newlib/libc/stdio/nano-vfprintf.c
index 0d42a940f..d2143ef08 100644
--- a/newlib/libc/stdio/nano-vfprintf.c
+++ b/newlib/libc/stdio/nano-vfprintf.c
@@ -356,32 +356,7 @@ __sprint_r (struct _reent *ptr,
       uio->uio_iovcnt = 0;
       return 0;
     }
-#if defined _WIDE_ORIENT && (!defined _ELIX_LEVEL || _ELIX_LEVEL >= 4)
-    if (fp->_flags2 & __SWID)
-      {
-	struct __siov *iov;
-	wchar_t *p;
-	int i, len;
-
-	iov = uio->uio_iov;
-	for (; uio->uio_resid != 0;
-	     uio->uio_resid -= len * sizeof (wchar_t), iov++)
-	  {
-	    p = (wchar_t *) iov->iov_base;
-	    len = iov->iov_len / sizeof (wchar_t);
-	    for (i = 0; i < len; i++)
-	      {
-		if (_fputwc_r (ptr, p[i], fp) == WEOF)
-		  {
-		    err = -1;
-		    goto out;
-		  }
-	      }
-	  }
-	}
-      else
-#endif
-	err = __sfvwrite_r(ptr, fp, uio);
+  err = __sfvwrite_r(ptr, fp, uio);
 out:
   uio->uio_resid = 0;
   uio->uio_iovcnt = 0;
@@ -407,27 +382,11 @@ __sfputs_r (struct _reent *ptr,
 {
   register int i;
 
-#if defined _WIDE_ORIENT && (!defined _ELIX_LEVEL || _ELIX_LEVEL >= 4)
-  if (fp->_flags2 & __SWID)
-    {
-      wchar_t *p;
-
-      p = (wchar_t *) buf;
-      for (i = 0; i < (len / sizeof (wchar_t)); i++)
-	{
-	  if (_fputwc_r (ptr, p[i], fp) == WEOF)
-	    return -1;
-	}
-    }
-  else
-#endif
+  for (i = 0; i < len; i++)
     {
-      for (i = 0; i < len; i++)
-	{
-	  /* Call __sfputc_r to skip _fputc_r.  */
-	  if (__sfputc_r (ptr, (int)buf[i], fp) == EOF)
-	    return -1;
-	}
+      /* Call __sfputc_r to skip _fputc_r.  */
+      if (__sfputc_r (ptr, (int)buf[i], fp) == EOF)
+	return -1;
     }
   return (0);
 }
diff --git a/newlib/libc/stdio/vfprintf.c b/newlib/libc/stdio/vfprintf.c
index 6a198e2c6..95412ced9 100644
--- a/newlib/libc/stdio/vfprintf.c
+++ b/newlib/libc/stdio/vfprintf.c
@@ -187,7 +187,7 @@ static char *rcsid = "$Id$";
 # endif
 #endif
 
-/* The __sprint_r/__ssprint_r functions are shared between all versions of
+/* The __ssputs_r/__ssprint_r functions are shared between all versions of
    vfprintf and vfwprintf.  They must only be defined once, which we do in
    the INTEGER_ONLY versions here. */
 #ifdef STRING_ONLY
@@ -370,23 +370,9 @@ __sfputs_r (struct _reent *ptr,
 {
 	register int i;
 
-#if defined _WIDE_ORIENT && (!defined _ELIX_LEVEL || _ELIX_LEVEL >= 4)
-	if (fp->_flags2 & __SWID) {
-		wchar_t *p;
-
-		p = (wchar_t *) buf;
-		for (i = 0; i < (len / sizeof (wchar_t)); i++) {
-			if (_fputwc_r (ptr, p[i], fp) == WEOF)
-				return -1;
-		}
-	} else {
-#else
-	{
-#endif
-		for (i = 0; i < len; i++) {
-			if (_fputc_r (ptr, buf[i], fp) == EOF)
-				return -1;
-		}
+	for (i = 0; i < len; i++) {
+		if (_fputc_r (ptr, buf[i], fp) == EOF)
+			return -1;
 	}
 	return (0);
 }
@@ -406,27 +392,7 @@ __sprint_r (struct _reent *ptr,
 		uio->uio_iovcnt = 0;
 		return (0);
 	}
-#if defined _WIDE_ORIENT && (!defined _ELIX_LEVEL || _ELIX_LEVEL >= 4)
-	if (fp->_flags2 & __SWID) {
-		struct __siov *iov;
-		wchar_t *p;
-		int i, len;
-
-		iov = uio->uio_iov;
-		for (; uio->uio_resid != 0;
-		     uio->uio_resid -= len * sizeof (wchar_t), iov++) {
-			p = (wchar_t *) iov->iov_base;
-			len = iov->iov_len / sizeof (wchar_t);
-			for (i = 0; i < len; i++) {
-				if (_fputwc_r (ptr, p[i], fp) == WEOF) {
-					err = -1;
-					goto out;
-				}
-			}
-		}
-	} else
-#endif
-		err = __sfvwrite_r(ptr, fp, uio);
+	err = __sfvwrite_r(ptr, fp, uio);
 out:
 	uio->uio_resid = 0;
 	uio->uio_iovcnt = 0;
diff --git a/newlib/libc/stdio/vfwprintf.c b/newlib/libc/stdio/vfwprintf.c
index 7807a1229..de494f44a 100644
--- a/newlib/libc/stdio/vfwprintf.c
+++ b/newlib/libc/stdio/vfwprintf.c
@@ -155,18 +155,95 @@ int _VFWPRINTF_R (struct _reent *, FILE *, const wchar_t *, va_list);
 # ifdef STRING_ONLY
 #  define __SPRINT __ssprint_r
 # else
-#  define __SPRINT __sprint_r
+#  define __SPRINT __swprint_r
 # endif
 int __SPRINT (struct _reent *, FILE *, register struct __suio *);
 #else
 # ifdef STRING_ONLY
 #  define __SPRINT __ssputs_r
 # else
-#  define __SPRINT __sfputs_r
+#  define __SPRINT __sfputws_r
 # endif
 int __SPRINT (struct _reent *, FILE *, const char *, size_t);
 #endif
+
 #ifndef STRING_ONLY
+#ifdef INTEGER_ONLY
+#ifndef _FVWRITE_IN_STREAMIO
+int
+__sfputws_r (struct _reent *ptr,
+       FILE *fp,
+       const char *buf,
+       size_t len)
+{
+	register int i;
+
+#if defined _WIDE_ORIENT && (!defined _ELIX_LEVEL || _ELIX_LEVEL >= 4)
+	wchar_t *p;
+
+	p = (wchar_t *) buf;
+	for (i = 0; i < (len / sizeof (wchar_t)); i++) {
+		if (_fputwc_r (ptr, p[i], fp) == WEOF)
+			return -1;
+	}
+#else
+	for (i = 0; i < len; i++) {
+		if (_fputc_r (ptr, buf[i], fp) == EOF)
+			return -1;
+	}
+#endif
+	return (0);
+}
+#endif
+/*
+ * Flush out all the vectors defined by the given uio,
+ * then reset it so that it can be reused.
+ */
+int
+__swprint_r (struct _reent *ptr,
+       FILE *fp,
+       register struct __suio *uio)
+{
+	register int err = 0;
+
+	if (uio->uio_resid == 0) {
+		uio->uio_iovcnt = 0;
+		return (0);
+	}
+#if defined _WIDE_ORIENT && (!defined _ELIX_LEVEL || _ELIX_LEVEL >= 4)
+	do {
+		struct __siov *iov;
+		wchar_t *p;
+		int i, len;
+
+		iov = uio->uio_iov;
+		for (; uio->uio_resid != 0;
+			 uio->uio_resid -= len * sizeof (wchar_t), iov++) {
+			p = (wchar_t *) iov->iov_base;
+			len = iov->iov_len / sizeof (wchar_t);
+			for (i = 0; i < len; i++) {
+				if (_fputwc_r (ptr, p[i], fp) == WEOF) {
+					err = -1;
+					goto out;
+				}
+			}
+		}
+	} while (0);
+#else
+	err = __sfvwrite_r(ptr, fp, uio);
+#endif
+out:
+	uio->uio_resid = 0;
+	uio->uio_iovcnt = 0;
+	return (err);
+}
+#else /* !INTEGER_ONLY */
+#ifndef _FVWRITE_IN_STREAMIO
+int __sfputws_r (struct _reent *, FILE *, const char *buf, size_t);
+#endif
+int __swprint_r (struct _reent *, FILE *, register struct __suio *);
+#endif /* !INTEGER_ONLY */
+
 #ifdef _UNBUF_STREAM_OPT
 /*
  * Helper function for `fprintf to unbuffered unix file': creates a
-- 
2.39.0

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

[Takashi Yano]

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

On Tue, 26 Sep 2023 12:41:47 +0900
Takashi Yano wrote:
> Hi,
> 
> I noticed that the following test case crashes at printf() with current
> newlib.
> 
> 
> #include <stdio.h>
> #include <wchar.h>
> #include <locale.h>
> 
> int main()
> {
> 	setlocale(LC_ALL, "C.UTF-8");
> 	wprintf(L"%ls\n", L"aaaa"); /* or fwide(stdout, 1); */
> 	printf("%ls\n", L"bbbb");  /* <--- crash here */
> 	return 0;
> }
> 
> 
> I looked into this problem and found the cause.
> 
> 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, crash does not happen, but garbage
> is printed. This hapens if printf("%ls\r\n", L"bbbb"); is used instead.
>                                       ^^
> 
> The same issue seemed to be reported ten years ago.
> https://sourceware.org/pipermail/newlib/2013/010831.html
> 
> I have built a patch attached for this issue.
> 
> With this patch, __sfputs_r/__sprint_r is split into two versions, one
> is for vfprintf which does not handle wide string, and the other (newly
> introduced __sfputws_r/__swprin_r) is for vfwprintf which handles wide
> string. Please note that fprintf gets working for wide orient stream
> just like BSD libc, which behaves differently from GNU libc.
> 
> This patch also fixes nano-vfprintf.c as well as vfprintf.c/vfwprintf.c
> in the same manner.

v2: Remove __sprint_r from nano-vfprintf.c which does not seem to be used
    anymore.

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

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

From e755461cd26805482f062fcc5db174c4b779adf9 Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.yano@nifty.ne.jp>
Date: Tue, 26 Sep 2023 08:19:07 +0900
Subject: [PATCH v2] 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, __sfputs_r/__sprint_r is split into two versions;
one is for vfprintf which does not handle wide string, and the other
(newly introduced __sfputws_r/__swprin_r) is for vfwprintf which
handles wide string. Please note that fprintf() gets working for
wide-oriented stream just like BSD libc, which behaves differently
from GNU libc.

Revewed-by:
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
---
 newlib/libc/stdio/nano-vfprintf.c | 72 ++-------------------------
 newlib/libc/stdio/vfprintf.c      | 44 ++---------------
 newlib/libc/stdio/vfwprintf.c     | 81 ++++++++++++++++++++++++++++++-
 3 files changed, 88 insertions(+), 109 deletions(-)

diff --git a/newlib/libc/stdio/nano-vfprintf.c b/newlib/libc/stdio/nano-vfprintf.c
index 0d42a940f..ffcb1df62 100644
--- a/newlib/libc/stdio/nano-vfprintf.c
+++ b/newlib/libc/stdio/nano-vfprintf.c
@@ -340,54 +340,6 @@ err:
   return EOF;
 }
 #else
-/* As __ssputs_r, __sprint_r is used by output functions for wide char,
-   like vfwprint.  */
-/* Flush out all the vectors defined by the given uio,
-   then reset it so that it can be reused.  */
-int
-__sprint_r (struct _reent *ptr,
-       FILE *fp,
-       register struct __suio *uio)
-{
-  register int err = 0;
-
-  if (uio->uio_resid == 0)
-    {
-      uio->uio_iovcnt = 0;
-      return 0;
-    }
-#if defined _WIDE_ORIENT && (!defined _ELIX_LEVEL || _ELIX_LEVEL >= 4)
-    if (fp->_flags2 & __SWID)
-      {
-	struct __siov *iov;
-	wchar_t *p;
-	int i, len;
-
-	iov = uio->uio_iov;
-	for (; uio->uio_resid != 0;
-	     uio->uio_resid -= len * sizeof (wchar_t), iov++)
-	  {
-	    p = (wchar_t *) iov->iov_base;
-	    len = iov->iov_len / sizeof (wchar_t);
-	    for (i = 0; i < len; i++)
-	      {
-		if (_fputwc_r (ptr, p[i], fp) == WEOF)
-		  {
-		    err = -1;
-		    goto out;
-		  }
-	      }
-	  }
-	}
-      else
-#endif
-	err = __sfvwrite_r(ptr, fp, uio);
-out:
-  uio->uio_resid = 0;
-  uio->uio_iovcnt = 0;
-  return err;
-}
-
 _NOINLINE_STATIC int
 __sfputc_r (struct _reent *ptr,
        int c,
@@ -407,27 +359,11 @@ __sfputs_r (struct _reent *ptr,
 {
   register int i;
 
-#if defined _WIDE_ORIENT && (!defined _ELIX_LEVEL || _ELIX_LEVEL >= 4)
-  if (fp->_flags2 & __SWID)
-    {
-      wchar_t *p;
-
-      p = (wchar_t *) buf;
-      for (i = 0; i < (len / sizeof (wchar_t)); i++)
-	{
-	  if (_fputwc_r (ptr, p[i], fp) == WEOF)
-	    return -1;
-	}
-    }
-  else
-#endif
+  for (i = 0; i < len; i++)
     {
-      for (i = 0; i < len; i++)
-	{
-	  /* Call __sfputc_r to skip _fputc_r.  */
-	  if (__sfputc_r (ptr, (int)buf[i], fp) == EOF)
-	    return -1;
-	}
+      /* Call __sfputc_r to skip _fputc_r.  */
+      if (__sfputc_r (ptr, (int)buf[i], fp) == EOF)
+	return -1;
     }
   return (0);
 }
diff --git a/newlib/libc/stdio/vfprintf.c b/newlib/libc/stdio/vfprintf.c
index 6a198e2c6..95412ced9 100644
--- a/newlib/libc/stdio/vfprintf.c
+++ b/newlib/libc/stdio/vfprintf.c
@@ -187,7 +187,7 @@ static char *rcsid = "$Id$";
 # endif
 #endif
 
-/* The __sprint_r/__ssprint_r functions are shared between all versions of
+/* The __ssputs_r/__ssprint_r functions are shared between all versions of
    vfprintf and vfwprintf.  They must only be defined once, which we do in
    the INTEGER_ONLY versions here. */
 #ifdef STRING_ONLY
@@ -370,23 +370,9 @@ __sfputs_r (struct _reent *ptr,
 {
 	register int i;
 
-#if defined _WIDE_ORIENT && (!defined _ELIX_LEVEL || _ELIX_LEVEL >= 4)
-	if (fp->_flags2 & __SWID) {
-		wchar_t *p;
-
-		p = (wchar_t *) buf;
-		for (i = 0; i < (len / sizeof (wchar_t)); i++) {
-			if (_fputwc_r (ptr, p[i], fp) == WEOF)
-				return -1;
-		}
-	} else {
-#else
-	{
-#endif
-		for (i = 0; i < len; i++) {
-			if (_fputc_r (ptr, buf[i], fp) == EOF)
-				return -1;
-		}
+	for (i = 0; i < len; i++) {
+		if (_fputc_r (ptr, buf[i], fp) == EOF)
+			return -1;
 	}
 	return (0);
 }
@@ -406,27 +392,7 @@ __sprint_r (struct _reent *ptr,
 		uio->uio_iovcnt = 0;
 		return (0);
 	}
-#if defined _WIDE_ORIENT && (!defined _ELIX_LEVEL || _ELIX_LEVEL >= 4)
-	if (fp->_flags2 & __SWID) {
-		struct __siov *iov;
-		wchar_t *p;
-		int i, len;
-
-		iov = uio->uio_iov;
-		for (; uio->uio_resid != 0;
-		     uio->uio_resid -= len * sizeof (wchar_t), iov++) {
-			p = (wchar_t *) iov->iov_base;
-			len = iov->iov_len / sizeof (wchar_t);
-			for (i = 0; i < len; i++) {
-				if (_fputwc_r (ptr, p[i], fp) == WEOF) {
-					err = -1;
-					goto out;
-				}
-			}
-		}
-	} else
-#endif
-		err = __sfvwrite_r(ptr, fp, uio);
+	err = __sfvwrite_r(ptr, fp, uio);
 out:
 	uio->uio_resid = 0;
 	uio->uio_iovcnt = 0;
diff --git a/newlib/libc/stdio/vfwprintf.c b/newlib/libc/stdio/vfwprintf.c
index 7807a1229..de494f44a 100644
--- a/newlib/libc/stdio/vfwprintf.c
+++ b/newlib/libc/stdio/vfwprintf.c
@@ -155,18 +155,95 @@ int _VFWPRINTF_R (struct _reent *, FILE *, const wchar_t *, va_list);
 # ifdef STRING_ONLY
 #  define __SPRINT __ssprint_r
 # else
-#  define __SPRINT __sprint_r
+#  define __SPRINT __swprint_r
 # endif
 int __SPRINT (struct _reent *, FILE *, register struct __suio *);
 #else
 # ifdef STRING_ONLY
 #  define __SPRINT __ssputs_r
 # else
-#  define __SPRINT __sfputs_r
+#  define __SPRINT __sfputws_r
 # endif
 int __SPRINT (struct _reent *, FILE *, const char *, size_t);
 #endif
+
 #ifndef STRING_ONLY
+#ifdef INTEGER_ONLY
+#ifndef _FVWRITE_IN_STREAMIO
+int
+__sfputws_r (struct _reent *ptr,
+       FILE *fp,
+       const char *buf,
+       size_t len)
+{
+	register int i;
+
+#if defined _WIDE_ORIENT && (!defined _ELIX_LEVEL || _ELIX_LEVEL >= 4)
+	wchar_t *p;
+
+	p = (wchar_t *) buf;
+	for (i = 0; i < (len / sizeof (wchar_t)); i++) {
+		if (_fputwc_r (ptr, p[i], fp) == WEOF)
+			return -1;
+	}
+#else
+	for (i = 0; i < len; i++) {
+		if (_fputc_r (ptr, buf[i], fp) == EOF)
+			return -1;
+	}
+#endif
+	return (0);
+}
+#endif
+/*
+ * Flush out all the vectors defined by the given uio,
+ * then reset it so that it can be reused.
+ */
+int
+__swprint_r (struct _reent *ptr,
+       FILE *fp,
+       register struct __suio *uio)
+{
+	register int err = 0;
+
+	if (uio->uio_resid == 0) {
+		uio->uio_iovcnt = 0;
+		return (0);
+	}
+#if defined _WIDE_ORIENT && (!defined _ELIX_LEVEL || _ELIX_LEVEL >= 4)
+	do {
+		struct __siov *iov;
+		wchar_t *p;
+		int i, len;
+
+		iov = uio->uio_iov;
+		for (; uio->uio_resid != 0;
+			 uio->uio_resid -= len * sizeof (wchar_t), iov++) {
+			p = (wchar_t *) iov->iov_base;
+			len = iov->iov_len / sizeof (wchar_t);
+			for (i = 0; i < len; i++) {
+				if (_fputwc_r (ptr, p[i], fp) == WEOF) {
+					err = -1;
+					goto out;
+				}
+			}
+		}
+	} while (0);
+#else
+	err = __sfvwrite_r(ptr, fp, uio);
+#endif
+out:
+	uio->uio_resid = 0;
+	uio->uio_iovcnt = 0;
+	return (err);
+}
+#else /* !INTEGER_ONLY */
+#ifndef _FVWRITE_IN_STREAMIO
+int __sfputws_r (struct _reent *, FILE *, const char *buf, size_t);
+#endif
+int __swprint_r (struct _reent *, FILE *, register struct __suio *);
+#endif /* !INTEGER_ONLY */
+
 #ifdef _UNBUF_STREAM_OPT
 /*
  * Helper function for `fprintf to unbuffered unix file': creates a
-- 
2.39.0

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

[Takashi Yano]

On Tue, 26 Sep 2023 12:41:47 +0900
Takashi Yano wrote:
> With this patch, __sfputs_r/__sprint_r is split into two versions, one
> is for vfprintf which does not handle wide string, and the other (newly
> introduced __sfputws_r/__swprin_r) is for vfwprintf which handles wide
> string. Please note that fprintf gets working for wide orient stream
> just like BSD libc, which behaves differently from GNU libc.

I confirmed musl libc also behaves as BSD libc.

In the GNU libc (glibc), fprintf() returns -1 with no errno set if the
stream is wide-oriented.

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

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

[Takashi Yano]

On Thu, 28 Sep 2023 12:58:27 +0900
Takashi Yano wrote:
> On Tue, 26 Sep 2023 12:41:47 +0900
> Takashi Yano wrote:
> > With this patch, __sfputs_r/__sprint_r is split into two versions, one
> > is for vfprintf which does not handle wide string, and the other (newly
> > introduced __sfputws_r/__swprin_r) is for vfwprintf which handles wide
> > string. Please note that fprintf gets working for wide orient stream
> > just like BSD libc, which behaves differently from GNU libc.
> 
> I confirmed musl libc also behaves as BSD libc.
> 
> In the GNU libc (glibc), fprintf() returns -1 with no errno set if the
> stream is wide-oriented.

I also confirmed that Solaris libc behaves as same as BSD libc.

I wonder what are the advantages of the glibc implementation?
Any historical reason?

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

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

[Brian Inglis]

On 2023-09-28 02:42, Takashi Yano wrote:
> On Thu, 28 Sep 2023 12:58:27 +0900
> Takashi Yano wrote:
>> On Tue, 26 Sep 2023 12:41:47 +0900
>> Takashi Yano wrote:
>>> With this patch, __sfputs_r/__sprint_r is split into two versions, one
>>> is for vfprintf which does not handle wide string, and the other (newly
>>> introduced __sfputws_r/__swprin_r) is for vfwprintf which handles wide
>>> string. Please note that fprintf gets working for wide orient stream
>>> just like BSD libc, which behaves differently from GNU libc.

Note that glibc handles %c/%s in wchar_t functions and %C/%lc/%S/%ls in char 
functions: see `man -m linux 3 fwide` NOTES.

>> I confirmed musl libc also behaves as BSD libc.
>>
>> In the GNU libc (glibc), fprintf() returns -1 with no errno set if the
>> stream is wide-oriented.

That is not documented in released man-pages-linux.
What is documented is that the width setting persists once set, and can not be 
changed until the stream is closed.

> I also confirmed that Solaris libc behaves as same as BSD libc.
> 
> I wonder what are the advantages of the glibc implementation?
> Any historical reason?

What are the behavioural differences you perceive?

There is a public inbox you can search at

	https://inbox.sourceware.org/libc-help/

and you can ask mailto:libc-help@sourceware.org about anything.

-- 
Take care. Thanks, Brian Inglis              Calgary, Alberta, Canada

La perfection est atteinte                   Perfection is achieved
non pas lorsqu'il n'y a plus rien à ajouter  not when there is no more to add
mais lorsqu'il n'y a plus rien à retirer     but when there is no more to cut
                                 -- Antoine de Saint-Exupéry

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

[Takashi Yano]

Ping?

Is this Corinna's domain?

On Tue, 26 Sep 2023 17:30:13 +0900
Takashi Yano wrote:
> On Tue, 26 Sep 2023 12:41:47 +0900
> Takashi Yano wrote:
> > Hi,
> > 
> > I noticed that the following test case crashes at printf() with current
> > newlib.
> > 
> > 
> > #include <stdio.h>
> > #include <wchar.h>
> > #include <locale.h>
> > 
> > int main()
> > {
> > 	setlocale(LC_ALL, "C.UTF-8");
> > 	wprintf(L"%ls\n", L"aaaa"); /* or fwide(stdout, 1); */
> > 	printf("%ls\n", L"bbbb");  /* <--- crash here */
> > 	return 0;
> > }
> > 
> > 
> > I looked into this problem and found the cause.
> > 
> > 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, crash does not happen, but garbage
> > is printed. This hapens if printf("%ls\r\n", L"bbbb"); is used instead.
> >                                       ^^
> > 
> > The same issue seemed to be reported ten years ago.
> > https://sourceware.org/pipermail/newlib/2013/010831.html
> > 
> > I have built a patch attached for this issue.
> > 
> > With this patch, __sfputs_r/__sprint_r is split into two versions, one
> > is for vfprintf which does not handle wide string, and the other (newly
> > introduced __sfputws_r/__swprin_r) is for vfwprintf which handles wide
> > string. Please note that fprintf gets working for wide orient stream
> > just like BSD libc, which behaves differently from GNU libc.
> > 
> > This patch also fixes nano-vfprintf.c as well as vfprintf.c/vfwprintf.c
> > in the same manner.
> 
> v2: Remove __sprint_r from nano-vfprintf.c which does not seem to be used
>     anymore.
> 
> -- 
> Takashi Yano <takashi.yano@nifty.ne.jp>


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

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

[Jeff Johnston]

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

I'll look at it.

-- Jeff J.

On Tue, Oct 3, 2023 at 4:30 AM Takashi Yano <takashi.yano@nifty.ne.jp>
wrote:

> Ping?
>
> Is this Corinna's domain?
>
> On Tue, 26 Sep 2023 17:30:13 +0900
> Takashi Yano wrote:
> > On Tue, 26 Sep 2023 12:41:47 +0900
> > Takashi Yano wrote:
> > > Hi,
> > >
> > > I noticed that the following test case crashes at printf() with current
> > > newlib.
> > >
> > >
> > > #include <stdio.h>
> > > #include <wchar.h>
> > > #include <locale.h>
> > >
> > > int main()
> > > {
> > >     setlocale(LC_ALL, "C.UTF-8");
> > >     wprintf(L"%ls\n", L"aaaa"); /* or fwide(stdout, 1); */
> > >     printf("%ls\n", L"bbbb");  /* <--- crash here */
> > >     return 0;
> > > }
> > >
> > >
> > > I looked into this problem and found the cause.
> > >
> > > 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, crash does not happen, but
> garbage
> > > is printed. This hapens if printf("%ls\r\n", L"bbbb"); is used instead.
> > >                                       ^^
> > >
> > > The same issue seemed to be reported ten years ago.
> > > https://sourceware.org/pipermail/newlib/2013/010831.html
> > >
> > > I have built a patch attached for this issue.
> > >
> > > With this patch, __sfputs_r/__sprint_r is split into two versions, one
> > > is for vfprintf which does not handle wide string, and the other (newly
> > > introduced __sfputws_r/__swprin_r) is for vfwprintf which handles wide
> > > string. Please note that fprintf gets working for wide orient stream
> > > just like BSD libc, which behaves differently from GNU libc.
> > >
> > > This patch also fixes nano-vfprintf.c as well as vfprintf.c/vfwprintf.c
> > > in the same manner.
> >
> > v2: Remove __sprint_r from nano-vfprintf.c which does not seem to be used
> >     anymore.
> >
> > --
> > Takashi Yano <takashi.yano@nifty.ne.jp>
>
>
> --
> Takashi Yano <takashi.yano@nifty.ne.jp>
>
>

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

[Brian Inglis]

On 2023-10-03 02:30, Takashi Yano wrote:
> Ping? Is this Corinna's domain?

https://sourceware.org/newlib/

Patience - the volunteer reviewers may be busy with work or life or be on 
vacation for a month!

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

[Takashi Yano]

On Tue, 3 Oct 2023 13:31:30 -0400
Jeff Johnston wrote:
> I'll look at it.

Thanks!

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

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

[Jeff Johnston]

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

Hi Takashi,

I finally took a look at this.  The issue is whether POSIX compliance is
desired.   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.

-- Jeff J.

On Wed, Sep 27, 2023 at 11:58 PM Takashi Yano <takashi.yano@nifty.ne.jp>
wrote:

> On Tue, 26 Sep 2023 12:41:47 +0900
> Takashi Yano wrote:
> > With this patch, __sfputs_r/__sprint_r is split into two versions, one
> > is for vfprintf which does not handle wide string, and the other (newly
> > introduced __sfputws_r/__swprin_r) is for vfwprintf which handles wide
> > string. Please note that fprintf gets working for wide orient stream
> > just like BSD libc, which behaves differently from GNU libc.
>
> I confirmed musl libc also behaves as BSD libc.
>
> In the GNU libc (glibc), fprintf() returns -1 with no errno set if the
> stream is wide-oriented.
>
> --
> Takashi Yano <takashi.yano@nifty.ne.jp>
>
>

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

[Takashi Yano]

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.

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

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

[Takashi Yano]

[-- 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

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

[Torbjorn SVENSSON]

On 2023-10-05 17:18, Takashi Yano wrote:
> 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?

Can you please define symbols for those 1 and -1 so that the code would 
be easier to understand when reading it?

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

[Corinna Vinschen]

Hey guys,

On Oct  4 16:16, Jeff Johnston wrote:
> Hi Takashi,
> 
> I finally took a look at this.  The issue is whether POSIX compliance is
> desired.   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 took a look into the POSIX docs.  POSIX aligns with ISO/IEC 9899:1999.
The description is slightly vague, in that it only has to say this:

  "Byte input/output functions cannot be applied to a wide-oriented
   stream, and wide-character input/output functions cannot be applied to
   a byte-oriented stream."

It does not explicitely outline what "cannot be applied" means in this
context.

IIUC, this *could* mean that in case of the testcase a crash is as much
standards-compliant as the GLibC behaviour. Not that a crash is desired,
of course...

In how far the BSD behaviour is covered by this description, I really
can't tell.

I wonder if the Austin group could clarify, or if a clarification
already exists and I just missed it.  CC Eric, in case he wants to
follow up on this.

Either way, I think that the safe way forward is actually to behave
as GLibC does.

> 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.

In terms of ISO/IEC 9899:1999 I agree. However, it also shows the
flaw that newlib crashes with a buffer overflow, which we should
avoid if possible.

Does that make sense?


Thanks,
Corinna

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

[Brian Inglis]

On 2023-11-02 12:53, Corinna Vinschen wrote:
> On Oct  4 16:16, Jeff Johnston wrote:
>> I finally took a look at this.  The issue is whether POSIX compliance is 
>> desired.   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 took a look into the POSIX docs.  POSIX aligns with ISO/IEC 9899:1999.
> The description is slightly vague, in that it only has to say this:
>    "Byte input/output functions cannot be applied to a wide-oriented
>     stream, and wide-character input/output functions cannot be applied to
>     a byte-oriented stream."
> It does not explicitely outline what "cannot be applied" means in this
> context.

That seems to me to imply that the compiler /could/ diagnose if oriented stream 
functions are used consistently, and that those explicitly byte- or 
wide-oriented stream I/O functions should check the orientation and fail, or do 
nothing, unless they are required to be flexible like the perror/psig... 
functions mentioned below, to adapt and not change the stream orientation.

> IIUC, this *could* mean that in case of the testcase a crash is as much
> standards-compliant as the GLibC behaviour. Not that a crash is desired,
> of course...
> In how far the BSD behaviour is covered by this description, I really
> can't tell.
> I wonder if the Austin group could clarify, or if a clarification
> already exists and I just missed it.  CC Eric, in case he wants to
> follow up on this.
> Either way, I think that the safe way forward is actually to behave
> as GLibC does.

As far as I could see, the only obvious changes are tweaks for exceptions about 
open_wmemstream.

It also looks like errno should be set to EBADF to indicate that the operation 
is invalid for the stream.

>> 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.
That should not be necessary, as the first byte- or wide-oriented stream I/O 
function called against the stream after fopen or freopen sets the orientation 
from unoriented; that includes fwide() if called with a non-zero argument; 
otherwise fwide() may query the stream orientation but not change it after it is 
set.

> In terms of ISO/IEC 9899:1999 I agree. However, it also shows the
> flaw that newlib crashes with a buffer overflow, which we should
> avoid if possible.

Also note from Takashi's issue raised by Eric 5 years ago, perror(), psignal(), 
and psiginfo() "shall not change the orientation of the standard error stream":

https://collaboration.opengroup.org/operational/mailarch.php?soph=N&action=show&archive=austin-group-l&num=27231&limit=100&offset=9400&sid=

https://www.mail-archive.com/austin-group-l@opengroup.org/msg02582.html

-- 
Take care. Thanks, Brian Inglis              Calgary, Alberta, Canada

La perfection est atteinte                   Perfection is achieved
non pas lorsqu'il n'y a plus rien à ajouter  not when there is no more to add
mais lorsqu'il n'y a plus rien à retirer     but when there is no more to cut
                                 -- Antoine de Saint-Exupéry

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

[Corinna Vinschen]

Hi Takashi, hi Jeff,


I checked the history of the orientation stuff and I think I came to a
conclusion.

On Oct  6 00:18, Takashi Yano wrote:
> 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

You can't reset the orientation once it's set.  fwide(3) only allows
to change the orientation if it's still undecided.  Once you did
set the orientation, you can't change it back, neither to undecided,
nor to the other orientation.  freopen(3) is the only way to reset
the orientation to undecided.

> 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?

It took me a while, but I think the BSD behaviour is only accepted
(acceptable) due its long history.  The only really correct way of
handling this issue is to do soemthing along the lines of GLibC.

I. e., while "cannot be applied" is sufficently vague, it should be
interpreted as "must not be applied", basically.  However, "must not"
kind of implies setting errno, but there's not a trace of that in
the standard.

Consequentially, IMHO, the way GLibC handles it sounds like the best way
out: The call is a no-op and returns a value indicating that the stream
isn't available for the given operation (EOF/WEOF/younameit), but it
does not change errno.  There's no errno value defined for this kind
of problem anyway.

Takashi, assuming everybody is ok, your patch below looks good.  I'm
just wondering if we really need an extra QUERY_ORIENT() macro.  What if
ORIENT() itself returns the actual value?  That would allow to just
write, for instance

-  ORIENT(fp, 1);
+  if (ORIENT(fp, 1) != 1)
+    return WEOF;


Thanks,
Corinna

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

[Brian Inglis]

On 2023-11-07 06:24, Corinna Vinschen wrote:
> Hi Takashi, hi Jeff,
> 
> 
> I checked the history of the orientation stuff and I think I came to a
> conclusion.
> 
> On Oct  6 00:18, Takashi Yano wrote:
>> 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
> 
> You can't reset the orientation once it's set.  fwide(3) only allows
> to change the orientation if it's still undecided.  Once you did
> set the orientation, you can't change it back, neither to undecided,
> nor to the other orientation.  freopen(3) is the only way to reset
> the orientation to undecided.
> 
>> 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?
> 
> It took me a while, but I think the BSD behaviour is only accepted
> (acceptable) due its long history.  The only really correct way of
> handling this issue is to do soemthing along the lines of GLibC.
> 
> I. e., while "cannot be applied" is sufficently vague, it should be
> interpreted as "must not be applied", basically.  However, "must not"
> kind of implies setting errno, but there's not a trace of that in
> the standard.
> 
> Consequentially, IMHO, the way GLibC handles it sounds like the best way
> out: The call is a no-op and returns a value indicating that the stream
> isn't available for the given operation (EOF/WEOF/younameit), but it
> does not change errno.  There's no errno value defined for this kind
> of problem anyway.

POSIX disagrees with glibc and states that errno should be set (to EALREADY, 
EBADF, EBADFD, EIO, ENOTSUP, EPERM, or add EORIENT or EWIDTH?) and callers need 
to do "the errno shuffle":

https://pubs.opengroup.org/onlinepubs/9699919799/functions/fwide.html

"[CX] [Option Start] The fwide() function shall not change the setting of errno 
if successful.

Since no return value is reserved to indicate an error, an application wishing 
to check for error situations should set errno to 0, then call fwide(), then 
check errno, and if it is non-zero, assume an error has occurred. [Option End]"
...
"ERRORS
The fwide() function may fail if:

[EBADF]
[CX] [Option Start] The stream argument is not a valid stream. [Option End]"

This lack of an error definition for oriented streams seems to be an omission, 
probably resulting from each implementation doing their own thing.

POSIX specifies only EBADF - all should use EBADF - if POSIX disagree with the 
interpretation of the intent they can clarify that in a Note in a TC.

[I would prefer ENOTTY - E-naughty - see Python (Monty) ;^> ]

-- 
Take care. Thanks, Brian Inglis              Calgary, Alberta, Canada

La perfection est atteinte                   Perfection is achieved
non pas lorsqu'il n'y a plus rien à ajouter  not when there is no more to add
mais lorsqu'il n'y a plus rien à retirer     but when there is no more to cut
                                 -- Antoine de Saint-Exupéry

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

[Corinna Vinschen]

On Nov  7 09:50, Brian Inglis wrote:
> On 2023-11-07 06:24, Corinna Vinschen wrote:
> > It took me a while, but I think the BSD behaviour is only accepted
> > (acceptable) due its long history.  The only really correct way of
> > handling this issue is to do soemthing along the lines of GLibC.
> > 
> > I. e., while "cannot be applied" is sufficently vague, it should be
> > interpreted as "must not be applied", basically.  However, "must not"
> > kind of implies setting errno, but there's not a trace of that in
> > the standard.
> > 
> > Consequentially, IMHO, the way GLibC handles it sounds like the best way
> > out: The call is a no-op and returns a value indicating that the stream
> > isn't available for the given operation (EOF/WEOF/younameit), but it
> > does not change errno.  There's no errno value defined for this kind
> > of problem anyway.
> 
> POSIX disagrees with glibc and states that errno should be set (to EALREADY,
> EBADF, EBADFD, EIO, ENOTSUP, EPERM, or add EORIENT or EWIDTH?) and callers
> need to do "the errno shuffle":
> 
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/fwide.html
> 
> "[CX] [Option Start] The fwide() function shall not change the setting of
> errno if successful.
> 
> Since no return value is reserved to indicate an error, an application
> wishing to check for error situations should set errno to 0, then call
> fwide(), then check errno, and if it is non-zero, assume an error has
> occurred. [Option End]"

You're totally missing the point.  The POSIX text is about an error
occuring in fwide(3), and the only possible error in fwide(3) is an
invalid file pointer.  Given that fwide(3) doesn't define a return
value indicating an error, the above errno checking is required in
the application.

However, Takashi's input and code as well as my reply are talking about
all the other stdio functions depending on the orientation value.  The
stdio function behaviour in terms of orientation has nothing to do with
the POSIX description of fwide(3) you're quoting.


Corinna

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

[Takashi Yano]

Hi Corinna,

On Tue, 7 Nov 2023 14:24:47 +0100
Corinna Vinschen wrote:
> Hi Takashi, hi Jeff,
> 
> 
> I checked the history of the orientation stuff and I think I came to a
> conclusion.
> 
> On Oct  6 00:18, Takashi Yano wrote:
> > 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
> 
> You can't reset the orientation once it's set.  fwide(3) only allows
> to change the orientation if it's still undecided.  Once you did
> set the orientation, you can't change it back, neither to undecided,
> nor to the other orientation.  freopen(3) is the only way to reset
> the orientation to undecided.
> 
> > 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?
> 
> It took me a while, but I think the BSD behaviour is only accepted
> (acceptable) due its long history.  The only really correct way of
> handling this issue is to do soemthing along the lines of GLibC.
> 
> I. e., while "cannot be applied" is sufficently vague, it should be
> interpreted as "must not be applied", basically.  However, "must not"
> kind of implies setting errno, but there's not a trace of that in
> the standard.
> 
> Consequentially, IMHO, the way GLibC handles it sounds like the best way
> out: The call is a no-op and returns a value indicating that the stream
> isn't available for the given operation (EOF/WEOF/younameit), but it
> does not change errno.  There's no errno value defined for this kind
> of problem anyway.
> 
> Takashi, assuming everybody is ok, your patch below looks good.  I'm
> just wondering if we really need an extra QUERY_ORIENT() macro.  What if
> ORIENT() itself returns the actual value?  That would allow to just
> write, for instance
> 
> -  ORIENT(fp, 1);
> +  if (ORIENT(fp, 1) != 1)
> +    return WEOF;

Thanks for reviewing the patch.
I revised the patch as you suggested.
Please see v4 patch I have just posted.

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