Subject: Re: bug#37585: Undefined behavior in nl, print_lineno

From: Paul Eggert <[email-masked]>
Date: Thu, 3 Oct 2019 12:43:54 -0700

On 10/2/19 7:50 AM, Roland Illig wrote:
> The current code says:
> 
>    next_line_no = line_no + page_incr;
>    if (next_line_no < line_no)
>      die (EXIT_FAILURE, 0, _("line number overflow"));
> 
> Since intmax_t is a regular integer type, overflow invokes undefined
> behavior and must therefore be checked using other means.

Thanks for the bug report. I looked for similar problems involving 
integer-overflow diagnostics in coreutils and installed the attached 
patches. The second patch should fix the bug you mentioned.
>From 1316620e81daf91317560226b2b63cbbf548c09d Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Thu, 3 Oct 2019 12:35:44 -0700
Subject: [PATCH 1/4] cp: simplify integer overflow checking

* src/copy.c (sparse_copy): Use INT_ADD_WRAPV instead
of doing overflow checking by hand.
---
 src/copy.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/copy.c b/src/copy.c
index 65cf65895..cd6104c7a 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -335,9 +335,7 @@ sparse_copy (int src_fd, int dest_fd, char *buf, size_t buf_size,
             }
           else  /* Coalesce writes/seeks.  */
             {
-              if (psize <= OFF_T_MAX - csize)
-                psize += csize;
-              else
+              if (INT_ADD_WRAPV (psize, csize, &psize))
                 {
                   error (0, 0, _("overflow reading %s"), quoteaf (src_name));
                   return false;
-- 
2.21.0

>From 89af2b307b455b53869bc9cf79af0272f7d8a1a2 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Thu, 3 Oct 2019 12:37:12 -0700
Subject: [PATCH 2/4] nl: fix integer-overflow bug
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Problem reported by Roland Illig (Bug#37585)
* src/nl.c (print_lineno): Don’t rely on undefined behavior when
checking for integer overflow.
---
 src/nl.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/src/nl.c b/src/nl.c
index 43092b4fe..d85408c8c 100644
--- a/src/nl.c
+++ b/src/nl.c
@@ -275,14 +275,10 @@ build_type_arg (char const **typep,
 static void
 print_lineno (void)
 {
-  intmax_t next_line_no;
-
   printf (lineno_format, lineno_width, line_no, separator_str);
 
-  next_line_no = line_no + page_incr;
-  if (next_line_no < line_no)
+  if (INT_ADD_WRAPV (line_no, page_incr, &line_no))
     die (EXIT_FAILURE, 0, _("line number overflow"));
-  line_no = next_line_no;
 }
 
 /* Switch to a header section. */
-- 
2.21.0

>From 72a348cc2d6160aa24bca93c23b1a17ffb5b1366 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Thu, 3 Oct 2019 12:38:15 -0700
Subject: [PATCH 3/4] numfmt: avoid unlikely integer overflow
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* src/numfmt.c (parse_format_string): Report overflow if
pad < -LONG_MAX, since that can’t be negated.
---
 src/numfmt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/numfmt.c b/src/numfmt.c
index 305a88603..c56641cfd 100644
--- a/src/numfmt.c
+++ b/src/numfmt.c
@@ -1081,7 +1081,7 @@ parse_format_string (char const *fmt)
 
   errno = 0;
   pad = strtol (fmt + i, &endptr, 10);
-  if (errno == ERANGE)
+  if (errno == ERANGE || pad < -LONG_MAX)
     die (EXIT_FAILURE, 0,
          _("invalid format %s (width overflow)"), quote (fmt));
 
-- 
2.21.0

>From d267ba04a6b4ad43e5a1311885f8ad9685502a5e Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Thu, 3 Oct 2019 12:41:22 -0700
Subject: [PATCH 4/4] truncate: avoid integer-overflow assumptions
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* src/truncate.c (do_ftruncate): Simplify overflow checking,
and don’t rely on theoretically-nonportable assumptions
like assuming that OFF_MAX < UINTMAX_MAX.
---
 src/truncate.c | 49 +++++++++++++++++++------------------------------
 1 file changed, 19 insertions(+), 30 deletions(-)

diff --git a/src/truncate.c b/src/truncate.c
index 4494ab51a..e7fb8543a 100644
--- a/src/truncate.c
+++ b/src/truncate.c
@@ -116,31 +116,29 @@ do_ftruncate (int fd, char const *fname, off_t ssize, off_t rsize,
     }
   if (block_mode)
     {
-      off_t const blksize = ST_BLKSIZE (sb);
-      if (ssize < OFF_T_MIN / blksize || ssize > OFF_T_MAX / blksize)
+      ptrdiff_t blksize = ST_BLKSIZE (sb);
+      intmax_t ssize0 = ssize;
+      if (INT_MULTIPLY_WRAPV (ssize, blksize, &ssize))
         {
           error (0, 0,
                  _("overflow in %" PRIdMAX
-                   " * %" PRIdMAX " byte blocks for file %s"),
-                 (intmax_t) ssize, (intmax_t) blksize,
-                 quoteaf (fname));
+                   " * %" PRIdPTR " byte blocks for file %s"),
+                 ssize0, blksize, quoteaf (fname));
           return false;
         }
-      ssize *= blksize;
     }
   if (rel_mode)
     {
-      uintmax_t fsize;
+      off_t fsize;
 
       if (0 <= rsize)
         fsize = rsize;
       else
         {
-          off_t file_size;
           if (usable_st_size (&sb))
             {
-              file_size = sb.st_size;
-              if (file_size < 0)
+              fsize = sb.st_size;
+              if (fsize < 0)
                 {
                   /* Sanity check.  Overflow is the only reason I can think
                      this would ever go negative. */
@@ -151,46 +149,37 @@ do_ftruncate (int fd, char const *fname, off_t ssize, off_t rsize,
             }
           else
             {
-              file_size = lseek (fd, 0, SEEK_END);
-              if (file_size < 0)
+              fsize = lseek (fd, 0, SEEK_END);
+              if (fsize < 0)
                 {
                   error (0, errno, _("cannot get the size of %s"),
                          quoteaf (fname));
                   return false;
                 }
             }
-          fsize = file_size;
         }
 
       if (rel_mode == rm_min)
-        nsize = MAX (fsize, (uintmax_t) ssize);
+        nsize = MAX (fsize, ssize);
       else if (rel_mode == rm_max)
-        nsize = MIN (fsize, (uintmax_t) ssize);
+        nsize = MIN (fsize, ssize);
       else if (rel_mode == rm_rdn)
         /* 0..ssize-1 -> 0 */
-        nsize = (fsize / ssize) * ssize;
-      else if (rel_mode == rm_rup)
-        /* 1..ssize -> ssize */
+        nsize = fsize - fsize % ssize;
+      else
         {
-          /* Here ssize>=1 && fsize>=0 */
-          uintmax_t const overflow = ((fsize + ssize - 1) / ssize) * ssize;
-          if (overflow > OFF_T_MAX)
+          if (rel_mode == rm_rup)
             {
-              error (0, 0, _("overflow rounding up size of file %s"),
-                     quoteaf (fname));
-              return false;
+              /* 1..ssize -> ssize */
+              off_t r = fsize % ssize;
+              ssize = r == 0 ? 0 : ssize - r;
             }
-          nsize = overflow;
-        }
-      else
-        {
-          if (ssize > OFF_T_MAX - (off_t)fsize)
+          if (INT_ADD_WRAPV (fsize, ssize, &nsize))
             {
               error (0, 0, _("overflow extending size of file %s"),
                      quoteaf (fname));
               return false;
             }
-          nsize = fsize + ssize;
         }
     }
   else
-- 
2.21.0