[PATCH] Fix a printf hook problem

[Jakub Jelinek <jakub at redhat dot com>]

Hi!

Yesterday and today I've worked on printf hooks support for libquadmath
(i.e. support for printing __float128 IEEE754 quad values on
x86_64/i386/ia64).  I'd like to use Q modifier for it, is anyone aware for
other uses of such modifier that would be incompatible with it?

I've used
http://www.eglibc.org/cgi-bin/viewcvs.cgi/*checkout*/libdfp/trunk/printf_dfp.c?content-type=text%2Fplain&rev=12594
as the source for this, but found a few issues in glibc and libdfp
printf_dfp.c.  Current libquadmath patch is at http://gcc.gnu.org/PR47642

1) The comment in printf-parsemb.c suggests that arginfo function should
   be able to return -1 to signal that it wants standard processing,
   even the code looks like it was meant to handle that, but
   as spec->ndata_args is unsigned (size_t), it is never < 0 and
   *printf crashes badly when the arginfo routine returns -1.
   Fixed by the first hunk in the patch below.
   There is a workaround for this in the libraries, essentially instead
   of returning -1 it can duplicate the stuff from __parse_one_specmb
   instead, nevertheless it would be nice to see this fixed in glibc.

2) When testing fa_IR.UTF-8 %Ie support, I've noticed that often there
   are garbage characters following the decimal point.  This is because
   decimal buffer (and thousands too) is not actually zero terminated,
   so if stack doesn't happen to be zero, we get garbage.  Fixed by the
   remaining two hunks.

3) %Ie doesn't work in wide orientation at all.  As COMPILE_WPRINTF
   is not used by printf_fp.c, but instead decision whether it
   is wide or not is based on wide variable, _i18n_number_rewrite
   will always just rewrite the narrow string, but with info->wide
   we actually print the wide string instead.  Not fixed here.

4) I was surprised to see there is no way to unregister the hooks
   (well, at least modifiers and types - specifiers can be
   unregistered currently by calling register_printf_specifier
   again with NULL, NULL and also that the implementation
   allows only a single converter/arginfo pair to be registered
   for each specifier letter.  This makes it e.g. impossible
   to handle both _Decimal* and __float128 printing at the same time.
   I guess glibc could iterate through all the registered
   arginfo functions as long as they return -1, and only the one
   that will say to handle it would then be used together with the
   corresponding converted function.  Then unregistering wouldn't
   work by calling it again with NULL, NULL though and thus
   some unregister_* APIs would be needed.

And the libdfp bugs:

1) Related to 1) above, libdfp arginfo function currently returns
   0 if it is not D/DD/H modifier, which means that whenever you
   use e/E/f/F/g/G/a/A specifier, if no modifier or some other modifier
   is used it assumes no argument needs to be consumed (similarly to %m),
   i.e. when you register libdfp suddenly "%e %Lg %f" will not work
   anymore.

2) __printf_dfp doesn't return -2 if modifier is not D/DD/H, which
   means that say %e is printed as if it was _Decimal32 number
   (and because of 1) above random garbage is printed as such number).

3) #ifndef OPTION_EGLIBC_LOCALE_CODE
   char decimald;
   #endif
   const char *decimal;
   ...
   #else
   /* Hard-code values from 'C' locale.  */
   decimald = ".";
   decimal = &decimald;
   decimalwc.wc = L'.';
   #endif
   can't possible work right, assigning "." to a char?  Surely
   that was supposed to be decimal = "."; and kill decimald.

4) libdfp doesn't have a way to unregister, which means if
   you say dlopen libdfp, call the register function and
   then dlclose it, next printf will likely segfault.

5) Maybe the TR requires it, but I find it surprising that
   a/A prints a decimal number instead of hexadecimal on
   for _Decimal*.

Anyway, here is a patch to fix glibc 1) and 2) above.

2011-02-11  Jakub Jelinek  <jakub@redhat.com>

	* stdio-common/printf-parsemb.c (__parse_one_specmb): Handle
	arginfo fn returning -1.

	* stdio-common/_i18n_number.h (_i18n_number_rewrite): Ensure decimal
	and thousands string is zero terminated.

--- libc/stdio-common/printf-parsemb.c.jj	2011-01-06 11:48:40.000000000 +0100
+++ libc/stdio-common/printf-parsemb.c	2011-02-11 16:47:33.034254929 +0100
@@ -295,9 +295,9 @@ __parse_one_specmb (const UCHAR_T *forma
       /* We don't try to get the types for all arguments if the format
 	 uses more than one.  The normal case is covered though.  If
 	 the call returns -1 we continue with the normal specifiers.  */
-      || (spec->ndata_args = (*__printf_arginfo_table[spec->info.spec])
-	  (&spec->info, 1, &spec->data_arg_type,
-	   &spec->size)) < 0)
+      || (int) (spec->ndata_args = (*__printf_arginfo_table[spec->info.spec])
+				   (&spec->info, 1, &spec->data_arg_type,
+				    &spec->size)) < 0)
     {
       /* Find the data argument types of a built-in spec.  */
       spec->ndata_args = 1;
--- libc/stdio-common/_i18n_number.h.jj	2011-01-06 11:48:40.087542069 +0100
+++ libc/stdio-common/_i18n_number.h	2011-02-12 13:59:50.436254163 +0100
@@ -30,8 +30,8 @@ _i18n_number_rewrite (CHAR_T *w, CHAR_T 
 # define decimal NULL
 # define thousands NULL
 #else
-  char decimal[MB_LEN_MAX];
-  char thousands[MB_LEN_MAX];
+  char decimal[MB_LEN_MAX + 1];
+  char thousands[MB_LEN_MAX + 1];
 #endif
 
   /* "to_outpunct" is a map from ASCII decimal point and thousands-sep
@@ -47,13 +47,19 @@ _i18n_number_rewrite (CHAR_T *w, CHAR_T 
       mbstate_t state;
       memset (&state, '\0', sizeof (state));
 
-      if (__wcrtomb (decimal, wdecimal, &state) == (size_t) -1)
+      size_t n = __wcrtomb (decimal, wdecimal, &state);
+      if (n == (size_t) -1)
 	memcpy (decimal, ".", 2);
+      else
+	decimal[n] = '\0';
 
       memset (&state, '\0', sizeof (state));
 
-      if (__wcrtomb (thousands, wthousands, &state) == (size_t) -1)
+      n = __wcrtomb (thousands, wthousands, &state);
+      if (n == (size_t) -1)
 	memcpy (thousands, ",", 2);
+      else
+	thousands[n] = '\0';
     }
 #endif
 


	Jakub