Re: [PATCH] error, warn, warnx: Use __fxprintf for wide printing [BZ #23519]

[Florian Weimer <fweimer at redhat dot com>]

On 08/14/2018 03:22 PM, Gabriel F. T. Gomes wrote:
> On Tue, 14 Aug 2018, Florian Weimer wrote:
>
> > void
> > vwarnx (const char *format, __gnuc_va_list ap)
> > {
> >    flockfile (stderr);
> > -  if (_IO_fwide (stderr, 0) > 0)
> > -    {
> > -      __fwprintf (stderr, L"%s: ", __progname);
> > -      convert_and_print (format, ap);
> > -      putwc_unlocked (L'\n', stderr);
> > -    }
> > -  else
> > -    {
> > -      fprintf (stderr, "%s: ", __progname);
> > -      if (format)
> > -	vfprintf (stderr, format, ap);
> > -      putc_unlocked ('\n', stderr);
> > -    }
> > +  __fxprintf (stderr, "%s: ", __progname);
> > +  if (format)
> > +    __vfxprintf (stderr, format, ap);
> > +  __fxprintf (stderr, "\n");
>
> Is it true that you can remove the call to 'convert_and_print' because
> __fxprintf and __vfxprintf already check the wideness of the output and
> convert (indirectly via locked_vfxprintf)?

Correct.  I failed to convert an fputs_unlocked call.

I'm also adding a test in the attached patch.

> > --- a/misc/error.c
> > +++ b/misc/error.c
> > @@ -203,72 +203,14 @@ static void _GL_ATTRIBUTE_FORMAT_PRINTF (3, 0) _GL_ARG_NONNULL ((3))
> > error_tail (int status, int errnum, const char *message, va_list args)
> > {
> > #if _LIBC
> > +  int ret = __vfxprintf (stderr, message, args);
> > +  if (ret < 0 && errno == ENOMEM && _IO_fwide (stderr, 0) > 0)
> > +    /* Leave a trace in case the heap allocation of the message string
> > +       failed.  */
> > +    fputws_unlocked (L"out of memory\n", stderr);
> > +#else
> > +  vfprintf (stderr, message, args);
> > #endif
>
> Just out of curiosity, how do we know that other programs use this code
> outside of glibc?  (assuming this is the reason for the _LIBC check).

In these cases, it is useful to check gnulib.  Often, there is a copy of 
the code there as well, and there is an expectation that we keep merging 
from gnulib (but this is not always possible).

Thanks,
Florian

Subject: [PATCH] error, warn, warnx: Use __fxprintf for wide printing [BZ #23519]
To: libc-alpha@sourceware.org

Also introduce the __vfxprintf function.

2018-08-14  Florian Weimer  <fweimer@redhat.com>

	[BZ #23519]
	* include/stdio.h (__vfxprintf): Declare.
	* stdio-common/fxprintf.c (__vfxprintf): New function.
	(__fxprintf): Call it.
	* misc/err.c (convert_and_print): Remove function.
	(vwarnx, vwarn): Call __fxprintf and __vfxprintf.
	* misc/error.c [_LIBC] (error_tail): Call __vfxprintf.
	* misc/Makefile (tests): Add tst-warn-wide.
	* misc/tst-warn-wide.c: New file.

diff --git a/include/stdio.h b/include/stdio.h
index 9162d4e247..49383fb2d7 100644
--- a/include/stdio.h
+++ b/include/stdio.h
@@ -126,6 +126,8 @@ extern int __fxprintf (FILE *__fp, const char *__fmt, ...)
      __attribute__ ((__format__ (__printf__, 2, 3))) attribute_hidden;
 extern int __fxprintf_nocancel (FILE *__fp, const char *__fmt, ...)
      __attribute__ ((__format__ (__printf__, 2, 3))) attribute_hidden;
+int __vfxprintf (FILE *__fp, const char *__fmt, __gnuc_va_list)
+  attribute_hidden;
 
 /* Read the next line from FP into BUFFER, of LENGTH bytes.  LINE will
    include the line terminator and a NUL terminator.  On success,
diff --git a/misc/Makefile b/misc/Makefile
index b7be2bc19a..9a87e81ae5 100644
--- a/misc/Makefile
+++ b/misc/Makefile
@@ -84,7 +84,7 @@ tests := tst-dirname tst-tsearch tst-fdset tst-efgcvt tst-mntent tst-hsearch \
 	 tst-error1 tst-pselect tst-insremque tst-mntent2 bug-hsearch1 \
 	 tst-mntent-blank-corrupt tst-mntent-blank-passno bug18240 \
 	 tst-preadvwritev tst-preadvwritev64 tst-makedev tst-empty \
-	 tst-preadvwritev2 tst-preadvwritev64v2
+	 tst-preadvwritev2 tst-preadvwritev64v2 tst-warn-wide
 
 tests-internal := tst-atomic tst-atomic-long tst-allocate_once
 tests-static := tst-empty
diff --git a/misc/err.c b/misc/err.c
index 2b836e8358..b6afe65516 100644
--- a/misc/err.c
+++ b/misc/err.c
@@ -37,68 +37,14 @@ extern char *__progname;
   va_end (ap);								      \
 }
 
-static void
-convert_and_print (const char *format, __gnuc_va_list ap)
-{
-#define ALLOCA_LIMIT	2000
-  size_t len;
-  wchar_t *wformat = NULL;
-  mbstate_t st;
-  size_t res;
-  const char *tmp;
-
-  if (format == NULL)
-    return;
-
-  len = strlen (format) + 1;
-
-  do
-    {
-      if (len < ALLOCA_LIMIT)
-	wformat = (wchar_t *) alloca (len * sizeof (wchar_t));
-      else
-	{
-	  if (wformat != NULL && len / 2 < ALLOCA_LIMIT)
-	    wformat = NULL;
-
-	  wformat = (wchar_t *) realloc (wformat, len * sizeof (wchar_t));
-
-	  if (wformat == NULL)
-	    {
-	      fputws_unlocked (L"out of memory\n", stderr);
-	      return;
-	    }
-	}
-
-      memset (&st, '\0', sizeof (st));
-      tmp =format;
-    }
-  while ((res = __mbsrtowcs (wformat, &tmp, len, &st)) == len);
-
-  if (res == (size_t) -1)
-    /* The string cannot be converted.  */
-    wformat = (wchar_t *) L"???";
-
-  __vfwprintf (stderr, wformat, ap);
-}
-
 void
 vwarnx (const char *format, __gnuc_va_list ap)
 {
   flockfile (stderr);
-  if (_IO_fwide (stderr, 0) > 0)
-    {
-      __fwprintf (stderr, L"%s: ", __progname);
-      convert_and_print (format, ap);
-      putwc_unlocked (L'\n', stderr);
-    }
-  else
-    {
-      fprintf (stderr, "%s: ", __progname);
-      if (format)
-	vfprintf (stderr, format, ap);
-      putc_unlocked ('\n', stderr);
-    }
+  __fxprintf (stderr, "%s: ", __progname);
+  if (format != NULL)
+    __vfxprintf (stderr, format, ap);
+  __fxprintf (stderr, "\n");
   funlockfile (stderr);
 }
 libc_hidden_def (vwarnx)
@@ -109,27 +55,17 @@ vwarn (const char *format, __gnuc_va_list ap)
   int error = errno;
 
   flockfile (stderr);
-  if (_IO_fwide (stderr, 0) > 0)
+  if (format != NULL)
     {
-      __fwprintf (stderr, L"%s: ", __progname);
-      if (format)
-	{
-	  convert_and_print (format, ap);
-	  fputws_unlocked (L": ", stderr);
-	}
+      __fxprintf (stderr, "%s: ", __progname);
+      __vfxprintf (stderr, format, ap);
       __set_errno (error);
-      __fwprintf (stderr, L"%m\n");
+      __fxprintf (stderr, ": %m\n");
     }
   else
     {
-      fprintf (stderr, "%s: ", __progname);
-      if (format)
-	{
-	  vfprintf (stderr, format, ap);
-	  fputs_unlocked (": ", stderr);
-	}
       __set_errno (error);
-      fprintf (stderr, "%m\n");
+      __fxprintf (stderr, "%s: %m\n", __progname);
     }
   funlockfile (stderr);
 }
diff --git a/misc/error.c b/misc/error.c
index 03378e2f2a..cb0de3af28 100644
--- a/misc/error.c
+++ b/misc/error.c
@@ -203,72 +203,14 @@ static void _GL_ATTRIBUTE_FORMAT_PRINTF (3, 0) _GL_ARG_NONNULL ((3))
 error_tail (int status, int errnum, const char *message, va_list args)
 {
 #if _LIBC
-  if (_IO_fwide (stderr, 0) > 0)
-    {
-      size_t len = strlen (message) + 1;
-      wchar_t *wmessage = NULL;
-      mbstate_t st;
-      size_t res;
-      const char *tmp;
-      bool use_malloc = false;
-
-      while (1)
-	{
-	  if (__libc_use_alloca (len * sizeof (wchar_t)))
-	    wmessage = (wchar_t *) alloca (len * sizeof (wchar_t));
-	  else
-	    {
-	      if (!use_malloc)
-		wmessage = NULL;
-
-	      wchar_t *p = (wchar_t *) realloc (wmessage,
-						len * sizeof (wchar_t));
-	      if (p == NULL)
-		{
-		  free (wmessage);
-		  fputws_unlocked (L"out of memory\n", stderr);
-		  return;
-		}
-	      wmessage = p;
-	      use_malloc = true;
-	    }
-
-	  memset (&st, '\0', sizeof (st));
-	  tmp = message;
-
-	  res = mbsrtowcs (wmessage, &tmp, len, &st);
-	  if (res != len)
-	    break;
-
-	  if (__builtin_expect (len >= SIZE_MAX / sizeof (wchar_t) / 2, 0))
-	    {
-	      /* This really should not happen if everything is fine.  */
-	      res = (size_t) -1;
-	      break;
-	    }
-
-	  len *= 2;
-	}
-
-      if (res == (size_t) -1)
-	{
-	  /* The string cannot be converted.  */
-	  if (use_malloc)
-	    {
-	      free (wmessage);
-	      use_malloc = false;
-	    }
-	  wmessage = (wchar_t *) L"???";
-	}
-
-      __vfwprintf (stderr, wmessage, args);
-
-      if (use_malloc)
-	free (wmessage);
-    }
-  else
+  int ret = __vfxprintf (stderr, message, args);
+  if (ret < 0 && errno == ENOMEM && _IO_fwide (stderr, 0) > 0)
+    /* Leave a trace in case the heap allocation of the message string
+       failed.  */
+    fputws_unlocked (L"out of memory\n", stderr);
+#else
+  vfprintf (stderr, message, args);
 #endif
-    vfprintf (stderr, message, args);
   va_end (args);
 
   ++error_message_count;
diff --git a/misc/tst-warn-wide.c b/misc/tst-warn-wide.c
new file mode 100644
index 0000000000..773bbba8d5
--- /dev/null
+++ b/misc/tst-warn-wide.c
@@ -0,0 +1,88 @@
+/* Test wide output conversion for warn.
+   Copyright (C) 2018 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <err.h>
+#include <errno.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <support/check.h>
+#include <support/xmemstream.h>
+#include <wchar.h>
+
+/* Used to trigger the large-string path in __fxprintf.  */
+#define PADDING \
+  "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX" \
+  "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX" \
+  "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"
+
+#define LPADDING \
+  L"XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"  \
+  "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX" \
+  "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"
+
+
+static void
+one_test (const char *message, int error_code, const wchar_t *expected)
+{
+  wchar_t *buffer = NULL;
+  size_t length = 0;
+  FILE *fp = open_wmemstream (&buffer, &length);
+  TEST_VERIFY_EXIT (fp != NULL);
+  FILE *old_stderr = stderr;
+  stderr = fp;
+  errno = error_code;
+  switch (error_code)
+    {
+    case E2BIG:
+      warn ("%s with padding " PADDING, message);
+      break;
+    case EAGAIN:
+      warn ("%s", message);
+      break;
+    case -1:
+      warnx ("%s", message);
+      break;
+    case -2:
+      warnx ("%s with padding " PADDING, message);
+      break;
+    }
+  stderr = old_stderr;
+  TEST_VERIFY_EXIT (!ferror (fp));
+  TEST_COMPARE (fclose (fp), 0);
+  if (wcscmp (buffer, expected) != 0)
+    FAIL_EXIT1 ("unexpected output: %ls", buffer);
+  free (buffer);
+}
+
+static int
+do_test (void)
+{
+  one_test ("no errno", -1,
+            L"tst-warn-wide: no errno\n");
+  one_test ("no errno", -2,
+            L"tst-warn-wide: no errno with padding " PADDING "\n");
+  one_test ("with errno", EAGAIN,
+            L"tst-warn-wide: with errno: Resource temporarily unavailable\n");
+  one_test ("with errno", E2BIG,
+            L"tst-warn-wide: with errno with padding " PADDING
+            ": Argument list too long\n");
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/stdio-common/fxprintf.c b/stdio-common/fxprintf.c
index c4a1146b20..8d02b71f91 100644
--- a/stdio-common/fxprintf.c
+++ b/stdio-common/fxprintf.c
@@ -61,19 +61,23 @@ locked_vfxprintf (FILE *fp, const char *fmt, va_list ap)
   return res;
 }
 
+int
+__vfxprintf (FILE *fp, const char *fmt, va_list ap)
+{
+  if (fp == NULL)
+    fp = stderr;
+  _IO_flockfile (fp);
+  int res = locked_vfxprintf (fp, fmt, ap);
+  _IO_funlockfile (fp);
+  return res;
+}
+
 int
 __fxprintf (FILE *fp, const char *fmt, ...)
 {
-  if (fp == NULL)
-    fp = stderr;
-
   va_list ap;
   va_start (ap, fmt);
-  _IO_flockfile (fp);
-
-  int res = locked_vfxprintf (fp, fmt, ap);
-
-  _IO_funlockfile (fp);
+  int res = __vfxprintf (fp, fmt, ap);
   va_end (ap);
   return res;
 }