getopt: clean up error reporting

getopt can print a whole bunch of error messages, and when used
standalone (from gnulib) it uses fprintf to do that.  But fprintf is a
cancellation point and getopt isn't, and also applying fprintf to a
stream in wide-character mode is not allowed.

glibc has an internal function called __fxprintf that writes a narrow
format string to a stream regardless of mode, but it only handles
ASCII format strings, and it's still a cancellation point.  getopt's
messages are translated, so they might not be ASCII.  So getopt has an
error message to an asprintf buffer, monkeys with internal flag bits
on stderr to disable cancellation, and then calls
__fxprintf(stderr, "%s", buffer).  There isn't even a helper function,
the code is duplicated every time.

This patch fixes __fxprintf to handle arbitrary multibyte format
strings, and adds a variant __fxprintf_nocancel that does the same
thing but also isn't a cancellation point.  (It still _works_ by
monkeying with internal flag bits on the FILE, but that's not really a
layering violation for code in stdio-common.)  All of the #ifdef _LIBC
blocks can then be reduced to their standalone versions with a little
help from some macros at the top of the file.

I also wrote a test case to verify that getopt really isn't a
cancellation point, and I'm glad I did, because it found two bugs, one
of which wasn't even to do with cancellation (see previous patch).

	* stdio-common/fxprintf.c (__fxprintf_nocancel): New function.
	(locked_vfxprintf): New helper function. Handle arbitrary
	multibyte strings, not just ASCII.
	* include/stdio.h: Declare __fxprintf_nocancel.

	* posix/getopt.c: When _LIBC is defined, define fprintf to
	__fxprintf_nocancel, flockfile to _IO_flockfile, and
	funlockfile to _IO_funlockfile.  When neither _LIBC nor
	_POSIX_THREAD_SAFE_FUNCTIONS is defined, define flockfile and
	funlockfile as no-ops.  (_getopt_internal_r): Remove all
	internal #ifdef _LIBC blocks; the standalone error-printing
	code can now be used for libc as well.  Add an
	flockfile/funlockfile pair around one case where the error
	message is printed in several chunks.  Don't use fputc.

	* posix/tst-getopt-cancel.c: New test.
	* posix/Makefile: Run it.

1 files changed, 53 insertions, 309 deletions

diff --git a/posix/getopt.c b/posix/getopt.c
index e616aa6..248fe5b 100644
--- a/posix/getopt.c
+++ b/posix/getopt.c
@@ -27,14 +27,30 @@
 
 #include <stdio.h>
 #include <stdlib.h>
-#include <unistd.h>
 #include <string.h>
+#include <unistd.h>
 
 #ifdef _LIBC
+/* When used as part of glibc, error printing must be done differently
+   for standards compliance.  getopt is not a cancellation point, so
+   it must not call functions that are, and it is specified by an
+   older standard than stdio locking, so it must not refer to
+   functions in the "user namespace" related to stdio locking.
+   Finally, it must use glibc's internal message translation so that
+   the messages are looked up in the proper text domain.  */
 # include <libintl.h>
+# define fprintf __fxprintf_nocancel
+# define flockfile(fp) _IO_flockfile (fp)
+# define funlockfile(fp) _IO_funlockfile (fp)
 #else
 # include "gettext.h"
 # define _(msgid) gettext (msgid)
+/* When used standalone, flockfile and funlockfile might not be
+   available.  */
+# ifndef _POSIX_THREAD_SAFE_FUNCTIONS
+#  define flockfile(fp) /* nop */
+#  define funlockfile(fp) /* nop */
+# endif
 #endif
 
 /* This implementation of 'getopt' has three modes for handling
@@ -98,7 +114,6 @@ int optopt = '?';
 /* Keep a global copy of all internal members of getopt_data.  */
 
 static struct _getopt_data getopt_data;
