[PATCH v2] newlib: libc: Fix bugs in the commit 3d94e07c49b5.

[PATCH v2] newlib: libc: Fix bugs in the commit 3d94e07c49b5.

[Takashi Yano]

The commit 3d94e07c49b5 has a few bugs which cause testsuite failure
in libstdc++. This is due to excess orientation check in __srefill_r()
and _ungetc_r(). Further, sscanf() family also calls ssvfscanf() family
with fp->_file == -1. This causes undesired orientation set/check for
sscanf() family. This patch fixes these problems.

Fixes: 3d94e07c49b5 ("newlib: libc: Fix crash on fprintf to a wide-oriented stream.")
Reported-by: Christophe Lyon <christophe.lyon@linaro.org>
Reported-by: Joel Sherrill <joel@rtems.org>
Reviewed-by: Corinna Vinschen <corinna@vinschen.de>
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
---
 newlib/libc/stdio/local.h   | 35 ++++++++++++++++++++---------------
 newlib/libc/stdio/refill.c  |  3 ---
 newlib/libc/stdio/ungetc.c  |  8 ++------
 newlib/libc/stdio/ungetwc.c |  2 ++
 4 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/newlib/libc/stdio/local.h b/newlib/libc/stdio/local.h
index 3b86cf19a..dfb9fbbd0 100644
--- a/newlib/libc/stdio/local.h
+++ b/newlib/libc/stdio/local.h
@@ -231,21 +231,26 @@ extern _READ_WRITE_RETURN_TYPE __swrite64 (struct _reent *, void *,
  * Set the orientation for a stream. If o > 0, the stream has wide-
  * orientation. If o < 0, the stream has byte-orientation.
  */
-#define ORIENT(fp,ori)			\
-  (					\
-    (					\
-      ((fp)->_flags & __SORD) ?		\
-	0				\
-      :					\
-	(				\
-	  ((fp)->_flags |= __SORD),	\
-	  (ori > 0) ?			\
-	    ((fp)->_flags2 |= __SWID)	\
-	  :				\
-	    ((fp)->_flags2 &= ~__SWID)	\
-	)				\
-    ),					\
-    ((fp)->_flags2 & __SWID) ? 1 : -1	\
+#define ORIENT(fp,ori)				\
+  (						\
+    ((fp)->_file < 0) ?				\
+      ((ori > 0) ? 1 : -1)			\
+    :						\
+      (						\
+	(					\
+	  ((fp)->_flags & __SORD) ?		\
+	    0					\
+	  :					\
+	    (					\
+	      ((fp)->_flags |= __SORD),		\
+	      (ori > 0) ?			\
+		((fp)->_flags2 |= __SWID)	\
+	      :					\
+		((fp)->_flags2 &= ~__SWID)	\
+	    )					\
+	),					\
+	((fp)->_flags2 & __SWID) ? 1 : -1	\
+      )						\
   )
 #else
 #define ORIENT(fp,ori) (-1)
diff --git a/newlib/libc/stdio/refill.c b/newlib/libc/stdio/refill.c
index c1ef7e120..cd71ed152 100644
--- a/newlib/libc/stdio/refill.c
+++ b/newlib/libc/stdio/refill.c
@@ -43,9 +43,6 @@ __srefill_r (struct _reent * ptr,
 
   CHECK_INIT (ptr, fp);
 
-  if (ORIENT (fp, -1) != -1)
-    return EOF;
-
   fp->_r = 0;			/* largely a convenience for callers */
 
   /* SysV does not make this test; take it out for compatibility */
diff --git a/newlib/libc/stdio/ungetc.c b/newlib/libc/stdio/ungetc.c
index 79914af08..5053fd6c4 100644
--- a/newlib/libc/stdio/ungetc.c
+++ b/newlib/libc/stdio/ungetc.c
@@ -125,12 +125,6 @@ _ungetc_r (struct _reent *rptr,
 
   _newlib_flockfile_start (fp);
 
-  if (ORIENT (fp, -1) != -1)
-    {
-      _newlib_flockfile_exit (fp);
-      return EOF;
-    }
-
   /* After ungetc, we won't be at eof anymore */
   fp->_flags &= ~__SEOF;
 
@@ -213,6 +207,8 @@ int
 ungetc (int c,
        register FILE *fp)
 {
+  if (ORIENT (fp, -1) != -1)
+    return EOF;
   return _ungetc_r (_REENT, c, fp);
 }
 #endif /* !_REENT_ONLY */
diff --git a/newlib/libc/stdio/ungetwc.c b/newlib/libc/stdio/ungetwc.c
index 18636d773..002b6292a 100644
--- a/newlib/libc/stdio/ungetwc.c
+++ b/newlib/libc/stdio/ungetwc.c
@@ -112,5 +112,7 @@ ungetwc (wint_t wc,
   struct _reent *reent = _REENT;
 
   CHECK_INIT (reent, fp);
+  if (ORIENT (fp, 1) != 1)
+    return WEOF;
   return _ungetwc_r (reent, wc, fp);
 }
-- 
2.39.0

Re: [PATCH v2] newlib: libc: Fix bugs in the commit 3d94e07c49b5.

[Corinna Vinschen]

Hi Takashi,

On Nov 11 01:09, Takashi Yano wrote:
> diff --git a/newlib/libc/stdio/ungetc.c b/newlib/libc/stdio/ungetc.c
> index 79914af08..5053fd6c4 100644
> --- a/newlib/libc/stdio/ungetc.c
> +++ b/newlib/libc/stdio/ungetc.c
> @@ -125,12 +125,6 @@ _ungetc_r (struct _reent *rptr,
>  
>    _newlib_flockfile_start (fp);
>  
> -  if (ORIENT (fp, -1) != -1)
> -    {
> -      _newlib_flockfile_exit (fp);
> -      return EOF;
> -    }
> -
>    /* After ungetc, we won't be at eof anymore */
>    fp->_flags &= ~__SEOF;
>  
> @@ -213,6 +207,8 @@ int
>  ungetc (int c,
>         register FILE *fp)
>  {
> +  if (ORIENT (fp, -1) != -1)
> +    return EOF;
>    return _ungetc_r (_REENT, c, fp);
>  }
>  #endif /* !_REENT_ONLY */

As per the reasoning in terms of old standards, we shouldn't check or set
the orientation in ungetc at all.


Thanks,
Corinna

Re: [PATCH v2] newlib: libc: Fix bugs in the commit 3d94e07c49b5.

[Corinna Vinschen]

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

Hi Takashi,

On Nov 13 14:07, Corinna Vinschen wrote:
> On Nov 11 01:09, Takashi Yano wrote:
> > diff --git a/newlib/libc/stdio/ungetc.c b/newlib/libc/stdio/ungetc.c
> > index 79914af08..5053fd6c4 100644
> > --- a/newlib/libc/stdio/ungetc.c
> > +++ b/newlib/libc/stdio/ungetc.c
> > @@ -125,12 +125,6 @@ _ungetc_r (struct _reent *rptr,
> >  
> >    _newlib_flockfile_start (fp);
> >  
> > -  if (ORIENT (fp, -1) != -1)
> > -    {
> > -      _newlib_flockfile_exit (fp);
> > -      return EOF;
> > -    }
> > -
> >    /* After ungetc, we won't be at eof anymore */
> >    fp->_flags &= ~__SEOF;
> >  
> > @@ -213,6 +207,8 @@ int
> >  ungetc (int c,
> >         register FILE *fp)
> >  {
> > +  if (ORIENT (fp, -1) != -1)
> > +    return EOF;
> >    return _ungetc_r (_REENT, c, fp);
> >  }
> >  #endif /* !_REENT_ONLY */
> 
> As per the reasoning in terms of old standards, we shouldn't check or set
> the orientation in ungetc at all.

Given all that has been discussed, I attached a followup proposal.
I hope that's ok.  We may want to release 3.4.10 in the next weeks.


Thanks,
Corinna

[-- Attachment #2: 0001-newlib-libc-Fix-bugs-in-the-commit-3d94e07c49b5.patch --]
[-- Type: text/plain, Size: 17118 bytes --]

From 528fcc9624a505c365cdb120e0dc0f0eee60398c Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.yano@nifty.ne.jp>
Date: Sat, 11 Nov 2023 01:09:47 +0900
Subject: [PATCH v3] newlib: libc: Fix bugs in the commit 3d94e07c49b5.

The commit 3d94e07c49b5 has a few bugs which cause testsuite failure
in libstdc++. This is due to excess orientation check in __srefill_r()
and _ungetc_r(). Further, sscanf() family also calls ssvfscanf() family
with fp->_file == -1. This causes undesired orientation set/check for
sscanf() family. This patch fixes these problems.

Also, as in GLibC, do not set orientation in ungetc, and only set, but
do not check orientation in ungetwc.

Fixes: 3d94e07c49b5 ("newlib: libc: Fix crash on fprintf to a wide-oriented stream.")
Reported-by: Christophe Lyon <christophe.lyon@linaro.org>
Reported-by: Joel Sherrill <joel@rtems.org>
Co-developed-by: Corinna Vinschen <corinna@vinschen.de>
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
Signed-off-by: Corinna Vinschen <corinna@vinschen.de>
---
 newlib/libc/stdio/asiprintf.c   | 2 ++
 newlib/libc/stdio/asniprintf.c  | 2 ++
 newlib/libc/stdio/asnprintf.c   | 2 ++
 newlib/libc/stdio/asprintf.c    | 2 ++
 newlib/libc/stdio/getc.c        | 6 ++++++
 newlib/libc/stdio/refill.c      | 3 ---
 newlib/libc/stdio/siprintf.c    | 2 ++
 newlib/libc/stdio/siscanf.c     | 2 ++
 newlib/libc/stdio/sniprintf.c   | 2 ++
 newlib/libc/stdio/snprintf.c    | 2 ++
 newlib/libc/stdio/sprintf.c     | 2 ++
 newlib/libc/stdio/sscanf.c      | 2 ++
 newlib/libc/stdio/swprintf.c    | 2 ++
 newlib/libc/stdio/swscanf.c     | 2 ++
 newlib/libc/stdio/ungetc.c      | 6 ------
 newlib/libc/stdio/ungetwc.c     | 1 +
 newlib/libc/stdio/vasiprintf.c  | 1 +
 newlib/libc/stdio/vasniprintf.c | 1 +
 newlib/libc/stdio/vasnprintf.c  | 1 +
 newlib/libc/stdio/vasprintf.c   | 1 +
 newlib/libc/stdio/vsiprintf.c   | 1 +
 newlib/libc/stdio/vsiscanf.c    | 1 +
 newlib/libc/stdio/vsniprintf.c  | 1 +
 newlib/libc/stdio/vsnprintf.c   | 1 +
 newlib/libc/stdio/vsprintf.c    | 1 +
 newlib/libc/stdio/vsscanf.c     | 1 +
 newlib/libc/stdio/vswprintf.c   | 1 +
 27 files changed, 42 insertions(+), 9 deletions(-)

diff --git a/newlib/libc/stdio/asiprintf.c b/newlib/libc/stdio/asiprintf.c
index af25e9a57acd..dedb7fbfabce 100644
--- a/newlib/libc/stdio/asiprintf.c
+++ b/newlib/libc/stdio/asiprintf.c
@@ -35,6 +35,7 @@ _asiprintf_r (struct _reent *ptr,
 
   /* mark a zero-length reallocatable buffer */
   f._flags = __SWR | __SSTR | __SMBF;
+  f._flags2 = 0;
   f._bf._base = f._p = NULL;
   f._bf._size = f._w = 0;
   f._file = -1;  /* No file. */
@@ -61,6 +62,7 @@ asiprintf (char **strp,
 
   /* mark a zero-length reallocatable buffer */
   f._flags = __SWR | __SSTR | __SMBF;
+  f._flags2 = 0;
   f._bf._base = f._p = NULL;
   f._bf._size = f._w = 0;
   f._file = -1;  /* No file. */
diff --git a/newlib/libc/stdio/asniprintf.c b/newlib/libc/stdio/asniprintf.c
index 97e77748e75d..e930c8d69f64 100644
--- a/newlib/libc/stdio/asniprintf.c
+++ b/newlib/libc/stdio/asniprintf.c
@@ -36,6 +36,7 @@ _asniprintf_r (struct _reent *ptr,
       len = 0;
       buf = NULL;
     }
+  f._flags2 = 0;
   f._bf._base = f._p = (unsigned char *) buf;
   /* For now, inherit the 32-bit signed limit of FILE._bf._size.
      FIXME - it would be nice to rewrite sys/reent.h to support size_t
@@ -82,6 +83,7 @@ asniprintf (char *buf,
       len = 0;
       buf = NULL;
     }
+  f._flags2 = 0;
   f._bf._base = f._p = (unsigned char *) buf;
   /* For now, inherit the 32-bit signed limit of FILE._bf._size.
      FIXME - it would be nice to rewrite sys/reent.h to support size_t
diff --git a/newlib/libc/stdio/asnprintf.c b/newlib/libc/stdio/asnprintf.c
index f82556143e74..4c9ad586020a 100644
--- a/newlib/libc/stdio/asnprintf.c
+++ b/newlib/libc/stdio/asnprintf.c
@@ -36,6 +36,7 @@ _asnprintf_r (struct _reent *__restrict ptr,
       len = 0;
       buf = NULL;
     }
+  f._flags2 = 0;
   f._bf._base = f._p = (unsigned char *) buf;
   /* For now, inherit the 32-bit signed limit of FILE._bf._size.
      FIXME - it would be nice to rewrite sys/reent.h to support size_t
@@ -88,6 +89,7 @@ asnprintf (char *__restrict buf,
       len = 0;
       buf = NULL;
     }
+  f._flags2 = 0;
   f._bf._base = f._p = (unsigned char *) buf;
   /* For now, inherit the 32-bit signed limit of FILE._bf._size.
      FIXME - it would be nice to rewrite sys/reent.h to support size_t
diff --git a/newlib/libc/stdio/asprintf.c b/newlib/libc/stdio/asprintf.c
index 542e70e4c308..511d0e0f3f4a 100644
--- a/newlib/libc/stdio/asprintf.c
+++ b/newlib/libc/stdio/asprintf.c
@@ -35,6 +35,7 @@ _asprintf_r (struct _reent *ptr,
 
   /* mark a zero-length reallocatable buffer */
   f._flags = __SWR | __SSTR | __SMBF;
+  f._flags2 = 0;
   f._bf._base = f._p = NULL;
   f._bf._size = f._w = 0;
   f._file = -1;  /* No file. */
@@ -67,6 +68,7 @@ asprintf (char **__restrict strp,
 
   /* mark a zero-length reallocatable buffer */
   f._flags = __SWR | __SSTR | __SMBF;
+  f._flags2 = 0;
   f._bf._base = f._p = NULL;
   f._bf._size = f._w = 0;
   f._file = -1;  /* No file. */
diff --git a/newlib/libc/stdio/getc.c b/newlib/libc/stdio/getc.c
index c8781320e603..12bffaf8f79a 100644
--- a/newlib/libc/stdio/getc.c
+++ b/newlib/libc/stdio/getc.c
@@ -81,6 +81,9 @@ _getc_r (struct _reent *ptr,
 {
   int result;
   CHECK_INIT (ptr, fp);
+  if (ORIENT (fp, -1) != -1)
+    return EOF;
+
   _newlib_flockfile_start (fp);
   result = __sgetc_r (ptr, fp);
   _newlib_flockfile_end (fp);
@@ -96,6 +99,9 @@ getc (register FILE *fp)
   struct _reent *reent = _REENT;
 
   CHECK_INIT (reent, fp);
+  if (ORIENT (fp, -1) != -1)
+    return EOF;
+
   _newlib_flockfile_start (fp);
   result = __sgetc_r (reent, fp);
   _newlib_flockfile_end (fp);
diff --git a/newlib/libc/stdio/refill.c b/newlib/libc/stdio/refill.c
index c1ef7e120b71..cd71ed1529ba 100644
--- a/newlib/libc/stdio/refill.c
+++ b/newlib/libc/stdio/refill.c
@@ -43,9 +43,6 @@ __srefill_r (struct _reent * ptr,
 
   CHECK_INIT (ptr, fp);
 
-  if (ORIENT (fp, -1) != -1)
-    return EOF;
-
   fp->_r = 0;			/* largely a convenience for callers */
 
   /* SysV does not make this test; take it out for compatibility */
diff --git a/newlib/libc/stdio/siprintf.c b/newlib/libc/stdio/siprintf.c
index bd29edd88110..b337925979bb 100644
--- a/newlib/libc/stdio/siprintf.c
+++ b/newlib/libc/stdio/siprintf.c
@@ -108,6 +108,7 @@ _siprintf_r (struct _reent *ptr,
   FILE f;
 
   f._flags = __SWR | __SSTR;
+  f._flags2 = 0;
   f._bf._base = f._p = (unsigned char *) str;
   f._bf._size = f._w = INT_MAX;
   f._file = -1;  /* No file. */
@@ -129,6 +130,7 @@ siprintf (char *str,
   FILE f;
 
   f._flags = __SWR | __SSTR;
+  f._flags2 = 0;
   f._bf._base = f._p = (unsigned char *) str;
   f._bf._size = f._w = INT_MAX;
   f._file = -1;  /* No file. */
diff --git a/newlib/libc/stdio/siscanf.c b/newlib/libc/stdio/siscanf.c
index b88b81088818..4c1363d0a766 100644
--- a/newlib/libc/stdio/siscanf.c
+++ b/newlib/libc/stdio/siscanf.c
@@ -90,6 +90,7 @@ siscanf (const char *str,
   FILE f;
 
   f._flags = __SRD | __SSTR;
+  f._flags2 = 0;
   f._bf._base = f._p = (unsigned char *) str;
   f._bf._size = f._r = strlen (str);
   f._read = __seofread;
@@ -114,6 +115,7 @@ _siscanf_r (struct _reent *ptr,
   FILE f;
 
   f._flags = __SRD | __SSTR;
+  f._flags2 = 0;
   f._bf._base = f._p = (unsigned char *) str;
   f._bf._size = f._r = strlen (str);
   f._read = __seofread;
diff --git a/newlib/libc/stdio/sniprintf.c b/newlib/libc/stdio/sniprintf.c
index b05ffcb12da9..852713f1ffb8 100644
--- a/newlib/libc/stdio/sniprintf.c
+++ b/newlib/libc/stdio/sniprintf.c
@@ -42,6 +42,7 @@ _sniprintf_r (struct _reent *ptr,
       return EOF;
     }
   f._flags = __SWR | __SSTR;
+  f._flags2 = 0;
   f._bf._base = f._p = (unsigned char *) str;
   f._bf._size = f._w = (size > 0 ? size - 1 : 0);
   f._file = -1;  /* No file. */
@@ -73,6 +74,7 @@ sniprintf (char *str,
       return EOF;
     }
   f._flags = __SWR | __SSTR;
+  f._flags2 = 0;
   f._bf._base = f._p = (unsigned char *) str;
   f._bf._size = f._w = (size > 0 ? size - 1 : 0);
   f._file = -1;  /* No file. */
diff --git a/newlib/libc/stdio/snprintf.c b/newlib/libc/stdio/snprintf.c
index 4a3084beacb3..b6130b998393 100644
--- a/newlib/libc/stdio/snprintf.c
+++ b/newlib/libc/stdio/snprintf.c
@@ -41,6 +41,7 @@ _snprintf_r (struct _reent *ptr,
       return EOF;
     }
   f._flags = __SWR | __SSTR;
+  f._flags2 = 0;
   f._bf._base = f._p = (unsigned char *) str;
   f._bf._size = f._w = (size > 0 ? size - 1 : 0);
   f._file = -1;  /* No file. */
@@ -78,6 +79,7 @@ snprintf (char *__restrict str,
       return EOF;
     }
   f._flags = __SWR | __SSTR;
+  f._flags2 = 0;
   f._bf._base = f._p = (unsigned char *) str;
   f._bf._size = f._w = (size > 0 ? size - 1 : 0);
   f._file = -1;  /* No file. */
diff --git a/newlib/libc/stdio/sprintf.c b/newlib/libc/stdio/sprintf.c
index be66ec6f5d73..92cae46189df 100644
--- a/newlib/libc/stdio/sprintf.c
+++ b/newlib/libc/stdio/sprintf.c
@@ -584,6 +584,7 @@ _sprintf_r (struct _reent *ptr,
   FILE f;
 
   f._flags = __SWR | __SSTR;
+  f._flags2 = 0;
   f._bf._base = f._p = (unsigned char *) str;
   f._bf._size = f._w = INT_MAX;
   f._file = -1;  /* No file. */
@@ -611,6 +612,7 @@ sprintf (char *__restrict str,
   FILE f;
 
   f._flags = __SWR | __SSTR;
+  f._flags2 = 0;
   f._bf._base = f._p = (unsigned char *) str;
   f._bf._size = f._w = INT_MAX;
   f._file = -1;  /* No file. */
diff --git a/newlib/libc/stdio/sscanf.c b/newlib/libc/stdio/sscanf.c
index 9c50361bc026..52145fc46c20 100644
--- a/newlib/libc/stdio/sscanf.c
+++ b/newlib/libc/stdio/sscanf.c
@@ -429,6 +429,7 @@ sscanf (const char *__restrict str,
   FILE f;
 
   f._flags = __SRD | __SSTR;
+  f._flags2 = 0;
   f._bf._base = f._p = (unsigned char *) str;
   f._bf._size = f._r = strlen (str);
   f._read = __seofread;
@@ -459,6 +460,7 @@ _sscanf_r (struct _reent *ptr,
   FILE f;
 
   f._flags = __SRD | __SSTR;
+  f._flags2 = 0;
   f._bf._base = f._p = (unsigned char *) str;
   f._bf._size = f._r = strlen (str);
   f._read = __seofread;
diff --git a/newlib/libc/stdio/swprintf.c b/newlib/libc/stdio/swprintf.c
index 634b22beb909..e590b522ced8 100644
--- a/newlib/libc/stdio/swprintf.c
+++ b/newlib/libc/stdio/swprintf.c
@@ -568,6 +568,7 @@ _swprintf_r (struct _reent *ptr,
       return EOF;
     }
   f._flags = __SWR | __SSTR;
+  f._flags2 = 0;
   f._bf._base = f._p = (unsigned char *) str;
   f._bf._size = f._w = (size > 0 ? (size - 1) * sizeof (wchar_t) : 0);
   f._file = -1;  /* No file. */
@@ -608,6 +609,7 @@ swprintf (wchar_t *__restrict str,
       return EOF;
     }
   f._flags = __SWR | __SSTR;
+  f._flags2 = 0;
   f._bf._base = f._p = (unsigned char *) str;
   f._bf._size = f._w = (size > 0 ? (size - 1) * sizeof (wchar_t) : 0);
   f._file = -1;  /* No file. */
diff --git a/newlib/libc/stdio/swscanf.c b/newlib/libc/stdio/swscanf.c
index 578ac3c93f76..0ef4d0ebde18 100644
--- a/newlib/libc/stdio/swscanf.c
+++ b/newlib/libc/stdio/swscanf.c
@@ -422,6 +422,7 @@ swscanf (const wchar_t *__restrict str, const wchar_t *__restrict fmt, ...)
   FILE f;
 
   f._flags = __SRD | __SSTR;
+  f._flags2 = 0;
   f._bf._base = f._p = (unsigned char *) str;
   f._bf._size = f._r = wcslen (str) * sizeof (wchar_t);
   f._read = __seofread;
@@ -446,6 +447,7 @@ _swscanf_r (struct _reent *ptr, const wchar_t *str, const wchar_t *fmt, ...)
   FILE f;
 
   f._flags = __SRD | __SSTR;
+  f._flags2 = 0;
   f._bf._base = f._p = (unsigned char *) str;
   f._bf._size = f._r = wcslen (str) * sizeof (wchar_t);
   f._read = __seofread;
diff --git a/newlib/libc/stdio/ungetc.c b/newlib/libc/stdio/ungetc.c
index 79914af0806e..8b10c8218616 100644
--- a/newlib/libc/stdio/ungetc.c
+++ b/newlib/libc/stdio/ungetc.c
@@ -125,12 +125,6 @@ _ungetc_r (struct _reent *rptr,
 
   _newlib_flockfile_start (fp);
 
-  if (ORIENT (fp, -1) != -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 18636d773edd..1d65a1ef039a 100644
--- a/newlib/libc/stdio/ungetwc.c
+++ b/newlib/libc/stdio/ungetwc.c
@@ -112,5 +112,6 @@ ungetwc (wint_t wc,
   struct _reent *reent = _REENT;
 
   CHECK_INIT (reent, fp);
+  ORIENT (fp, 1);
   return _ungetwc_r (reent, wc, fp);
 }
diff --git a/newlib/libc/stdio/vasiprintf.c b/newlib/libc/stdio/vasiprintf.c
index 1deb2dbcf3d8..d9c11eaf503f 100644
--- a/newlib/libc/stdio/vasiprintf.c
+++ b/newlib/libc/stdio/vasiprintf.c
@@ -50,6 +50,7 @@ _vasiprintf_r (struct _reent *ptr,
   FILE f;
 
   f._flags = __SWR | __SSTR | __SMBF ;
+  f._flags2 = 0;
   f._bf._base = f._p = NULL;
   f._bf._size = f._w = 0;
   f._file = -1;  /* No file. */
diff --git a/newlib/libc/stdio/vasniprintf.c b/newlib/libc/stdio/vasniprintf.c
index b24cd96fb625..c85ca06e8c1a 100644
--- a/newlib/libc/stdio/vasniprintf.c
+++ b/newlib/libc/stdio/vasniprintf.c
@@ -36,6 +36,7 @@ _vasniprintf_r (struct _reent *ptr,
       len = 0;
       buf = NULL;
     }
+  f._flags2 = 0;
   f._bf._base = f._p = (unsigned char *) buf;
   /* For now, inherit the 32-bit signed limit of FILE._bf._size.
      FIXME - it would be nice to rewrite sys/reent.h to support size_t
diff --git a/newlib/libc/stdio/vasnprintf.c b/newlib/libc/stdio/vasnprintf.c
index b3787ec7a1fa..02f55caa9a95 100644
--- a/newlib/libc/stdio/vasnprintf.c
+++ b/newlib/libc/stdio/vasnprintf.c
@@ -36,6 +36,7 @@ _vasnprintf_r (struct _reent *ptr,
       len = 0;
       buf = NULL;
     }
+  f._flags2 = 0;
   f._bf._base = f._p = (unsigned char *) buf;
   /* For now, inherit the 32-bit signed limit of FILE._bf._size.
      FIXME - it would be nice to rewrite sys/reent.h to support size_t
diff --git a/newlib/libc/stdio/vasprintf.c b/newlib/libc/stdio/vasprintf.c
index d7ec9c8400ef..b466936bd94b 100644
--- a/newlib/libc/stdio/vasprintf.c
+++ b/newlib/libc/stdio/vasprintf.c
@@ -56,6 +56,7 @@ _vasprintf_r (struct _reent *ptr,
   FILE f;
 
   f._flags = __SWR | __SSTR | __SMBF ;
+  f._flags2 = 0;
   f._bf._base = f._p = NULL;
   f._bf._size = f._w = 0;
   f._file = -1;  /* No file. */
diff --git a/newlib/libc/stdio/vsiprintf.c b/newlib/libc/stdio/vsiprintf.c
index 8128cb772684..af07ad85e8cd 100644
--- a/newlib/libc/stdio/vsiprintf.c
+++ b/newlib/libc/stdio/vsiprintf.c
@@ -50,6 +50,7 @@ _vsiprintf_r (struct _reent *ptr,
   FILE f;
 
   f._flags = __SWR | __SSTR;
+  f._flags2 = 0;
   f._bf._base = f._p = (unsigned char *) str;
   f._bf._size = f._w = INT_MAX;
   f._file = -1;  /* No file. */
diff --git a/newlib/libc/stdio/vsiscanf.c b/newlib/libc/stdio/vsiscanf.c
index d7135ef71278..4e28d1079390 100644
--- a/newlib/libc/stdio/vsiscanf.c
+++ b/newlib/libc/stdio/vsiscanf.c
@@ -49,6 +49,7 @@ _vsiscanf_r (struct _reent *ptr,
   FILE f;
 
   f._flags = __SRD | __SSTR;
+  f._flags2 = 0;
   f._bf._base = f._p = (unsigned char *) str;
   f._bf._size = f._r = strlen (str);
   f._read = __seofread;
diff --git a/newlib/libc/stdio/vsniprintf.c b/newlib/libc/stdio/vsniprintf.c
index e7625ffd5345..a50a0a993115 100644
--- a/newlib/libc/stdio/vsniprintf.c
+++ b/newlib/libc/stdio/vsniprintf.c
@@ -58,6 +58,7 @@ _vsniprintf_r (struct _reent *ptr,
       return EOF;
     }
   f._flags = __SWR | __SSTR;
+  f._flags2 = 0;
   f._bf._base = f._p = (unsigned char *) str;
   f._bf._size = f._w = (size > 0 ? size - 1 : 0);
   f._file = -1;  /* No file. */
diff --git a/newlib/libc/stdio/vsnprintf.c b/newlib/libc/stdio/vsnprintf.c
index ecf82eaefff7..d9c72b526ba5 100644
--- a/newlib/libc/stdio/vsnprintf.c
+++ b/newlib/libc/stdio/vsnprintf.c
@@ -64,6 +64,7 @@ _vsnprintf_r (struct _reent *ptr,
       return EOF;
     }
   f._flags = __SWR | __SSTR;
+  f._flags2 = 0;
   f._bf._base = f._p = (unsigned char *) str;
   f._bf._size = f._w = (size > 0 ? size - 1 : 0);
   f._file = -1;  /* No file. */
diff --git a/newlib/libc/stdio/vsprintf.c b/newlib/libc/stdio/vsprintf.c
index 4d51fd0dc03f..4d4619fd0324 100644
--- a/newlib/libc/stdio/vsprintf.c
+++ b/newlib/libc/stdio/vsprintf.c
@@ -56,6 +56,7 @@ _vsprintf_r (struct _reent *ptr,
   FILE f;
 
   f._flags = __SWR | __SSTR;
+  f._flags2 = 0;
   f._bf._base = f._p = (unsigned char *) str;
   f._bf._size = f._w = INT_MAX;
   f._file = -1;  /* No file. */
diff --git a/newlib/libc/stdio/vsscanf.c b/newlib/libc/stdio/vsscanf.c
index e9e84088f7cf..6150f541bb24 100644
--- a/newlib/libc/stdio/vsscanf.c
+++ b/newlib/libc/stdio/vsscanf.c
@@ -55,6 +55,7 @@ _vsscanf_r (struct _reent *ptr,
   FILE f;
 
   f._flags = __SRD | __SSTR;
+  f._flags2 = 0;
   f._bf._base = f._p = (unsigned char *) str;
   f._bf._size = f._r = strlen (str);
   f._read = __seofread;
diff --git a/newlib/libc/stdio/vswprintf.c b/newlib/libc/stdio/vswprintf.c
index 08a8545baef8..ab2e52ff4921 100644
--- a/newlib/libc/stdio/vswprintf.c
+++ b/newlib/libc/stdio/vswprintf.c
@@ -46,6 +46,7 @@ _vswprintf_r (struct _reent *ptr,
       return EOF;
     }
   f._flags = __SWR | __SSTR;
+  f._flags2 = 0;
   f._bf._base = f._p = (unsigned char *) str;
   f._bf._size = f._w = (size > 0 ? (size - 1) * sizeof (wchar_t) : 0);
   f._file = -1;  /* No file. */
-- 
2.41.0
:q

Re: [PATCH v2] newlib: libc: Fix bugs in the commit 3d94e07c49b5.

[Brian Inglis]

On 2023-11-15 09:31, Corinna Vinschen wrote:
> Also, as in GLibC, do not set orientation in ungetc, and only set, but
> do not check orientation in ungetwc.

Function ungetwc is a wide character input/output function, as ungetc is a byte 
input/output function, as per N3096 C202X CD2 2023-04 7.23 Input/output 
<stdio.h> 7.23.1 Introduction #6:

"6 The input/output functions are given the following collective terms:
— The wide character input functions — those functions described in 7.31 that 
perform input into wide characters and wide strings: fgetwc, fgetws, getwc, 
getwchar, fwscanf, wscanf, vfwscanf, and vwscanf.
— The wide character output functions — those functions described in 7.31 that 
perform output from wide characters and wide strings: fputwc, fputws, putwc, 
putwchar, fwprintf, wprintf, vfwprintf, and vwprintf.
— The wide character input/output functions — the union of the ungetwc function, 
the wide character input functions, and the wide character output functions.
— The byte input/output functions — those functions described in this subclause 
that perform input/output: fgetc, fgets, fprintf, fputc, fputs, fread, fscanf, 
fwrite, getc, getchar, printf, putc, putchar, puts, scanf, ungetc, vfprintf, 
vfscanf, vprintf, and vscanf."

and POSIX 202X Draft 3 concurs in 3.417 and 3.56.

All functions of incorrect orientation are invalid once orientation is set, so 
in ungetwc and ungetc, orientation should be checked, as in any of the input and 
output functions named, and return W/EOF if the the stream is the opposite 
orientation, as normal operation is not successful, as per the function 
definitions, and C202X CD2 7.23.2 Streams #4-5:

"4 Each stream has an orientation. After a stream is associated with an external 
file, but before any operations are performed on it, the stream is unoriented.
Once a wide character input/output function has been applied to an unoriented 
stream, the stream becomes a wide-oriented stream.
Similarly, once a byte input/output function has been applied to an unoriented 
stream, the stream becomes a byte-oriented stream.
Only a call to the freopen function or the fwide function can otherwise alter 
the orientation of a stream. (A successful call to freopen removes any orientation.)
5 Byte input/output functions shall not be applied to a wide-oriented stream and 
wide character input/output functions shall not be applied to a byte-oriented 
stream."

and POSIX 202X D3 concurs in 2.5.2 Stream Orientation and Encoding Rules.

In C202X CD2 Annex J Portability issues J.2 Undefined behavior specifies:

"(149) A byte input/output function is applied to a wide-oriented stream, or a 
wide character input/output function is applied to a byte-oriented stream (7.23.2)."

-- 
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: [PATCH v2] newlib: libc: Fix bugs in the commit 3d94e07c49b5.

[Corinna Vinschen]

On Nov 15 15:53, Brian Inglis wrote:
> On 2023-11-15 09:31, Corinna Vinschen wrote:
> > Also, as in GLibC, do not set orientation in ungetc, and only set, but
> > do not check orientation in ungetwc.
> 
> Function ungetwc is a wide character input/output function, as ungetc is a
> byte input/output function, as per N3096 C202X CD2 2023-04 7.23 Input/output
> <stdio.h> 7.23.1 Introduction #6:

Yes?  The idea was to follow GLibC behaviour.  The reason that GLibC
ungetc does not set and check orientation is compatibility with history.

https://nvlpubs.nist.gov/nistpubs/Legacy/FIPS/fipspub160.pdf, page 145
suggests that one can call ungetc on any stream.  Ungetwc hadn't been
introduced at the time.  Therefore ungetc can't presuppose an
orientation and also shouldn't set it.

As for ungetwc only setting the orientation but not checking it, I
didn't get a conclusive answer yet if that's deliberate or a bug.
Either way, we can do it as GLibC does, or we can check the orientation.
I just followed GLibC behaviour for now ¯\_(ツ)_/¯.


Corinna

Re: [PATCH v2] newlib: libc: Fix bugs in the commit 3d94e07c49b5.

[Takashi Yano]

Hi Corinna,

On Wed, 15 Nov 2023 17:31:39 +0100
Corinna Vinschen wrote:
> Hi Takashi,
> 
> On Nov 13 14:07, Corinna Vinschen wrote:
> > On Nov 11 01:09, Takashi Yano wrote:
> > > diff --git a/newlib/libc/stdio/ungetc.c b/newlib/libc/stdio/ungetc.c
> > > index 79914af08..5053fd6c4 100644
> > > --- a/newlib/libc/stdio/ungetc.c
> > > +++ b/newlib/libc/stdio/ungetc.c
> > > @@ -125,12 +125,6 @@ _ungetc_r (struct _reent *rptr,
> > >  
> > >    _newlib_flockfile_start (fp);
> > >  
> > > -  if (ORIENT (fp, -1) != -1)
> > > -    {
> > > -      _newlib_flockfile_exit (fp);
> > > -      return EOF;
> > > -    }
> > > -
> > >    /* After ungetc, we won't be at eof anymore */
> > >    fp->_flags &= ~__SEOF;
> > >  
> > > @@ -213,6 +207,8 @@ int
> > >  ungetc (int c,
> > >         register FILE *fp)
> > >  {
> > > +  if (ORIENT (fp, -1) != -1)
> > > +    return EOF;
> > >    return _ungetc_r (_REENT, c, fp);
> > >  }
> > >  #endif /* !_REENT_ONLY */
> > 
> > As per the reasoning in terms of old standards, we shouldn't check or set
> > the orientation in ungetc at all.
> 
> Given all that has been discussed, I attached a followup proposal.
> I hope that's ok.  We may want to release 3.4.10 in the next weeks.

LGTM, except for:
1) Shouldn't we add flags2 = 0 also for _fmemopen_r(), _fopencookie_r(),
   _funopen_r() and internal_open_memstream_r()?
2) __swbuf_r() has ORIENT (fp, -1) call, however, result is not checked.

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

Re: [PATCH v2] newlib: libc: Fix bugs in the commit 3d94e07c49b5.

[Corinna Vinschen]

Hi Takashi,

On Nov 16 21:27, Takashi Yano wrote:
> Hi Corinna,
> 
> On Wed, 15 Nov 2023 17:31:39 +0100
> Corinna Vinschen wrote:
> > Hi Takashi,
> > 
> > On Nov 13 14:07, Corinna Vinschen wrote:
> > > On Nov 11 01:09, Takashi Yano wrote:
> > > > diff --git a/newlib/libc/stdio/ungetc.c b/newlib/libc/stdio/ungetc.c
> > > > index 79914af08..5053fd6c4 100644
> > > > --- a/newlib/libc/stdio/ungetc.c
> > > > +++ b/newlib/libc/stdio/ungetc.c
> > > > @@ -125,12 +125,6 @@ _ungetc_r (struct _reent *rptr,
> > > >  
> > > >    _newlib_flockfile_start (fp);
> > > >  
> > > > -  if (ORIENT (fp, -1) != -1)
> > > > -    {
> > > > -      _newlib_flockfile_exit (fp);
> > > > -      return EOF;
> > > > -    }
> > > > -
> > > >    /* After ungetc, we won't be at eof anymore */
> > > >    fp->_flags &= ~__SEOF;
> > > >  
> > > > @@ -213,6 +207,8 @@ int
> > > >  ungetc (int c,
> > > >         register FILE *fp)
> > > >  {
> > > > +  if (ORIENT (fp, -1) != -1)
> > > > +    return EOF;
> > > >    return _ungetc_r (_REENT, c, fp);
> > > >  }
> > > >  #endif /* !_REENT_ONLY */
> > > 
> > > As per the reasoning in terms of old standards, we shouldn't check or set
> > > the orientation in ungetc at all.
> > 
> > Given all that has been discussed, I attached a followup proposal.
> > I hope that's ok.  We may want to release 3.4.10 in the next weeks.
> 
> LGTM, except for:
> 1) Shouldn't we add flags2 = 0 also for _fmemopen_r(), _fopencookie_r(),
>    _funopen_r() and internal_open_memstream_r()?

Not necessary.  All three functions get the FILE* from a call to __sfp,
which always sets _flags2 to 0 anyway.

> 2) __swbuf_r() has ORIENT (fp, -1) call, however, result is not checked.

Good catch.  Looks like setting the orientation at this point is
wrong.  __swbuf_r is called via the __sputc_r macro in stdio.h.
But __sputc_r is also called from __fputwc as well as, potentially,
from _fputws_r.

AFAICS, we already set the orientation from the calling functions
_putc_r (covering putchar, putc, fputc) and from _puts_r as
well as _fputs_r (covering puts, fputs).

On the wide-char side, we set the orientation from _fputwc_r
(covering putwchar, putwc, fputwc) and from _fputws_r (covering
fputws).

So I *think* we can safely remove the ORIENT call from __swbuf_r,
or did I miss some other way to call __swbuf_r?


Thanks,
Corinna

Re: [PATCH v2] newlib: libc: Fix bugs in the commit 3d94e07c49b5.

[Corinna Vinschen]

On Nov 16 14:19, Corinna Vinschen wrote:
> On Nov 16 21:27, Takashi Yano wrote:
> > 2) __swbuf_r() has ORIENT (fp, -1) call, however, result is not checked.
> 
> Good catch.  Looks like setting the orientation at this point is
> wrong.  __swbuf_r is called via the __sputc_r macro in stdio.h.
> But __sputc_r is also called from __fputwc as well as, potentially,
> from _fputws_r.
> 
> AFAICS, we already set the orientation from the calling functions
> _putc_r (covering putchar, putc, fputc) and from _puts_r as
> well as _fputs_r (covering puts, fputs).
> 
> On the wide-char side, we set the orientation from _fputwc_r
> (covering putwchar, putwc, fputwc) and from _fputws_r (covering
> fputws).
> 
> So I *think* we can safely remove the ORIENT call from __swbuf_r,
> or did I miss some other way to call __swbuf_r?

Yes, I did.  The _getchar_unlocked and _putchar_unlocked macros as well
as the __SINGLE_THREAD__ getc and putc macros(*).  I also missed the
putc{har}_unlocked and getc{har}_unlocked functions.

They all call __sgetc_r or __sputs_r, both being macros in stdio.h.
The underlying functions __srget_r and __swbuf_r are shared between
byte and wide-char orientation functions.

Drat.  Looks like we really have to do the ORIENT stuff inside the
__sgetc_r/__sputs_r macros...


Corinna



(*) I'm deliberately ignoring the fast_putc macro. It's so much beyond
    standards and old as dirt.  I wonder if we shouldn't remove it.

Re: [PATCH v2] newlib: libc: Fix bugs in the commit 3d94e07c49b5.

[Corinna Vinschen]

On Nov 16 14:48, Corinna Vinschen wrote:
> On Nov 16 14:19, Corinna Vinschen wrote:
> > On Nov 16 21:27, Takashi Yano wrote:
> > > 2) __swbuf_r() has ORIENT (fp, -1) call, however, result is not checked.
> > 
> > Good catch.  Looks like setting the orientation at this point is
> > wrong.  __swbuf_r is called via the __sputc_r macro in stdio.h.
> > But __sputc_r is also called from __fputwc as well as, potentially,
> > from _fputws_r.
> > 
> > AFAICS, we already set the orientation from the calling functions
> > _putc_r (covering putchar, putc, fputc) and from _puts_r as
> > well as _fputs_r (covering puts, fputs).
> > 
> > On the wide-char side, we set the orientation from _fputwc_r
> > (covering putwchar, putwc, fputwc) and from _fputws_r (covering
> > fputws).
> > 
> > So I *think* we can safely remove the ORIENT call from __swbuf_r,
> > or did I miss some other way to call __swbuf_r?
> 
> Yes, I did.  The _getchar_unlocked and _putchar_unlocked macros as well
> as the __SINGLE_THREAD__ getc and putc macros(*).  I also missed the
> putc{har}_unlocked and getc{har}_unlocked functions.
> 
> They all call __sgetc_r or __sputs_r, both being macros in stdio.h.
> The underlying functions __srget_r and __swbuf_r are shared between
> byte and wide-char orientation functions.
> 
> Drat.  Looks like we really have to do the ORIENT stuff inside the
> __sgetc_r/__sputs_r macros...

That's not feasible.  The only way out as I see it is that we don't
share the underlying functions and macros between byte and wide-char
functions:

- _fputwc_r and _fputws_r must not call __sputc_r from stdio.h, they
  should get their own version.

- The underlying functions __srget_r and __swbuf_r have to be split into
  two functions each, or they have to get an additional parameter
  deciding how to set the orientation.

- In turn, we can remove most of the ORIENT calls in the calling functions.


Corinna

Re: [PATCH v2] newlib: libc: Fix bugs in the commit 3d94e07c49b5.

[Corinna Vinschen]

On Nov 16 15:08, Corinna Vinschen wrote:
> On Nov 16 14:48, Corinna Vinschen wrote:
> > On Nov 16 14:19, Corinna Vinschen wrote:
> > > On Nov 16 21:27, Takashi Yano wrote:
> > > > 2) __swbuf_r() has ORIENT (fp, -1) call, however, result is not checked.
> > > 
> > > Good catch.  Looks like setting the orientation at this point is
> > > wrong.  __swbuf_r is called via the __sputc_r macro in stdio.h.
> > > But __sputc_r is also called from __fputwc as well as, potentially,
> > > from _fputws_r.
> > > 
> > > AFAICS, we already set the orientation from the calling functions
> > > _putc_r (covering putchar, putc, fputc) and from _puts_r as
> > > well as _fputs_r (covering puts, fputs).
> > > 
> > > On the wide-char side, we set the orientation from _fputwc_r
> > > (covering putwchar, putwc, fputwc) and from _fputws_r (covering
> > > fputws).
> > > 
> > > So I *think* we can safely remove the ORIENT call from __swbuf_r,
> > > or did I miss some other way to call __swbuf_r?
> > 
> > Yes, I did.  The _getchar_unlocked and _putchar_unlocked macros as well
> > as the __SINGLE_THREAD__ getc and putc macros(*).  I also missed the
> > putc{har}_unlocked and getc{har}_unlocked functions.
> > 
> > They all call __sgetc_r or __sputs_r, both being macros in stdio.h.
> > The underlying functions __srget_r and __swbuf_r are shared between
> > byte and wide-char orientation functions.
> > 
> > Drat.  Looks like we really have to do the ORIENT stuff inside the
> > __sgetc_r/__sputs_r macros...
> 
> That's not feasible.  The only way out as I see it is that we don't
> share the underlying functions and macros between byte and wide-char
> functions:
> 
> - _fputwc_r and _fputws_r must not call __sputc_r from stdio.h, they
>   should get their own version.
> 
> - The underlying functions __srget_r and __swbuf_r have to be split into
>   two functions each, or they have to get an additional parameter
>   deciding how to set the orientation.
> 
> - In turn, we can remove most of the ORIENT calls in the calling functions.

I applied matching patches. After checking the code for as couple of
hours I'm pretty sure __srget_r doesn't have to be split because all
wide-char oriented functions call __srefill_r directly.

I seriously hope I didn't miss anything.  Feedback appreciated.


Thanks,
Corinna