GNU bug report logs - #7347
[PATCH] stat: do not rely on undefined behavior in printf formats

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 7347 in the body.
You can then email your comments to 7347 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: bug-coreutils <at> gnu.org
Subject: [PATCH] stat: do not rely on undefined behavior in printf formats
Date: Sat, 06 Nov 2010 14:03:44 -0700

I have not pushed this, as I understand you're trying to put out a
release, but this fixes some portability bugs in 'stat', such that it
was relying on undefined behavior, which presumably could cause
'stat' to dump core, or worse, on non-GNU platforms.

The downside of this patch is that the "I" printf flag is now ignored.
However, support for "I" wasn't working anyway (because of time stamp
fractions), so this isn't much of a loss.  We can add proper "I"
support later, if there's demand for it.

From 844d06f2f74cc26e648c9951f9ab43ae11c1dfc9 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert <at> cs.ucla.edu>
Date: Sat, 6 Nov 2010 13:57:08 -0700
Subject: [PATCH] stat: do not rely on undefined behavior in printf formats

* src/stat.c (digits, printf_flags): New static vars.
(make_format): New function.
(out_string, out_int, out_uint, out_uint_o, out_uint_x):
(out_minus_zero): Use it to avoid undefined behavior when invoking
printf.
(print_it): Check for invalid conversion specifications such as
%..X and %1-X, which would otherwise rely on undefined behavior
when invoking printf.
* tests/misc/stat-nanoseconds: Check that the "I" printf flag
doesn't mess up in the C locale, as it formerly did on non-GNU
hosts.
---
 src/stat.c                  |   47 ++++++++++++++++++++++++++++++++++++------
 tests/misc/stat-nanoseconds |    1 +
 2 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/src/stat.c b/src/stat.c
index 99f115b..ae7ce02 100644
--- a/src/stat.c
+++ b/src/stat.c
@@ -150,6 +150,16 @@ statfs (char const *filename, struct fs_info *buf)
 #define hextobin(c) ((c) >= 'a' && (c) <= 'f' ? (c) - 'a' + 10 : \
                      (c) >= 'A' && (c) <= 'F' ? (c) - 'A' + 10 : (c) - '0')
 
+static char const digits[] = "0123456789";
+
+/* Flags that are portable for use in printf, for at least one
+   conversion specifier; make_format removes unportable flags as
+   needed for particular specifiers.  The glibc 2.2 extension "I" is
+   listed here; it is removed by make_format because it has undefined
+   behavior elsewhere and because it is incompatible with
+   out_epoch_sec.  */
+static char const printf_flags[] = "'-+ #0I";
+
 #define PROGRAM_NAME "stat"
 
 #define AUTHORS proper_name ("Michael Meskes")
@@ -467,40 +477,59 @@ human_time (struct timespec t)
   return str;
 }
 
+/* PFORMAT points to a '%' followed by a prefix of a format, all of
+   size PREFIX_LEN.  The flags allowed for this format are
+   ALLOWED_FLAGS; remove other printf flags from the prefix, then
+   append SUFFIX.  */
+static void
+make_format (char *pformat, size_t prefix_len, char const *allowed_flags,
+             char const *suffix)
+{
+  char *dst = pformat + 1;
+  char const *src;
+  char const *srclim = pformat + prefix_len;
+  for (src = dst; src < srclim && strchr (printf_flags, *src); src++)
+    if (strchr (allowed_flags, *src))
+      *dst++ = *src;
+  while (src < srclim)
+    *dst++ = *src++;
+  strcpy (dst, suffix);
+}
+
 static void
 out_string (char *pformat, size_t prefix_len, char const *arg)
 {
-  strcpy (pformat + prefix_len, "s");
+  make_format (pformat, prefix_len, "-", "s");
   printf (pformat, arg);
 }
 static int
 out_int (char *pformat, size_t prefix_len, intmax_t arg)
 {
-  strcpy (pformat + prefix_len, PRIdMAX);
+  make_format (pformat, prefix_len, "'-+ 0", PRIdMAX);
   return printf (pformat, arg);
 }
 static int
 out_uint (char *pformat, size_t prefix_len, uintmax_t arg)
 {
-  strcpy (pformat + prefix_len, PRIuMAX);
+  make_format (pformat, prefix_len, "'-0", PRIuMAX);
   return printf (pformat, arg);
 }
 static void
 out_uint_o (char *pformat, size_t prefix_len, uintmax_t arg)
 {
-  strcpy (pformat + prefix_len, PRIoMAX);
+  make_format (pformat, prefix_len, "-#0", PRIoMAX);
   printf (pformat, arg);
 }
 static void
 out_uint_x (char *pformat, size_t prefix_len, uintmax_t arg)
 {
-  strcpy (pformat + prefix_len, PRIxMAX);
+  make_format (pformat, prefix_len, "-#0", PRIxMAX);
   printf (pformat, arg);
 }
 static int
 out_minus_zero (char *pformat, size_t prefix_len)
 {
-  strcpy (pformat + prefix_len, ".0f");
+  make_format (pformat, prefix_len, "'-+ 0", ".0f");
   return printf (pformat, -0.25);
 }
 