-
 
 /* Exchange two adjacent subsequences of ARGV.
    One subsequence is elements [first_nonopt,last_nonopt)
@@ -271,7 +286,7 @@ _getopt_internal_r (int argc, char *const *argv, const char *optstring,
   if (d->optind == 0 || !d->__initialized)
     {
       if (d->optind == 0)
-	d->optind = 1;	/* Don't scan ARGV[0], the program name.  */
+	d->optind = 1;  /* Don't scan ARGV[0], the program name.  */
       optstring = _getopt_initialize (argc, argv, optstring, d,
 				      posixly_correct);
       d->__initialized = 1;
@@ -441,42 +456,8 @@ _getopt_internal_r (int argc, char *const *argv, const char *optstring,
 	      first.next = ambig_list;
 	      ambig_list = &first;
 
-#if defined _LIBC
-	      char *buf = NULL;
-	      size_t buflen = 0;
-
-	      FILE *fp = __open_memstream (&buf, &buflen);
-	      if (fp != NULL)
-		{
-		  fprintf (fp,
-			   _("%s: option '%s' is ambiguous; possibilities:"),
-			   argv[0], argv[d->optind]);
-
-		  do
-		    {
-		      fprintf (fp, " '--%s'", ambig_list->p->name);
-		      ambig_list = ambig_list->next;
-		    }
-		  while (ambig_list != NULL);
-
-		  fputc_unlocked ('\n', fp);
-
-		  if (__glibc_likely (fclose (fp) != EOF))
-		    {
-		      _IO_flockfile (stderr);
-
-		      int old_flags2 = ((_IO_FILE *) stderr)->_flags2;
-		      ((_IO_FILE *) stderr)->_flags2 |= _IO_FLAGS2_NOTCANCEL;
+	      flockfile (stderr);
 
-		      __fxprintf (NULL, "%s", buf);
-
-		      ((_IO_FILE *) stderr)->_flags2 = old_flags2;
-		      _IO_funlockfile (stderr);
-
-		      free (buf);
-		    }
-		}
-#else
 	      fprintf (stderr,
 		       _("%s: option '%s' is ambiguous; possibilities:"),
 		       argv[0], argv[d->optind]);
@@ -487,8 +468,11 @@ _getopt_internal_r (int argc, char *const *argv, const char *optstring,
 		}
 	      while (ambig_list != NULL);
 
-	      fputc ('\n', stderr);
-#endif
+	      /* This must use 'fprintf' even though it's only printing a
+		 single character, so that it goes through __fxprintf_nocancel
+		 when compiled as part of glibc.  */
+	      fprintf (stderr, "\n");
+	      funlockfile (stderr);
 	    }
 	  d->__nextchar += strlen (d->__nextchar);
 	  d->optind++;
