Re: [Bug-gnulib] linebreak.c proposed patches for size-calculation overflows

[Paul Eggert]

Bruno Haible <address@hidden> writes:

> I imagine macros that let me write:
> 
>              bool overflow = false;
>              size_t size = xsum3 (xtimes (n, sizeof (size_t), &overflow),
>                                   m,
>                                   m,
>                                   &overflow);
>              if (!overflow)
>                {
>                  char *memory = malloc (size);
> 
> What do you think?

I'd rather reserve (size_t) -1 to represent overflow, and limit myself
to binary operators, e.g.:

             char *memory = malloc (xsum (xtimes (n, sizeof (size_t)),
                                          xsum (m, m)));

but even this is relatively hard to read compared to the original, and
it assumes that malloc (SIZE_MAX) always fails, which is probably a
portable assumption but is a bit worrisome nonetheless.

> Other proposals how this code could be written in a
> maintainable way?

Currently linebreak.c doesn't include xalloc.h, but if you don't mind
depending on xalloc.h perhaps there's a macro or two that we could
add.  (For example, we could arrange for xmalloc (SIZE_MAX) to fail
reliably, if we want to use the convention that SIZE_MAX means
overflow.)

Here's another possibility, which doesn't rely on xalloc.h.  This
patch adds only 2 lines of code to linebreak.c, totalling 4 new
comparisons followed by conditional branches.  The other solutions
that I can think of offhand are all less efficient than this, which is
perhaps not that big a deal here but will be an issue elsewhere.

2003-10-31  Paul Eggert  <address@hidden>

        * linebreak.c (iconv_string_length): Return (size_t)(-1) when
        there is an size-calculation overflow.
        (mbs_possible_linebreaks, mbs_width_linebreaks):
        Do not allow overflow in size calculations.

--- linebreak.c.~1.4.~  Wed Jul 30 23:27:05 2003
+++ linebreak.c Fri Oct 31 15:15:44 2003
@@ -1379,9 +1379,10 @@ iconv_string_length (iconv_t cd, const c
       char *outptr = tmpbuf;
       size_t outsize = TMPBUFSIZE;
       size_t res = iconv (cd, (ICONV_CONST char **) &inptr, &insize, &outptr, 
&outsize);
-      if (res == (size_t)(-1) && errno != E2BIG)
+      size_t tmpcount = outptr - tmpbuf;
+      count += tmpcount;
+      if ((res == (size_t)(-1) && errno != E2BIG) || count < tmpcount)
         return (size_t)(-1);
-      count += outptr - tmpbuf;
     }
   /* Avoid glibc-2.1 bug and Solaris 7 through 9 bug.  */
 #if defined _LIBICONV_VERSION \
@@ -1390,9 +1391,10 @@ iconv_string_length (iconv_t cd, const c
     char *outptr = tmpbuf;
     size_t outsize = TMPBUFSIZE;
     size_t res = iconv (cd, NULL, NULL, &outptr, &outsize);
-    if (res == (size_t)(-1))
+    size_t tmpcount = outptr - tmpbuf;
+    count += tmpcount;
+    if (res == (size_t)(-1) || count < tmpcount)
       return (size_t)(-1);
-    count += outptr - tmpbuf;
   }
   /* Return to the initial state.  */
   iconv (cd, NULL, NULL, NULL, NULL);
@@ -1510,8 +1512,8 @@ mbs_possible_linebreaks (const char *s, 
         to_utf8 = (iconv_t)(-1);
       else
 # endif
-      to_utf8 = iconv_open (UTF8_NAME, encoding);
-      if (to_utf8 != (iconv_t)(-1))
+      if (n <= (size_t)(-1) / (sizeof (size_t) + 2 * MB_LEN_MAX)
+          && (to_utf8 = iconv_open (UTF8_NAME, encoding)) != (iconv_t)(-1))
         {
           /* Determine the length of the resulting UTF-8 string.  */
           size_t m = iconv_string_length (to_utf8, s, n);
@@ -1603,8 +1605,8 @@ mbs_width_linebreaks (const char *s, siz
         to_utf8 = (iconv_t)(-1);
       else
 # endif
-      to_utf8 = iconv_open (UTF8_NAME, encoding);
-      if (to_utf8 != (iconv_t)(-1))
+      if (n <= (size_t)(-1) / (sizeof (size_t) + 3 * MB_LEN_MAX)
+          && (to_utf8 = iconv_open (UTF8_NAME, encoding)) != (iconv_t)(-1))
         {
           /* Determine the length of the resulting UTF-8 string.  */
           size_t m = iconv_string_length (to_utf8, s, n);