Re: vasnprintf: avoid using %n in the general case

[Bruno Haible]

Hi Jeremie,

> The attached patch changes vasnprintf.c to avoid using %n in the general
> case, ie when the return value of snprintf is usable.  This should help
> if more systems decide to make tighten %n usage.  There are plans for
> that in OpenBSD land.

Thanks for the suggestion. Yes, this is a good idea. (I didn't know about
the OpenBSD plans.)

> The existing comments in vasnprintf.c mention systems where
> gl_SNPRINTF_RETVAL_C99 and gl_SNPRINTF_TRUNCATION_C99 pass.  This patch
> only considers the usability of the return value of snprintf, as lack
> of truncation seems to be a different problem (apparently handled later
> in the code).

I prefer to limit the shortcut to those platforms where both
gl_SNPRINTF_RETVAL_C99 and gl_SNPRINTF_TRUNCATION_C99 pass. These are
the conditions I found when considering this code in detail a couple of
years ago. Maybe you are right that only one of the conditions is needed;
but I don't want to live on the risky edge here.


2020-10-04  Bruno Haible  <bruno@clisp.org>

        vasnprintf: Don't use %n on modern, ISO C 99 compliant platforms.
        Suggested by Jeremie Courreges-Anglas <jca@wxcvbn.org> in
        <https://lists.gnu.org/archive/html/bug-gnulib/2020-10/msg00010.html>.
        * m4/vasnprintf.m4 (gl_PREREQ_VASNPRINTF): Define
        HAVE_SNPRINTF_TRUNCATION_C99.
        * lib/vasnprintf.c (VASNPRINTF): Don't use %n if
        HAVE_SNPRINTF_RETVAL_C99 && HAVE_SNPRINTF_TRUNCATION_C99.

diff --git a/lib/vasnprintf.c b/lib/vasnprintf.c
index 7f75139..b232d14 100644
--- a/lib/vasnprintf.c
+++ b/lib/vasnprintf.c
@@ -5117,39 +5117,32 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
 #endif
                   *fbp = dp->conversion;
 #if USE_SNPRINTF
-# if ! (((__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 3))        \
-         && !defined __UCLIBC__)                                            \
-        || (defined __APPLE__ && defined __MACH__)                          \
-        || defined __ANDROID__                                              \
-        || (defined _WIN32 && ! defined __CYGWIN__))
-                fbp[1] = '%';
-                fbp[2] = 'n';
-                fbp[3] = '\0';
-# else
-                /* On glibc2 systems from glibc >= 2.3 - probably also older
-                   ones - we know that snprintf's return value conforms to
-                   ISO C 99: the tests gl_SNPRINTF_RETVAL_C99 and
-                   gl_SNPRINTF_TRUNCATION_C99 pass.
-                   Therefore we can avoid using %n in this situation.
-                   On glibc2 systems from 2004-10-18 or newer, the use of %n
-                   in format strings in writable memory may crash the program
-                   (if compiled with _FORTIFY_SOURCE=2), so we should avoid it
-                   in this situation.  */
-                /* On Mac OS X 10.3 or newer, we know that snprintf's return
-                   value conforms to ISO C 99: the tests gl_SNPRINTF_RETVAL_C99
-                   and gl_SNPRINTF_TRUNCATION_C99 pass.
-                   Therefore we can avoid using %n in this situation.
-                   On Mac OS X 10.13 or newer, the use of %n in format strings
-                   in writable memory by default crashes the program, so we
-                   should avoid it in this situation.  */
-                /* On Android, we know that snprintf's return value conforms to
-                   ISO C 99: the tests gl_SNPRINTF_RETVAL_C99 and
-                   gl_SNPRINTF_TRUNCATION_C99 pass.
-                   Therefore we can avoid using %n in this situation.
-                   Starting on 2018-03-07, the use of %n in format strings
-                   produces a fatal error (see
-                   
<https://android.googlesource.com/platform/bionic/+/41398d03b7e8e0dfb951660ae713e682e9fc0336>),
-                   so we should avoid it.  */
+# if ((HAVE_SNPRINTF_RETVAL_C99 && HAVE_SNPRINTF_TRUNCATION_C99)            \
+      || ((__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 3))       \
+          && !defined __UCLIBC__)                                           \
+      || (defined __APPLE__ && defined __MACH__)                            \
+      || defined __ANDROID__                                                \
+      || (defined _WIN32 && ! defined __CYGWIN__))
+                /* On systems where we know that snprintf's return value
+                   conforms to ISO C 99 (HAVE_SNPRINTF_RETVAL_C99) and that
+                   snprintf always produces NUL-terminated strings
+                   (HAVE_SNPRINTF_TRUNCATION_C99), it is possible to avoid
+                   using %n.  And it is desirable to do so, because more and
+                   more platforms no longer support %n, for "security reasons".
+                   In particular, the following platforms:
+                     - On glibc2 systems from 2004-10-18 or newer, the use of
+                       %n in format strings in writable memory may crash the
+                       program (if compiled with _FORTIFY_SOURCE=2).
+                     - On Mac OS X 10.13 or newer, the use of %n in format
+                       strings in writable memory by default crashes the
+                       program.
+                     - On Android, starting on 2018-03-07, the use of %n in
+                       format strings produces a fatal error (see
+                       
<https://android.googlesource.com/platform/bionic/+/41398d03b7e8e0dfb951660ae713e682e9fc0336>).
+                   On these platforms, HAVE_SNPRINTF_RETVAL_C99 and
+                   HAVE_SNPRINTF_TRUNCATION_C99 are 1. We have listed them
+                   explicitly in the condition above, in case of cross-
+                   compilation (just to be sure).  */
                 /* On native Windows systems (such as mingw), we can avoid 
using
                    %n because:
                      - Although the gl_SNPRINTF_TRUNCATION_C99 test fails,
@@ -5166,6 +5159,10 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
                      
<https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/set-printf-count-output>
                    So we should avoid %n in this situation.  */
                 fbp[1] = '\0';
+# else           /* AIX <= 5.1, HP-UX, IRIX, OSF/1, Solaris <= 9, BeOS */
+                fbp[1] = '%';
+                fbp[2] = 'n';
+                fbp[3] = '\0';
 # endif
 #else
                 fbp[1] = '\0';
diff --git a/m4/vasnprintf.m4 b/m4/vasnprintf.m4
index 4567061..43f070e 100644
--- a/m4/vasnprintf.m4
+++ b/m4/vasnprintf.m4
@@ -1,4 +1,4 @@
-# vasnprintf.m4 serial 37
+# vasnprintf.m4 serial 38
 dnl Copyright (C) 2002-2004, 2006-2020 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -74,6 +74,16 @@ AC_DEFUN_ONCE([gl_PREREQ_VASNPRINTF],
          if the buffer had been large enough.])
       ;;
   esac
+  dnl Additionally, the use of %n can be eliminated by assuming that snprintf
+  dnl always produces NUL-terminated strings (no truncation).
+  AC_REQUIRE([gl_SNPRINTF_TRUNCATION_C99])
+  case "$gl_cv_func_snprintf_truncation_c99" in
+    *yes)
+      AC_DEFINE([HAVE_SNPRINTF_TRUNCATION_C99], [1],
+        [Define if the string produced by the snprintf function is always NUL
+         terminated.])
+      ;;
+  esac
 ])
 
 # Extra prerequisites of lib/vasnprintf.c for supporting 'long double'