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

Message ID 20180814120325.5D500405971E1@oldenburg.str.redhat.com
State Superseded
Headers

Commit Message

Florian Weimer Aug. 14, 2018, 12:03 p.m. UTC
  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.
  

Comments

Gabriel F. T. Gomes Aug. 14, 2018, 1:22 p.m. UTC | #1
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)?

It appears so, and your patch looks good to me.

>--- 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).
  
Joseph Myers Aug. 14, 2018, 7:31 p.m. UTC | #2
On Tue, 14 Aug 2018, Gabriel F. T. Gomes wrote:

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

Not only is it used in gnulib, but we have at least two open bugs for 
issues that only apply for such use outside glibc (10412, 17089).
  

Patch

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/err.c b/misc/err.c
index 2b836e8358..790ee127d5 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)
+    __vfxprintf (stderr, format, ap);
+  __fxprintf (stderr, "\n");
   funlockfile (stderr);
 }
 libc_hidden_def (vwarnx)
@@ -109,28 +55,14 @@  vwarn (const char *format, __gnuc_va_list ap)
   int error = errno;
 
   flockfile (stderr);
-  if (_IO_fwide (stderr, 0) > 0)
+  __fxprintf (stderr, "%s: ", __progname);
+  if (format)
     {
-      __fwprintf (stderr, L"%s: ", __progname);
-      if (format)
-	{
-	  convert_and_print (format, ap);
-	  fputws_unlocked (L": ", stderr);
-	}
-      __set_errno (error);
-      __fwprintf (stderr, L"%m\n");
-    }
-  else
-    {
-      fprintf (stderr, "%s: ", __progname);
-      if (format)
-	{
-	  vfprintf (stderr, format, ap);
-	  fputs_unlocked (": ", stderr);
-	}
-      __set_errno (error);
-      fprintf (stderr, "%m\n");
+      __vfxprintf (stderr, format, ap);
+      fputs_unlocked (": ", stderr);
     }
+  __set_errno (error);
+  __fxprintf (stderr, "%m\n");
   funlockfile (stderr);
 }
 libc_hidden_def (vwarn)
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/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;
 }