Re: [PATCH] inttostr.h: add compile-time buffer overrun checks

[Paul Eggert]

On 10/18/2010 11:41 PM, Jim Meyering wrote:

> I have to agree.  The whole point of inttostr functions is to
> provide a minimal and robust mechanism for converting integral
> values to strings.

I looked into this some more and came up with a different idea: add a
primitive umax2str that automatically allocates a big-enough buffer in
the caller's stack frame.  So, instead of this:

  if (repetition)
    {
      char buf[INT_BUFSIZE_BOUND (uintmax_t)];
      fprintf (stderr, _(" on repetition %s\n"), umaxtostr (repetition, buf));
    }

the caller can write this:

  if (repetition)
    fprintf (stderr, _(" on repetition %s\n"), umax2str (repetition));

Sometimes code still needs umaxtostr, so we'll continue to support that,
and do compile-time checking on the buffer if it's easy, but the idea
is to prefer umax2str when possible, since it makes code nicer to read
and easier to maintain.

Some downsides of umax2str, imax2str, etc. are:

 * They return a pointer to storage whose lifetime is that of the
   enclosing block.  It's possible that someone who doesn't understand
   how they work, will write code that ends up with dangling pointers.

 * The calling code (though not gnulib) must assume support for
   C99 compound literals.  Coreutils already assumes a C99 compiler,
   so I expect this is not a fatal objection for coreutils.

 * GCC generates slightly worse code for umax2str than for umaxtostr,
   since it unnecessarily stores zeros into the literals.  The number
   of extra instructions and the pressure on the cache is fairly
   small, but still, this is annoying.  Perhaps someday we can
   convince GCC to generate better code.

 * C99 compound literals are not that widely used in this way, and
   it's possible that we'll tickle compiler bugs.

Anyway, if you like this approach despite the downsides, here is a
proposed gnulib patch to add support for umax2str etc., while also
adding compile-time bounds checking for umaxtostr etc. when the
compile-time checking is easy.  This implementation does not support
VLA buffers but the need for that is practically zero.

Attached is a proposed coreutils patch to use this new feature.

>From 5d050036e995a9088d0b635bf6352d16424c38c0 Mon Sep 17 00:00:00 2001
From: Paul Eggert <address@hidden>
Date: Sun, 24 Oct 2010 18:20:58 -0700
Subject: [PATCH] inttostr: add int2str etc., and compile-time checks for 
inttostr etc.

This patch is by Jim Meyering <address@hidden>, Bruno Haible
<address@hidden>, and myself.

* lib/anytostr.c: Don't include config.h and inttostr.h; that is
now the includer's responsibility.
* lib/imaxtostr.c (imaxtostr): Undef after including those files.
* lib/inttostr.c (inttostr): Likewise.
* lib/offtostr.c (offtostr): Likewise.
* lib/uinttostr.c (uinttostr): Likewise.
* lib/umaxtostr.c (umaxtostr): Likewise.
* lib/inttostr.h: Include verify.h, as we need it (again).
(__gl_might_be_char_ptr, __gl_size_bound, __gl_inttostr): New macros.
(imaxtostr, inttostr, offtostr, uinttostr, umaxtostr): New macros,
that check the buffer size at compile time if possible.
(int2str_call, imax2str, int2str, off2str, uint2str, umax2str):
New macros, which assume C99, and which allocate a big-enough
buffer in the caller's frame.
* modules/inttostr (Depends-on): Add verify (again).
* modules/inttostr-tests (Files): Add tests/test-inttostr2.c.
* tests/test-inttostr2.c: New file.
---
 ChangeLog              |   25 +++++++++++++
 lib/anytostr.c         |    4 +--
 lib/imaxtostr.c        |    3 ++
 lib/inttostr.c         |    3 ++
 lib/inttostr.h         |   42 ++++++++++++++++++++++
 lib/offtostr.c         |    3 ++
 lib/uinttostr.c        |    3 ++
 lib/umaxtostr.c        |    3 ++
 modules/inttostr       |    1 +
 modules/inttostr-tests |    1 +
 tests/test-inttostr2.c |   89 ++++++++++++++++++++++++++++++++++++++++++++++++
 11 files changed, 174 insertions(+), 3 deletions(-)
 create mode 100644 tests/test-inttostr2.c

