Subject: Re: bug#73068: printf: please implement POSIX:2024 argument reordering

From: Pádraig Brady <[email-masked]>
Date: Fri, 13 Sep 2024 13:55:26 +0100

On 12/09/2024 20:33, Paul Eggert wrote:
> On 2024-09-12 12:03, Pádraig Brady wrote:
> 
> 
>> This is tricky enough, that we should be as restrictive as possible here,
>> so I may resort to strspn(f, "0123456789") to parse instead.
>> I'll think a bit about it.
> 
> The code's also assuming INT_MAX < INTMAX_MAX, which POSIX doesn't
> require. You could put in a static_assert to that effect, I suppose, to
> document the assumption.

Indeed. We would have incorrectly taken the INTMAX_MAX arg in the
(albeit unlikely) case where INT_MAX >= INTMAX_MAX,
and the provided number overflowed INTMAX_MAX.
To be explicit, strtol() doesn't return 0 in that case,
so we need to check overflow (like Colin suggested).

> More important, though, if you're not in the C locale all bets are off
> as far as what strtoimax will also parse.
> 
> When I ran into this problem with GNU tar, I ended by giving up on
> strtoimax and did my own little integer parser. It does exactly what I
> want and I don't have to fire up the strtoimax complexity+locale engine.

Right, it's best to preparse for the above reason,
and to avoid any confusion re leading spaces etc. like I previously mentioned.

The attached adjustment does the preparse with strspn(),
and only does the strtoimax() for appropriate strings.

cheers,
Pádraig

diff --git a/src/printf.c b/src/printf.c
index 37844f14a..442935705 100644
--- a/src/printf.c
+++ b/src/printf.c
@@ -454,14 +454,23 @@ print_formatted (char const *format, int argc, char **argv)
 
 #define GET_CURR_ARG(POS)				\
 do {							\
-  char *arge;						\
-  intmax_t arg = POS==3 ? 0 : strtoimax (f, &arge, 10);	\
-  if (0 < arg && arg <= INT_MAX && *arge == '$')	\
+  intmax_t arg = 0;					\
+  size_t argl;						\
+  /* Check with strspn() first to avoid spaces etc.	\
+     This also avoids any locale ambiguities.  */	\
+  if (POS != 3 && (argl = strspn (f, "0123456789"))	\
+      && f[argl] == '$')				\
+    {							\
+      errno = 0;					\
+      arg = strtoimax (f, NULL, 10);			\
+      if (errno)					\
+        arg = 0;					\
+    }							\
+  if (1 <= arg && arg <= INT_MAX)			\
     /* Process indexed %i$ format.  */			\
-    /* Note '$' comes before any flags.  */		\
     {							\
       SET_CURR_ARG (arg - 1);				\
-      f = arge + 1;					\
+      f += argl + 1;					\
       if (POS == 0)					\
         direc_arg = arg - 1;				\
     }							\
diff --git a/tests/printf/printf-indexed.sh b/tests/printf/printf-indexed.sh
index d0f799ab1..5d63d68f8 100755
--- a/tests/printf/printf-indexed.sh
+++ b/tests/printf/printf-indexed.sh
@@ -75,19 +75,28 @@ printf_check '   1' '%2$*1$d\n' 4 1
 # Indexed arg, and sequential width
 printf_check '   1' '%2$*d\n' 4 1
 
-# Flags come after $ (0 is not a flag here):
+# Flags come after $ (0 is not a flag here but allowed):
 printf_check '   1' '%01$4d\n' 1
 # Flags come after $ (0 is a flag here):
 printf_check '0001' '%1$0*2$d\n' 1 4
 # Flags come after $ (-2 not taken as a valid index here):
 printf_check_err 'printf: %-2$: invalid conversion specification' \
                  '%-2$s %1$s\n' A B
+# Flags come after $ (' ' is not allowed as part of number here)
+printf_check_err 'printf: % 2$: invalid conversion specification' \
+                 '% 2$s %1$s\n' A B
 
 # Ensure only base 10 numbers are accepted
 printf_check_err "printf: 'A': expected a numeric value" \
                  '%0x2$s %2$s\n' A B
+# Ensure empty numbers are rejected
+printf_check_err 'printf: %$: invalid conversion specification' \
+                 '%$d\n' 1
 # Verify int limits (avoiding comparisons with argc etc.)
 printf_check_err "printf: %${INT_OFLOW}\$: invalid conversion specification" \
                  "%${INT_OFLOW}\$d\n" 1
+# Verify intmax limits, ensuring overflow detected
+printf_check_err "printf: %${INTMAX_OFLOW}\$: invalid conversion specification" \
+                 "%${INTMAX_OFLOW}\$d\n" 1
 
 Exit $fail