@@ -510,57 +494,17 @@ _getopt_internal_r (int argc, char *const *argv, const char *optstring,
 		{
 		  if (print_errors)
 		    {
-#if defined _LIBC
-		      char *buf;
-		      int n;
-#endif
-
 		      if (argv[d->optind - 1][1] == '-')
-			{
-			  /* --option */
-#if defined _LIBC
-			  n = __asprintf (&buf, _("\
-%s: option '--%s' doesn't allow an argument\n"),
-					  argv[0], pfound->name);
-#else
-			  fprintf (stderr, _("\
+			/* --option */
+			fprintf (stderr, _("\
 %s: option '--%s' doesn't allow an argument\n"),
-				   argv[0], pfound->name);
-#endif
-			}
+				 argv[0], pfound->name);
 		      else
-			{
-			  /* +option or -option */
-#if defined _LIBC
-			  n = __asprintf (&buf, _("\
-%s: option '%c%s' doesn't allow an argument\n"),
-					  argv[0], argv[d->optind - 1][0],
-					  pfound->name);
-#else
-			  fprintf (stderr, _("\
+			/* +option or -option */
+			fprintf (stderr, _("\
 %s: option '%c%s' doesn't allow an argument\n"),
-				   argv[0], argv[d->optind - 1][0],
-				   pfound->name);
-#endif
-			}
-
-#if defined _LIBC
-		      if (n >= 0)
-			{
-			  _IO_flockfile (stderr);
-
-			  int old_flags2 = ((_IO_FILE *) stderr)->_flags2;
-			  ((_IO_FILE *) stderr)->_flags2
-			    |= _IO_FLAGS2_NOTCANCEL;
-
-			  __fxprintf (NULL, "%s", buf);
-
-			  ((_IO_FILE *) stderr)->_flags2 = old_flags2;
-			  _IO_funlockfile (stderr);
-
-			  free (buf);
-			}
-#endif
+				 argv[0], argv[d->optind - 1][0],
+				 pfound->name);
 		    }
 
 		  d->__nextchar += strlen (d->__nextchar);
@@ -576,33 +520,10 @@ _getopt_internal_r (int argc, char *const *argv, const char *optstring,
 	      else
 		{
 		  if (print_errors)
-		    {
-#if defined _LIBC
-		      char *buf;
-
-		      if (__asprintf (&buf, _("\
-%s: option '--%s' requires an argument\n"),
-				      argv[0], pfound->name) >= 0)
-			{
-			  _IO_flockfile (stderr);
-
-			  int old_flags2 = ((_IO_FILE *) stderr)->_flags2;
-			  ((_IO_FILE *) stderr)->_flags2
-			    |= _IO_FLAGS2_NOTCANCEL;
-
-			  __fxprintf (NULL, "%s", buf);
-
-			  ((_IO_FILE *) stderr)->_flags2 = old_flags2;
-			  _IO_funlockfile (stderr);
+		    fprintf (stderr,
+			     _("%s: option '--%s' requires an argument\n"),
+			     argv[0], pfound->name);
 
-			  free (buf);
-			}
-#else
-		      fprintf (stderr,
-			       _("%s: option '--%s' requires an argument\n"),
-			       argv[0], pfound->name);
-#endif
-		    }
 		  d->__nextchar += strlen (d->__nextchar);
 		  d->optopt = pfound->val;
 		  return optstring[0] == ':' ? ':' : '?';
@@ -628,50 +549,14 @@ _getopt_internal_r (int argc, char *const *argv, const char *optstring,
 	{
 	  if (print_errors)
 	    {
-#if defined _LIBC
-	      char *buf;
-	      int n;
-#endif
-
 	      if (argv[d->optind][1] == '-')
-		{
-		  /* --option */
-#if defined _LIBC
-		  n = __asprintf (&buf, _("%s: unrecognized option '--%s'\n"),
-				  argv[0], d->__nextchar);
-#else
-		  fprintf (stderr, _("%s: unrecognized option '--%s'\n"),
-			   argv[0], d->__nextchar);
-#endif
-		}
+		/* --option */
+		fprintf (stderr, _("%s: unrecognized option '--%s'\n"),
+			 argv[0], d->__nextchar);
 	      else
-		{
-		  /* +option or -option */
-#if defined _LIBC
-		  n = __asprintf (&buf, _("%s: unrecognized option '%c%s'\n"),
-				  argv[0], argv[d->optind][0], d->__nextchar);
-#else
-		  fprintf (stderr, _("%s: unrecognized option '%c%s'\n"),
-			   argv[0], argv[d->optind][0], d->__nextchar);
-#endif
-		}
-
-#if defined _LIBC
-	      if (n >= 0)
-		{
-		  _IO_flockfile (stderr);
-
-		  int old_flags2 = ((_IO_FILE *) stderr)->_flags2;
-		  ((_IO_FILE *) stderr)->_flags2 |= _IO_FLAGS2_NOTCANCEL;
-
-		  __fxprintf (NULL, "%s", buf);
-
-		  ((_IO_FILE *) stderr)->_flags2 = old_flags2;
-		  _IO_funlockfile (stderr);
-
-		  free (buf);
-		}
-#endif
+		/* +option or -option */
+		fprintf (stderr, _("%s: unrecognized option '%c%s'\n"),
+			 argv[0], argv[d->optind][0], d->__nextchar);
 	    }
 	  d->__nextchar = (char *) "";
 	  d->optind++;
@@ -693,36 +578,7 @@ _getopt_internal_r (int argc, char *const *argv, const char *optstring,
     if (temp == NULL || c == ':' || c == ';')
       {
 	if (print_errors)
-	  {
-#if defined _LIBC
-	    char *buf;
-	    int n;
-#endif
-
-#if defined _LIBC
-	    n = __asprintf (&buf, _("%s: invalid option -- '%c'\n"),
-			    argv[0], c);
-#else
-	    fprintf (stderr, _("%s: invalid option -- '%c'\n"), argv[0], c);
-#endif
-
-#if defined _LIBC
-	    if (n >= 0)
-	      {
-		_IO_flockfile (stderr);
-
-		int old_flags2 = ((_IO_FILE *) stderr)->_flags2;
-		((_IO_FILE *) stderr)->_flags2 |= _IO_FLAGS2_NOTCANCEL;
-
-		__fxprintf (NULL, "%s", buf);
-
-		((_IO_FILE *) stderr)->_flags2 = old_flags2;
-		_IO_funlockfile (stderr);
-
-		free (buf);
-	      }
-#endif
-	  }
+	  fprintf (stderr, _("%s: invalid option -- '%c'\n"), argv[0], c);
 	d->optopt = c;
 	return '?';
       }
@@ -751,32 +607,10 @@ _getopt_internal_r (int argc, char *const *argv, const char *optstring,
 	else if (d->optind == argc)
 	  {
 	    if (print_errors)
-	      {
-#if defined _LIBC
-		char *buf;
-
-		if (__asprintf (&buf,
-				_("%s: option requires an argument -- '%c'\n"),
-				argv[0], c) >= 0)
-		  {
-		    _IO_flockfile (stderr);
-
-		    int old_flags2 = ((_IO_FILE *) stderr)->_flags2;
-		    ((_IO_FILE *) stderr)->_flags2 |= _IO_FLAGS2_NOTCANCEL;
-
-		    __fxprintf (NULL, "%s", buf);
-
-		    ((_IO_FILE *) stderr)->_flags2 = old_flags2;
-		    _IO_funlockfile (stderr);
+	      fprintf (stderr,
+		       _("%s: option requires an argument -- '%c'\n"),
+		       argv[0], c);
 
-		    free (buf);
-		  }
-#else
-		fprintf (stderr,
-			 _("%s: option requires an argument -- '%c'\n"),
-			 argv[0], c);
-#endif
-	      }
 	    d->optopt = c;
 	    if (optstring[0] == ':')
 	      c = ':';
@@ -825,30 +659,9 @@ _getopt_internal_r (int argc, char *const *argv, const char *optstring,
 	if (ambig && !exact)
 	  {
 	    if (print_errors)
-	      {
-#if defined _LIBC
-		char *buf;
-
-		if (__asprintf (&buf, _("%s: option '-W %s' is ambiguous\n"),
-				argv[0], d->optarg) >= 0)
-		  {
-		    _IO_flockfile (stderr);
-
-		    int old_flags2 = ((_IO_FILE *) stderr)->_flags2;
-		    ((_IO_FILE *) stderr)->_flags2 |= _IO_FLAGS2_NOTCANCEL;
+	      fprintf (stderr, _("%s: option '-W %s' is ambiguous\n"),
+		       argv[0], d->optarg);
 
-		    __fxprintf (NULL, "%s", buf);
-
-		    ((_IO_FILE *) stderr)->_flags2 = old_flags2;
-		    _IO_funlockfile (stderr);
-
-		    free (buf);
-		  }
-#else
-		fprintf (stderr, _("%s: option '-W %s' is ambiguous\n"),
-			 argv[0], d->optarg);
-#endif
-	      }
 	    d->__nextchar += strlen (d->__nextchar);
 	    return '?';
 	  }
@@ -864,33 +677,9 @@ _getopt_internal_r (int argc, char *const *argv, const char *optstring,
 		else
 		  {
 		    if (print_errors)
-		      {
-#if defined _LIBC
-			char *buf;
-
-			if (__asprintf (&buf, _("\
-%s: option '-W %s' doesn't allow an argument\n"),
-					argv[0], pfound->name) >= 0)
-			  {
-			    _IO_flockfile (stderr);
-
-			    int old_flags2 = ((_IO_FILE *) stderr)->_flags2;
-			    ((_IO_FILE *) stderr)->_flags2
-			      |= _IO_FLAGS2_NOTCANCEL;
-
-			    __fxprintf (NULL, "%s", buf);
-
-			    ((_IO_FILE *) stderr)->_flags2 = old_flags2;
-			    _IO_funlockfile (stderr);
-
-			    free (buf);
-			  }
-#else
-			fprintf (stderr, _("\
+		      fprintf (stderr, _("\
 %s: option '-W %s' doesn't allow an argument\n"),
-				 argv[0], pfound->name);
-#endif
-		      }
+			       argv[0], pfound->name);
 
 		    d->__nextchar += strlen (d->__nextchar);
 		    return '?';
@@ -903,33 +692,10 @@ _getopt_internal_r (int argc, char *const *argv, const char *optstring,
 		else
 		  {
 		    if (print_errors)
-		      {
-#if defined _LIBC
-			char *buf;
-
-			if (__asprintf (&buf, _("\
+		      fprintf (stderr, _("\
 %s: option '-W %s' requires an argument\n"),
-					argv[0], pfound->name) >= 0)
-			  {
-			    _IO_flockfile (stderr);
-
-			    int old_flags2 = ((_IO_FILE *) stderr)->_flags2;
-			    ((_IO_FILE *) stderr)->_flags2
-			      |= _IO_FLAGS2_NOTCANCEL;
-
-			    __fxprintf (NULL, "%s", buf);
-
-			    ((_IO_FILE *) stderr)->_flags2 = old_flags2;
-			    _IO_funlockfile (stderr);
+			       argv[0], pfound->name);
 
-			    free (buf);
-			  }
-#else
-			fprintf (stderr, _("\
-%s: option '-W %s' requires an argument\n"),
-				 argv[0], pfound->name);
-#endif
-		      }
 		    d->__nextchar += strlen (d->__nextchar);
 		    return optstring[0] == ':' ? ':' : '?';
 		  }
@@ -978,32 +744,10 @@ _getopt_internal_r (int argc, char *const *argv, const char *optstring,
 	    else if (d->optind == argc)
 	      {
 		if (print_errors)
-		  {
-#if defined _LIBC
-		    char *buf;
+		  fprintf (stderr,
+			   _("%s: option requires an argument -- '%c'\n"),
+			   argv[0], c);
 
-		    if (__asprintf (&buf, _("\
-%s: option requires an argument -- '%c'\n"),
-				    argv[0], c) >= 0)
-		      {
-			_IO_flockfile (stderr);
-
-			int old_flags2 = ((_IO_FILE *) stderr)->_flags2;
-			((_IO_FILE *) stderr)->_flags2 |= _IO_FLAGS2_NOTCANCEL;
-
-			__fxprintf (NULL, "%s", buf);
-
-			((_IO_FILE *) stderr)->_flags2 = old_flags2;
-			_IO_funlockfile (stderr);
-
-			free (buf);
-		      }
-#else
-		    fprintf (stderr,
-			     _("%s: option requires an argument -- '%c'\n"),
-			     argv[0], c);
-#endif
-		  }
 		d->optopt = c;
 		if (optstring[0] == ':')
 		  c = ':';