diff --git a/ChangeLog b/ChangeLog
index 95328fe..9b4b9cc 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,28 @@
+2010-10-24  Paul Eggert  <address@hidden>
+
+       inttostr: add int2str etc., and compile-time checks for inttostr etc.
+
+       This patch is by Jim Meyering <address@hidden>, Bruno Haible
+       <address@hidden>, and myself.
+
+       * lib/anytostr.c: Don't include config.h and inttostr.h; that is
+       now the includer's responsibility.
+       * lib/imaxtostr.c (imaxtostr): Undef after including those files.
+       * lib/inttostr.c (inttostr): Likewise.
+       * lib/offtostr.c (offtostr): Likewise.
+       * lib/uinttostr.c (uinttostr): Likewise.
+       * lib/umaxtostr.c (umaxtostr): Likewise.
+       * lib/inttostr.h: Include verify.h, as we need it (again).
+       (__gl_might_be_char_ptr, __gl_size_bound, __gl_inttostr): New macros.
+       (imaxtostr, inttostr, offtostr, uinttostr, umaxtostr): New macros,
+       that check the buffer size at compile time if possible.
+       (int2str_call, imax2str, int2str, off2str, uint2str, umax2str):
+       New macros, which assume C99, and which allocate a big-enough
+       buffer in the caller's frame.
+       * modules/inttostr (Depends-on): Add verify (again).
+       * modules/inttostr-tests (Files): Add tests/test-inttostr2.c.
+       * tests/test-inttostr2.c: New file.
+
 2010-10-23  Paul Eggert  <address@hidden>
 
        inttostr: simplify by removing unnecessary redundancy
diff --git a/lib/anytostr.c b/lib/anytostr.c
index eb71553..7681761 100644
--- a/lib/anytostr.c
+++ b/lib/anytostr.c
@@ -22,9 +22,7 @@
 # pragma GCC diagnostic ignored "-Wtype-limits"
 #endif
 
-#include <config.h>
-
-#include "inttostr.h"
+/* This file is meant to be included; see inttostr.c for an example use.  */
 
 /* Convert I to a printable string in BUF, which must be at least
    INT_BUFSIZE_BOUND (INTTYPE) bytes long.  Return the address of the
diff --git a/lib/imaxtostr.c b/lib/imaxtostr.c
index b91ac98..a944136 100644
--- a/lib/imaxtostr.c
+++ b/lib/imaxtostr.c
@@ -1,3 +1,6 @@
+#include <config.h>
+#include "inttostr.h"
+#undef imaxtostr
 #define anytostr imaxtostr
 #define inttype intmax_t
 #include "anytostr.c"
diff --git a/lib/inttostr.c b/lib/inttostr.c
index c96b5ca..0415ce6 100644
--- a/lib/inttostr.c
+++ b/lib/inttostr.c
@@ -1,3 +1,6 @@
+#include <config.h>
+#include "inttostr.h"
+#undef inttostr
 #define anytostr inttostr
 #define inttype int
 #include "anytostr.c"
diff --git a/lib/inttostr.h b/lib/inttostr.h
index 4f74968..eb6f542 100644
--- a/lib/inttostr.h
+++ b/lib/inttostr.h
@@ -21,6 +21,7 @@
 #include <sys/types.h>
 
 #include "intprops.h"
+#include "verify.h"
 
 #ifndef __GNUC_PREREQ
 # if defined __GNUC__ && defined __GNUC_MINOR__
@@ -44,3 +45,44 @@ char *inttostr (int, char *) 
__attribute_warn_unused_result__;
 char *offtostr (off_t, char *) __attribute_warn_unused_result__;
 char *uinttostr (unsigned int, char *) __attribute_warn_unused_result__;
 char *umaxtostr (uintmax_t, char *) __attribute_warn_unused_result__;
+
+/* Nonzero if P might be a char * pointer, zero if it is definitely not.  */
+#if __GNUC_PREREQ (3,1)
+# define __gl_might_be_char_ptr(p) \
+     __builtin_types_compatible_p (__typeof__ (p), char *)
+#else
+# define __gl_might_be_char_ptr(p) (sizeof (p) == sizeof (char *))
+#endif
+
+/* An upper bound on the size of the object that P points at.  Avoid
+   GCC's __builtin_object_size here, because GCC (as of version 4.5.1)
+   incorrectly rejects its use in an integral constant expression.  */
+#define __gl_size_bound(p)                                     \
+   (__gl_might_be_char_ptr (p) ? (size_t) -1 : sizeof (p))
+
+/* Convert a value of type T to a string, by invoking FUNC on the
+   value N, and using a char array S to hold the resulting string.
+   Diagnose obvious size problems at compile-time.  */
+#define __gl_inttostr(t, func, n, s)                                    \
+  ((void) verify_true (INT_BUFSIZE_BOUND (t) <= __gl_size_bound (s)),   \
+   (func) (n, s))
+
+/* Redefine imaxtostr etc. to check the size of the array, if possible.
+   S is not allowed to be a variable length array.  */
+#define imaxtostr(n, s) __gl_inttostr (intmax_t, imaxtostr, n, s)
+#define inttostr(n, s) __gl_inttostr (int, inttostr, n, s)
+#define offtostr(n, s) __gl_inttostr (off_t, offtostr, n, s)
+#define uinttostr(n, s) __gl_inttostr (unsigned int, uinttostr, n, s)
+#define umaxtostr(n, s) __gl_inttostr (uintmax_t, umaxtostr, n, s)
+
+/* Easier-to-use macros, which don't require a string buffer argument.
+   They can be used in code that assumes C99-style compound literals.
+   They return a pointer to storage with scope equal to that of the
+   enclosing block.  */
+#define int2str_call(t, func, n)                        \
+  func (n, ((char [INT_BUFSIZE_BOUND (t)]) {0,}))
+#define imax2str(n) int2str_call (intmax_t, imaxtostr, n)
+#define int2str(n) int2str_call (int, inttostr, n)
+#define off2str(n) int2str_call (off_t, offtostr, n)
+#define uint2str(n) int2str_call (unsigned int, uinttostr, n)
+#define umax2str(n) int2str_call (uintmax_t, umaxtostr, n)
diff --git a/lib/offtostr.c b/lib/offtostr.c
index 96082aa..c48fff1 100644
--- a/lib/offtostr.c
+++ b/lib/offtostr.c
@@ -1,3 +1,6 @@
+#include <config.h>
+#include "inttostr.h"
+#undef offtostr
 #define anytostr offtostr
 #define inttype off_t
 #include "anytostr.c"
