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

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.

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

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;
 }

On Tue, 14 Aug 2018, Florian Weimer wrote:

>On 08/14/2018 03:22 PM, Gabriel F. T. Gomes wrote:
>>   
>> 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.

OK.

>I'm also adding a test in the attached patch.
>
>
>+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");

'git am' is complaining about the indentation in this block.  Other than
that, this version (with the test case and the removal of the call to
fputs_unlocked) looks good to me, too.

On 08/14/2018 04:55 PM, Gabriel F. T. Gomes wrote:
>> +  one_test ("with errno", E2BIG,
>> +            L"tst-warn-wide: with errno with padding " PADDING
>> +            ": Argument list too long\n");

> 'git am' is complaining about the indentation in this block.  Other than
> that, this version (with the test case and the removal of the call to
> fputs_unlocked) looks good to me, too.

That's odd.  It's all spaces.  Or is this about the split wide string 
literal?

Thanks for the review.

Florian

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