bug#8435: misuse of error ("...%d...", ...) on 64-bit hosts

[Paul Eggert]

On 04/08/2011 01:58 AM, Eli Zaretskii wrote:

> So I think it would be better to fix these problems as follows:
>   ...
>   . Fix doprnt to avoid overflow when EMACS_INT is a 64-bit type, if
>     it could overflow.  (I don't see such a danger, but maybe I
>     overlook something.)

That wouldn't work, not because doprnt overflows with EMACS_INT, but
because doprnt doesn't work with ordinary 'int': it treats all integer
arguments as if they were EMACS_INT, and this relies on unportable
va_arg behavior.

It's true that doprnt also has overflow problems on 64-bit hosts.
For example, it overruns a buffer when formatting a string whose
length doesn't fit in 'int'.  But that's a separate issue.

No doubt these problems could be worked around with sufficient
hacking, but why bother?  The main reason doprnt exists is that
vsnprintf didn't exist back when doprnt was written, so we had to
write it ourselves.  But now that we can rely on vsnprintf, let's use
it rather than continuing to maintain our reinvented buggy wheel.

> From now on, any code that needs to use %c for displaying a
> character codepoint will need to convert it manually before calling
> the message functions.

Yes, that's true.  It's not a problem now, since there's only one
occurrence of this situation in the Emacs source code, and it is easy
to treat it as a one-off.  If it turns into a problem, we can easily
address it, by replacing this sort of code:

    int codepoint = (whatever);
    error ("Invalid FINAL-CHAR %c, it should be `0'..`~'", codepoint);

with something like this:

    int codepoint = (whatever);
    error ("Invalid FINAL-CHAR %s, it should be `0'..`~'", cvt (codepoint));

where 'cvt' converts a codepoint to a string suitable for 'error'.

> I don't think we have any reason to support strings longer than
> INT_MAX in these functions.  They are used to display messages in the
> echo area/minibuffer, so they can hardly be close to INT_MAX anyway.
> We could simply document that and move on.  For bullet-proof code, we
> could even check the length and truncate the string before passing it
> to verror or its subroutines.

OK, thanks, then here's a further patch to do something along those
lines.  It reliably reports "memory full" when the resulting error
string is longer than INT_MAX; that's better than crashing, which is
what doprnt currently does.

* eval.c: Port to Windows vsnprintf (Bug#8435).
Include <limits.h>.
(SIZE_MAX): Define if the headers do not.
(verror): Do not give up if vsnprintf returns a negative count.
Instead, grow the buffer.  This ports to Windows vsnprintf, which
does not conform to C99.  Problem reported by Eli Zaretskii.
Also, simplify the allocation scheme, by avoiding the need for
calling realloc, and removing the ALLOCATED variable.
=== modified file 'src/eval.c'
--- src/eval.c  2011-04-07 05:19:50 +0000
+++ src/eval.c  2011-04-08 23:08:55 +0000
@@ -18,6 +18,7 @@


 #include <config.h>
+#include <limits.h>
 #include <setjmp.h>
 #include "lisp.h"
 #include "blockinput.h"
@@ -30,6 +31,10 @@
 #include "xterm.h"
 #endif

+#ifndef SIZE_MAX
+# define SIZE_MAX ((size_t) -1)
+#endif
+
 /* This definition is duplicated in alloc.c and keyboard.c.  */
 /* Putting it in lisp.h makes cc bomb out!  */

@@ -1978,36 +1983,37 @@
 {
   char buf[4000];
   size_t size = sizeof buf;
-  size_t size_max = (size_t) -1;
+  size_t size_max =
+    min (MOST_POSITIVE_FIXNUM, min (INT_MAX, SIZE_MAX - 1)) + 1;
   char *buffer = buf;
-  int allocated = 0;
   int used;
   Lisp_Object string;

   while (1)
     {
       used = vsnprintf (buffer, size, m, ap);
+
       if (used < 0)
-       used = 0;
-      if (used < size)
+       {
+         /* Non-C99 vsnprintf, such as w32, returns -1 when SIZE is too small.
+            Guess a larger USED to work around the incompatibility.  */
+         used = (size <= size_max / 2 ? 2 * size
+                 : size < size_max ? size_max - 1
+                 : size_max);
+       }
+      else if (used < size)
        break;
-      if (size <= size_max / 2)
-       size *= 2;
-      else if (size < size_max)
-       size = size_max;
-      else
+      if (size_max <= used)
        memory_full ();
-      if (allocated)
-       buffer = (char *) xrealloc (buffer, size);
-      else
-       {
-         buffer = (char *) xmalloc (size);
-         allocated = 1;
-       }
+      size = used + 1;
+
+      if (buffer != buf)
+       xfree (buffer);
+      buffer = (char *) xmalloc (size);
     }

   string = make_string (buffer, used);
-  if (allocated)
+  if (buffer != buf)
     xfree (buffer);

   xsignal1 (Qerror, string);