@@ -1028,8 +1057,12 @@ print_it (char const *format, char const *filename,
         {
         case '%':
           {
-            size_t len = strspn (b + 1, "#-+.I 0123456789");
+            size_t len = strspn (b + 1, printf_flags);
             char const *fmt_char = b + len + 1;
+            fmt_char += strspn (fmt_char, digits);
+            if (*fmt_char == '.')
+              fmt_char += 1 + strspn (fmt_char + 1, digits);
+            len = fmt_char - (b + 1);
             unsigned int fmt_code = *fmt_char;
             memcpy (dest, b, len + 1);
 
diff --git a/tests/misc/stat-nanoseconds b/tests/misc/stat-nanoseconds
index 0f41eb0..cd21596 100755
--- a/tests/misc/stat-nanoseconds
+++ b/tests/misc/stat-nanoseconds
@@ -39,6 +39,7 @@ test "$(stat -c   %13.6X k)" =  ' 67413.023456'       || fail=1
 test "$(stat -c  %013.6X k)" =   067413.023456        || fail=1
 test "$(stat -c  %-13.6X k)" =   '67413.023456 '      || fail=1
 test "$(stat -c  %18.10X k)" = '  67413.0234567890'   || fail=1
+test "$(stat -c %I18.10X k)" = '  67413.0234567890'   || fail=1
 test "$(stat -c %018.10X k)" =  0067413.0234567890    || fail=1
 test "$(stat -c %-18.10X k)" =   '67413.0234567890  ' || fail=1
 
-- 
1.7.2

Message #8 received at 7347 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 7347 <at> debbugs.gnu.org
Subject: Re: bug#7347: [PATCH] stat: do not rely on undefined behavior in
	printf formats
Date: Sat, 06 Nov 2010 22:53:31 +0100

Paul Eggert wrote:
> I have not pushed this, as I understand you're trying to put out a
> release, but this fixes some portability bugs in 'stat', such that it
> was relying on undefined behavior, which presumably could cause
> 'stat' to dump core, or worse, on non-GNU platforms.
>
> The downside of this patch is that the "I" printf flag is now ignored.
> However, support for "I" wasn't working anyway (because of time stamp
> fractions), so this isn't much of a loss.  We can add proper "I"
> support later, if there's demand for it.
>
> Subject: [PATCH] stat: do not rely on undefined behavior in printf formats
>
> * src/stat.c (digits, printf_flags): New static vars.
> (make_format): New function.
> (out_string, out_int, out_uint, out_uint_o, out_uint_x):
> (out_minus_zero): Use it to avoid undefined behavior when invoking
> printf.
> (print_it): Check for invalid conversion specifications such as
> %..X and %1-X, which would otherwise rely on undefined behavior
> when invoking printf.
> * tests/misc/stat-nanoseconds: Check that the "I" printf flag
> doesn't mess up in the C locale, as it formerly did on non-GNU
> hosts.

Thanks for the patch!
I looked through it, applied it and tested it.
I see fixes (admittedly fringe, as you say, but still)
and don't see anything that might cause trouble, so go ahead and push it.
Nobody will miss the "I" flag.  I think I've never even seen it used.

Message #13 received at 7347-done <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Jim Meyering <jim <at> meyering.net>
Cc: 7347-done <at> debbugs.gnu.org
Subject: Re: bug#7347: [PATCH] stat: do not rely on undefined behavior in
	printf formats
Date: Sat, 06 Nov 2010 18:14:08 -0700

On 11/06/2010 02:53 PM, Jim Meyering wrote:
> I see fixes (admittedly fringe, as you say, but still)
> and don't see anything that might cause trouble, so go ahead and push it.

Thanks for checking it; I just pushed it.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.