diff --git a/lib/uinttostr.c b/lib/uinttostr.c
index 48fd98f..f915de9 100644
--- a/lib/uinttostr.c
+++ b/lib/uinttostr.c
@@ -1,3 +1,6 @@
+#include <config.h>
+#include "inttostr.h"
+#undef uinttostr
 #define anytostr uinttostr
 #define inttype unsigned int
 #include "anytostr.c"
diff --git a/lib/umaxtostr.c b/lib/umaxtostr.c
index f95bfc3..10043ae 100644
--- a/lib/umaxtostr.c
+++ b/lib/umaxtostr.c
@@ -1,3 +1,6 @@
+#include <config.h>
+#include "inttostr.h"
+#undef umaxtostr
 #define anytostr umaxtostr
 #define inttype uintmax_t
 #include "anytostr.c"
diff --git a/modules/inttostr b/modules/inttostr
index 6bbec28..495ef25 100644
--- a/modules/inttostr
+++ b/modules/inttostr
@@ -14,6 +14,7 @@ m4/inttostr.m4
 Depends-on:
 intprops
 stdint
+verify
 
 configure.ac:
 gl_INTTOSTR
diff --git a/modules/inttostr-tests b/modules/inttostr-tests
index 48dbe50..aa7698f 100644
--- a/modules/inttostr-tests
+++ b/modules/inttostr-tests
@@ -1,6 +1,7 @@
 Files:
 tests/macros.h
 tests/test-inttostr.c
+tests/test-inttostr2.c
 
 Depends-on:
 intprops
diff --git a/tests/test-inttostr2.c b/tests/test-inttostr2.c
new file mode 100644
index 0000000..f01d942
--- /dev/null
+++ b/tests/test-inttostr2.c
@@ -0,0 +1,89 @@
+/* Test Fortify support for inttostr functions.
+   Copyright (C) 2010 Free Software Foundation, Inc.
+
+   This program is free software: you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program 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 General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* Compile this with a GCC >= 4.3 on a glibc system, with options -O -S.  */
+
+/* Defining _FORTIFY_SOURCE, together with option -O, on a glibc system
+   leads to a definition of __USE_FORTIFY_LEVEL.  */
+#undef _FORTIFY_SOURCE
+#define _FORTIFY_SOURCE 1
+
+#include <config.h>
+
+#include "inttostr.h"
+
+#include <string.h>
+
+/* Buffer too small: warning at compile time.  */
+
+char *
+test_small_1 (int n)
+{
+  char buf[5];
+  return strdup (inttostr (n, buf)); /* expect warning here */
+}
+
+/* Buffer too small: would like warning at compile time, but cannot
+   easily do it due to GCC's mishandling of __builtin_object_size.  */
+
+char *
+test_small_2 (int n)
+{
+  char buf[5];
+  return strdup (inttostr (n, &buf[0])); /* would like warning here */
+}
+
+/* Buffer large enough: no warning, and call inttostr.  */
+
+char *
+test_large_1 (int n)
+{
+  char buf[12];
+  return strdup (inttostr (n, buf));
+}
+
+char *
+test_large_2 (int n)
+{
+  char buf[12];
+  return strdup (inttostr (n, &buf[0]));
+}
+
+/* Buffer is a variable length array; this is not supported.  */
+
+char *
+test_variable_length_array (int n)
+{
+  char buf[n < 0 ? 21 : 20];
+  return strdup (inttostr (n, buf)); /* expect warning here */
+}
+
+/* Buffer size unknown.
+   No warning. Call inttostr.  */
+
+char *
+test_variable_length_array_pointer (int n)
+{
+  char buf[n < 0 ? 21 : 20];
+  return strdup (inttostr (n, &buf[0]));
+}
+
+/*
+ * For Emacs M-x compile
+ * Local Variables:
+ * compile-command: "gcc -I. -I.. -I../gllib  -O -S test-inttostr2.c"
+ * End:
+ */
-- 
1.7.2

cu.patch
Description: Text Data