bugGNU roff - Bugs: bug #64301, [troff] susceptible to integer...

 
 

bug #64301: [troff] susceptible to integer overflow

Submitter:  G. Branden Robinson <gbranden>
Submitted:  Tue 13 Jun 2023 09:52:33 PM UTC
   
 
Category:  Core Severity:  4 - Important
Item Group:  Crash/Unresponsive Status:  Fixed
Privacy:  Public Assigned to:  gbranden
Open/Closed:  Closed Planned Release:  1.24.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 26 Jul 2024 04:30:08 PM UTC, comment #9: 

comment #8:

> Comment #6 appears to have been truncated.


A definite hazard with Savannah, I've found, but not the case here.

It was a scribble meant to illustrate the use cases of `pl` and `ll` requests with gigantic argument values.

 G. Branden Robinson <gbranden>
Group administrator
Fri 26 Jul 2024 02:58:40 PM UTC, comment #8: 

Comment #6 appears to have been truncated.

 Dave <barx>
Group Member
Thu 25 Jul 2024 09:16:16 PM UTC, comment #7: 


commit a3e0a4fe337a1482410a307180825e55c0fc0869
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Thu Jul 25 15:10:39 2024 -0500

    src/roff/troff/number.cpp: Fix Savannah #64301.

    * src/roff/troff/number.cpp (vunits::vunits, hunits::hunits): Migrate to
      C23 checked arithmetic macros.  Throw error if addition overflows.

    Fixes <https://savannah.gnu.org/bugs/?64301>.  (For real this time,
    fingers crossed.)

commit a7687009739feb5592f33de6e6a3573bae7fc349
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Thu Jul 25 08:22:33 2024 -0500

    [troff]: Add more tests of arithmetic.

    * src/roff/groff/tests/arithmetic-works.sh: Add more tests.

    Test fails at this commit.


 G. Branden Robinson <gbranden>
Group administrator
Sat 20 Jul 2024 07:24:22 PM UTC, comment #6: 

I missed a couple of spots.


diff --git a/src/roff/groff/tests/arithmetic-works.sh b/src/roff/groff/tests/arithmetic-works.sh
index 3f0b91701..bb4c175b4 100755
--- a/src/roff/groff/tests/arithmetic-works.sh
+++ b/src/roff/groff/tests/arithmetic-works.sh
@@ -43,6 +43,10 @@ input='.
 @&.v=@n(.v
 .ll -2
 @&.l=@n(.l
+.ll 2147483647u
+.ll
+.pl 2147483647u
+.pl
 .'

 # Expected:


 G. Branden Robinson <gbranden>
Group administrator
Fri 19 Jul 2024 07:15:31 PM UTC, comment #5: 


commit 5f608c758ffc1558fd4d07f387a3559bd6b95af5
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Fri Jul 19 13:53:40 2024 -0500

    Add test of *roff arithmetic.

    * src/roff/groff/tests/arithmetic-works.sh: Do it.
    * src/roff/groff/groff.am (groff_TESTS): Run test.

commit 7570fbf57040df6320709263803ba98fe8a41719
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Thu Jul 18 22:03:19 2024 -0500

    [troff]: Fix Savannah #64301 (15/15).

    * src/roff/troff/number.cpp (is_valid_term): When negating a term,
      remove manual detection of negation of `INT_MIN` (guaranteed to
      overflow in a two's complement representation), relying on `ckd_mul()`
      to indicate it.

    Fixes <https://savannah.gnu.org/bugs/?64301>.  You have to compile GNU
    troff with the (GCC) compiler option `-ftrapv` for arithmetic to trap
    and cause a core dump.

commit 7a7c693db3974c22bde0492f0a333a1a051b4b5f
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Thu Jul 18 21:54:54 2024 -0500

    [troff]: Fix Savannah #64301 (14/15).

    * src/roff/troff/number.cpp (is_valid_term): When applying the *roff `|`
      boundary-relative motion operator, remove manual detection of
      overflow, relying on `ckd_sub()` to indicate it, and store result to
      temporary variable in case it overflows and must be discarded.
      Otherwise, copy temporary to the storage for the term being
      interpreted.

commit b4f9d7010d10703df99770db0e4d91fa57422d38
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Jul 15 18:02:24 2024 -0500

    [troff]: Fix Savannah #64301 (13/15).

    * src/roff/troff/number.cpp (is_valid_term): When parsing integer
      literals, remove manual detection of overflow, relying on `ckd_mul()`
      and `ckd_add()` to indicate it.  When an overflowing literal is
      encountered, skip excess digits.  This is a behavior change, albeit an
      esoteric one; evaluation of a numeric term no longer aborts in this
      scenario.

commit f3a331e907f7824a7edff025bed21fb2110a2860
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Jul 15 12:10:47 2024 -0500

    [troff]: Fix Savannah #64301 (12/15).

    * src/roff/troff/number.cpp (get_hunits): Use `ckd_add()` (with
      temporary variable, annotating why) instead of primitive operation,
      and throw error diagnostic if arithmetic wraps.

commit 277152bb27731d6672b28a9c51f28ff2cda931b0
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Jul 15 10:46:28 2024 -0500

    [troff]: Fix Savannah #64301 (11/15).

    * src/roff/troff/hvunits.h (hunits operator +): Use `ckd_add()` instead
      of primitive operation, and throw error diagnostic if arithmetic
      wraps.

commit baadf5f03bfacec08e34ef2e0263f2de8bd1f2ef
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Jul 15 10:41:34 2024 -0500

    [troff]: Fix Savannah #64301 (10/15).

    * src/roff/troff/hvunits.h (hunits operator -): Use `ckd_sub()` instead
      of primitive operation, and throw error diagnostic if arithmetic
      wraps.

commit ba57af884546e75fa28c6769abfd4d2fd286f27b
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Jul 15 10:07:51 2024 -0500

    [troff]: Fix Savannah #64301 (9/15).

    * src/roff/troff/hvunits.h (hunits operator *): Use `ckd_mul()` instead
      of primitive operation, and throw error diagnostic if arithmetic
      wraps.

commit 870f3a6e69b51eec85bb8383002b6134fec2dd83
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Jul 15 10:21:16 2024 -0500

    [troff]: Fix Savannah #64301 (8/15).

    * src/roff/troff/hvunits.h (hunits::to_units): Use `ckd_mul()` instead
      of primitive operation, and throw error diagnostic if arithmetic
      wraps.

commit 40d51279673ac09f05285cf6cd8818a833eca117
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Jul 15 09:59:55 2024 -0500

    [troff]: Fix Savannah #64301 (7/15).

    * src/roff/troff/number.cpp (get_vunits): Use `ckd_add()` (with
      temporary variable, annotating why) instead of primitive operation,
      and throw error diagnostic if arithmetic wraps.

commit cd6f015058e8bb41cbc06141acd364d6dfc5ecc7
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Jul 15 09:54:05 2024 -0500

    [troff]: Fix Savannah #64301 (6/15).

    * src/roff/troff/hvunits.h (vunits operator *): Use `ckd_mul()` instead
      of primitive operation, and throw error diagnostic if arithmetic
      wraps.

commit 5565799763b22ff9b9f9f27bbb0ac3f9bd76b895
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Jul 15 09:51:19 2024 -0500

    [troff]: Fix Savannah #64301 (5/15).

    * src/roff/troff/hvunits.h (vunits operator +): Use `ckd_add()` instead
      of primitive operation, and throw error diagnostic if arithmetic
      wraps.

commit 4d90e3b1097c98c39b3b778384d255af69014709
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Jul 15 09:49:57 2024 -0500

    [troff]: Fix Savannah #64301 (4/15).

    * src/roff/troff/hvunits.h (vunits::to_units): Use `ckd_mul()` instead
      of primitive operation, and throw error diagnostic if arithmetic
      wraps.

commit 64f649db296577d3c20657c1d7f863aa2b63ce60
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Jul 15 10:28:38 2024 -0500

    [troff]: Fix Savannah #64301 (3/15).

    * src/roff/troff/hvunits.h: Include `config.h` and `stdckdint.h`
      headers.

      (vunits operator -): Use `ckd_sub()` instead of primitive operation,
      and throw error diagnostic if arithmetic wraps.

commit e0f26fa73f6e7628f84e4ad5abe4fd643ad836af
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Jul 15 09:45:24 2024 -0500

    [troff]: Fix Savannah #64301 (2/15).

    * src/roff/troff/number.cpp: Include `config.h` and `stdckdint.h`
      headers.

      (get_number, get_integer, is_valid_expression): Use `ckd_add()`,
      `ckd_sub()`, and `ckd_mul()` instead of primitive operations, and
      throw error diagnostic if arithmetic wraps.

      (is_valid_expression): Remove manual detection of overflow.

commit b12e14107a71e244c614328e82f24dddc194bd58
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Jul 15 09:44:55 2024 -0500

    [troff]: Fix Savannah #64301 (1/15).

    [troff]: Prevent traps on overflowing integer arithmetic.

    * bootstrap.conf: Add "stdckdint" to `gnulib_modules`.

    Also fix whitespace nit and add editor aid comments.


 G. Branden Robinson <gbranden>
Group administrator
Mon 15 Jul 2024 05:35:59 PM UTC, comment #4: 

Patch #7 was hosed.  Fixed that, and patch #12 (new) keeps all tests passing.

Is the finish line in sight?


commit 22b79dc48ab3bcae3e37719ed39d50c3f7363db1
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Jul 15 09:59:55 2024 -0500

    XXX stdckdint number.cpp get_vunits (7/x)

diff --git a/src/roff/troff/number.cpp b/src/roff/troff/number.cpp
index 66bb62cd2..cdecd1246 100644
--- a/src/roff/troff/number.cpp
+++ b/src/roff/troff/number.cpp
@@ -118,6 +118,9 @@ static incr_number_result get_incr_number(units *res, unsigned char);
 bool get_vunits(vunits *res, unsigned char si, vunits prev_value)
 {
   units v;
+  // Use a primitive temporary because having the ckd macros store to
+  // &(res->n) requires `friend` access and produces wrong results.
+  int i;
   switch (get_incr_number(&v, si)) {
   case INVALID:
     return false;
@@ -125,10 +128,14 @@ bool get_vunits(vunits *res, unsigned char si, vunits prev_value)
     *res = v;
     break;
   case INCREMENT:
-    *res = prev_value + v;
+    if (ckd_add(&i, prev_value.to_units(), v))
+      warning(WARN_RANGE, "integer addition saturated");
+    *res = i;
     break;
   case DECREMENT:
-    *res = prev_value - v;
+    if (ckd_sub(&i, prev_value.to_units(), v))
+      warning(WARN_RANGE, "integer subtraction saturated");
+    *res = i;
     break;
   default:
     assert(0 == "unhandled case returned by get_incr_number()");

commit 31b0fe34f1a93b87cbd529c1d4e766d31fe954b1 (HEAD -> master)
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Jul 15 12:10:47 2024 -0500

    XXX stdckdint number.cpp get_hunits (12/x)

diff --git a/src/roff/troff/number.cpp b/src/roff/troff/number.cpp
index cdecd1246..6a3fca45f 100644
--- a/src/roff/troff/number.cpp
+++ b/src/roff/troff/number.cpp
@@ -146,6 +146,9 @@ bool get_vunits(vunits *res, unsigned char si, vunits prev_value)
 bool get_hunits(hunits *res, unsigned char si, hunits prev_value)
 {
   units h;
+  // Use a primitive temporary because having the ckd macros store to
+  // &(res->n) requires `friend` access and produces wrong results.
+  int i;
   switch (get_incr_number(&h, si)) {
   case INVALID:
     return false;
@@ -153,10 +156,14 @@ bool get_hunits(hunits *res, unsigned char si, hunits prev_value)
     *res = h;
     break;
   case INCREMENT:
-    *res = prev_value + h;
+    if (ckd_add(&i, prev_value.to_units(), h))
+      warning(WARN_RANGE, "integer addition saturated");
+    *res = i;
     break;
   case DECREMENT:
-    *res = prev_value - h;
+    if (ckd_sub(&i, prev_value.to_units(), h))
+      warning(WARN_RANGE, "integer subtraction saturated");
+    *res = i;
     break;
   default:
     assert(0 == "unhandled case returned by get_incr_number()");


 G. Branden Robinson <gbranden>
Group administrator
Mon 15 Jul 2024 04:23:58 PM UTC, comment #3: 

Checkpoint.


commit 21e441cc615f294dec9e3f296b7e6a8f8f5bc812
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Jul 15 09:44:55 2024 -0500

    XXX stdckdint bootstrap.conf (1/x)

diff --git a/bootstrap.conf b/bootstrap.conf
index 20bee83f1..0e34b056c 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -22,7 +22,7 @@ m4_base=gnulib_m4
 # gnulib C source files
 source_base=lib

-# additional standard files, particularly added by
+# additional standard files, particularly added by
 # automake --add-missing
 build_aux=build-aux

@@ -45,6 +45,7 @@ gnulib_modules="
     vsnprintf
     stat
     stdbool-c99
+    stdckdint
     stdint
     sys_wait
 "
@@ -107,3 +108,12 @@ bootstrap_post_import_hook ()
   # Automake requires that ChangeLog exist.
   touch ChangeLog || return 1
 }
+
+##### Editor settings
+# Local Variables:
+# coding: latin-1
+# fill-column: 72
+# mode: text
+# version-control: never
+# End:
+# vim: set autoindent shiftwidth=2 textwidth=72:

commit 5ec5ecb56ca468ce9d1fd62ad00e376e3f2db5b7
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Jul 15 09:45:24 2024 -0500

    XXX stdckdint number.cpp get_number, is_valid_expression (2/x)

diff --git a/src/roff/troff/number.cpp b/src/roff/troff/number.cpp
index 1c83c5316..66bb62cd2 100644
--- a/src/roff/troff/number.cpp
+++ b/src/roff/troff/number.cpp
@@ -16,6 +16,11 @@ 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/>. */

+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <stdckdint.h>

 #include "troff.h"
 #include "hvunits.h"
@@ -162,10 +167,12 @@ bool get_number(units *res, unsigned char si, units prev_value)
     *res = u;
     break;
   case INCREMENT:
-    *res = prev_value + u;
+    if (ckd_add(res, prev_value, u))
+      warning(WARN_RANGE, "integer addition saturated");
     break;
   case DECREMENT:
-    *res = prev_value - u;
+    if (ckd_sub(res, prev_value, u))
+      warning(WARN_RANGE, "integer subtraction saturated");
     break;
   default:
     assert(0 == "unhandled case returned by get_incr_number()");
@@ -183,10 +190,12 @@ bool get_integer(int *res, int prev_value)
     *res = i;
     break;
   case INCREMENT:
-    *res = prev_value + int(i);
+    if (ckd_add(res, prev_value, i))
+      warning(WARN_RANGE, "integer addition saturated");
     break;
   case DECREMENT:
-    *res = prev_value - int(i);
+    if (ckd_sub(res, prev_value, i))
+      warning(WARN_RANGE, "integer subtraction saturated");
     break;
   default:
     assert(0 == "unhandled case returned by get_incr_number()");
@@ -296,7 +305,6 @@ static bool is_valid_expression(units *u, int scaling_unit,
     if (!is_valid_term(&u2, scaling_unit, is_parenthesized,
                       is_mandatory))
       return false;
-    bool had_overflow = false;
     switch (op) {
     case '<':
       *u = *u < u2;
@@ -328,57 +336,22 @@ static bool is_valid_expression(units *u, int scaling_unit,
       *u = *u > 0 || u2 > 0;
       break;
     case '+':
-      if (u2 < 0) {
-       if (*u < INT_MIN - u2)
-         had_overflow = true;
-      }
-      else if (u2 > 0) {
-       if (*u > INT_MAX - u2)
-         had_overflow = true;
-      }
-      if (had_overflow) {
+      if (ckd_add(u, *u, u2)) {
        error("addition overflow");
        return false;
       }
-      *u += u2;
       break;
     case '-':
-      if (u2 < 0) {
-       if (*u > INT_MAX + u2)
-         had_overflow = true;
-      }
-      else if (u2 > 0) {
-       if (*u < INT_MIN + u2)
-         had_overflow = true;
-      }
-      if (had_overflow) {
+      if (ckd_sub(u, *u, u2)) {
        error("subtraction overflow");
        return false;
       }
-      *u -= u2;
       break;
     case '*':
-      if (u2 < 0) {
-       if (*u > 0) {
-         if ((unsigned)*u > -(unsigned)INT_MIN / -(unsigned)u2)
-           had_overflow = true;
-       }
-       else if (-(unsigned)*u > INT_MAX / -(unsigned)u2)
-         had_overflow = true;
-      }
-      else if (u2 > 0) {
-       if (*u > 0) {
-         if (*u > INT_MAX / u2)
-           had_overflow = true;
-       }
-       else if (-(unsigned)*u > -(unsigned)INT_MIN / u2)
-         had_overflow = true;
-      }
-      if (had_overflow) {
+      if (ckd_mul(u, *u, u2)) {
        error("multiplication overflow");
        return false;
       }
-      *u *= u2;
       break;
     case '/':
       if (u2 == 0) {

commit 416e7c891028ecdf5ccbe9e1cff64eae43013717
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Jul 15 10:28:38 2024 -0500

    XXX stdckdint hvunits.h vunits operator - (3/x)

diff --git a/src/roff/troff/hvunits.h b/src/roff/troff/hvunits.h
index c685788df..7e20a0c86 100644
--- a/src/roff/troff/hvunits.h
+++ b/src/roff/troff/hvunits.h
@@ -16,6 +16,12 @@ 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/>. */

+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <stdckdint.h>
+
 class vunits {
   int n;
 public:
@@ -105,14 +111,17 @@ inline vunits operator -(const vunits & x, const vunits & y)
 {
   vunits r;
   r = x;
-  r.n -= y.n;
+  if (ckd_sub(&r.n, r.n, y.n))
+    warning(WARN_RANGE, "integer subtraction saturated");
   return r;
 }

 inline vunits operator -(const vunits & x)
 {
   vunits r;
-  r.n = -x.n;
+  // Why?  Consider -(INT_MIN) in two's complement.
+  if (ckd_mul(&r.n, x.n, -1))
+    warning(WARN_RANGE, "integer multiplication saturated");
   return r;
 }


commit f917b38491d5efce19e7449bf6a8410ffc513f53
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Jul 15 09:49:57 2024 -0500

    XXX stdckdint hvunits.h vunits::to_units (4/x)

diff --git a/src/roff/troff/hvunits.h b/src/roff/troff/hvunits.h
index 7e20a0c86..1e3f860e2 100644
--- a/src/roff/troff/hvunits.h
+++ b/src/roff/troff/hvunits.h
@@ -91,7 +91,10 @@ inline vunits:: vunits() : n(0)

 inline units vunits::to_units()
 {
-  return n * vresolution;
+  units r;
+  if (ckd_mul(&r, n, vresolution))
+    warning(WARN_RANGE, "integer multiplication saturated");
+  return r;
 }

 inline bool vunits::is_zero()

commit cb45ee9be57fba38b391fd93245146bee15c5b2f
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Jul 15 09:51:19 2024 -0500

    XXX stdckdint hvunits.h vunits operator + (5/x)

diff --git a/src/roff/troff/hvunits.h b/src/roff/troff/hvunits.h
index 1e3f860e2..7a03eedce 100644
--- a/src/roff/troff/hvunits.h
+++ b/src/roff/troff/hvunits.h
@@ -106,7 +106,8 @@ inline vunits operator +(const vunits & x, const vunits & y)
 {
   vunits r;
   r = x;
-  r.n += y.n;
+  if (ckd_add(&r.n, r.n, y.n))
+    warning(WARN_RANGE, "integer addition saturated");
   return r;
 }


commit 1834d2a1dd5340e5233ac1d1f7b5133a79003523
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Jul 15 09:54:05 2024 -0500

    XXX stdckdint hvunits.h vunits operator * (6/x)

diff --git a/src/roff/troff/hvunits.h b/src/roff/troff/hvunits.h
index 7a03eedce..f6a216e62 100644
--- a/src/roff/troff/hvunits.h
+++ b/src/roff/troff/hvunits.h
@@ -146,7 +146,8 @@ inline vunits operator *(const vunits & x, int n)
 {
   vunits r;
   r = x;
-  r.n *= n;
+  if (ckd_mul(&r.n, x.n, n))
+    warning(WARN_RANGE, "integer multiplication saturated");
   return r;
 }

@@ -154,7 +155,8 @@ inline vunits operator *(int n, const vunits & x)
 {
   vunits r;
   r = x;
-  r.n *= n;
+  if (ckd_mul(&r.n, x.n, n))
+    warning(WARN_RANGE, "integer multiplication saturated");
   return r;
 }


commit 9c11619aea027ec87c46f66311354568bad35b66
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Jul 15 09:59:55 2024 -0500

    XXX stdckdint hvunits.h, number.cpp get_vunits (7/x)

diff --git a/src/roff/troff/hvunits.h b/src/roff/troff/hvunits.h
index f6a216e62..9d32e300c 100644
--- a/src/roff/troff/hvunits.h
+++ b/src/roff/troff/hvunits.h
@@ -47,6 +47,7 @@ public:
   friend inline bool operator >=(const vunits&, const vunits&);
   friend inline bool operator ==(const vunits&, const vunits&);
   friend inline bool operator !=(const vunits&, const vunits&);
+  friend bool get_vunits(vunits *, unsigned char, vunits);
 };

 extern const vunits V0;
diff --git a/src/roff/troff/number.cpp b/src/roff/troff/number.cpp
index 66bb62cd2..e43340144 100644
--- a/src/roff/troff/number.cpp
+++ b/src/roff/troff/number.cpp
@@ -125,10 +125,12 @@ bool get_vunits(vunits *res, unsigned char si, vunits prev_value)
     *res = v;
     break;
   case INCREMENT:
-    *res = prev_value + v;
+    if (ckd_add(&(res->n), prev_value.n, v))
+      warning(WARN_RANGE, "integer addition saturated");
     break;
   case DECREMENT:
-    *res = prev_value - v;
+    if (ckd_sub(&(res->n), prev_value.n, v))
+      warning(WARN_RANGE, "integer subtraction saturated");
     break;
   default:
     assert(0 == "unhandled case returned by get_incr_number()");

commit 1a195e791f7131bfb7a8574c40f70e83a59b68c8
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Jul 15 10:21:16 2024 -0500

    XXX stdckdint hvunits.h hunits::to_units (8/x)

diff --git a/src/roff/troff/hvunits.h b/src/roff/troff/hvunits.h
index 9d32e300c..da036012c 100644
--- a/src/roff/troff/hvunits.h
+++ b/src/roff/troff/hvunits.h
@@ -209,7 +209,10 @@ inline hunits:: hunits() : n(0)

 inline units hunits::to_units()
 {
-  return n * hresolution;
+  units r;
+  if (ckd_mul(&r, n, hresolution))
+    warning(WARN_RANGE, "integer multiplication saturated");
+  return r;
 }

 inline bool hunits::is_zero()

commit 31dcea2c15c355a47672d345fe25cf2fe81c2981
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Jul 15 10:07:51 2024 -0500

    XXX stdckdint hvunits.h hunits operator * (9/x)

diff --git a/src/roff/troff/hvunits.h b/src/roff/troff/hvunits.h
index da036012c..ff554a913 100644
--- a/src/roff/troff/hvunits.h
+++ b/src/roff/troff/hvunits.h
@@ -261,7 +261,8 @@ inline hunits operator *(const hunits & x, int n)
 {
   hunits r;
   r = x;
-  r.n *= n;
+  if (ckd_mul(&r.n, x.n, n))
+    warning(WARN_RANGE, "integer multiplication saturated");
   return r;
 }

@@ -269,7 +270,8 @@ inline hunits operator *(int n, const hunits & x)
 {
   hunits r;
   r = x;
-  r.n *= n;
+  if (ckd_mul(&r.n, x.n, n))
+    warning(WARN_RANGE, "integer multiplication saturated");
   return r;
 }


commit c27407c9ba78173fa18454004b66075b54cdaf07
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Jul 15 10:41:34 2024 -0500

    XXX stdckdint hvunits.h hunits operator - (10/x)

diff --git a/src/roff/troff/hvunits.h b/src/roff/troff/hvunits.h
index ff554a913..dce32201f 100644
--- a/src/roff/troff/hvunits.h
+++ b/src/roff/troff/hvunits.h
@@ -232,7 +232,8 @@ inline hunits operator -(const hunits & x, const hunits & y)
 {
   hunits r;
   r = x;
-  r.n -= y.n;
+  if (ckd_sub(&r.n, r.n, y.n))
+    warning(WARN_RANGE, "integer subtraction saturated");
   return r;
 }

@@ -240,7 +241,9 @@ inline hunits operator -(const hunits & x)
 {
   hunits r;
   r = x;
-  r.n = -x.n;
+  // Why?  Consider -(INT_MIN) in two's complement.
+  if (ckd_mul(&r.n, x.n, -1))
+    warning(WARN_RANGE, "integer multiplication saturated");
   return r;
 }


commit 6071c70662b4948982a493fe6c14b6fb7e73674c
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Jul 15 10:46:28 2024 -0500

    XXX stdckdint hvunits.h vunits operator + (11/x)

diff --git a/src/roff/troff/hvunits.h b/src/roff/troff/hvunits.h
index dce32201f..95e644c2a 100644
--- a/src/roff/troff/hvunits.h
+++ b/src/roff/troff/hvunits.h
@@ -224,7 +224,8 @@ inline hunits operator +(const hunits & x, const hunits & y)
 {
   hunits r;
   r = x;
-  r.n += y.n;
+  if (ckd_add(&r.n, r.n, y.n))
+    warning(WARN_RANGE, "integer addition saturated");
   return r;
 }


 G. Branden Robinson <gbranden>
Group administrator
Sun 14 Jul 2024 09:44:22 PM UTC, comment #2: 

At long last I have this underway.

Here's what I have so far.


 4976d2ccb34e9bf6e93a7e91aad63cf599d3ce67
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Sun Jul 14 10:33:26 2024 -0500

    XXX stdckdint

diff --git a/bootstrap.conf b/bootstrap.conf
index 20bee83f1..0e34b056c 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -22,7 +22,7 @@ m4_base=gnulib_m4
 # gnulib C source files
 source_base=lib

-# additional standard files, particularly added by
+# additional standard files, particularly added by
 # automake --add-missing
 build_aux=build-aux

@@ -45,6 +45,7 @@ gnulib_modules="
     vsnprintf
     stat
     stdbool-c99
+    stdckdint
     stdint
     sys_wait
 "
@@ -107,3 +108,12 @@ bootstrap_post_import_hook ()
   # Automake requires that ChangeLog exist.
   touch ChangeLog || return 1
 }
+
+##### Editor settings
+# Local Variables:
+# coding: latin-1
+# fill-column: 72
+# mode: text
+# version-control: never
+# End:
+# vim: set autoindent shiftwidth=2 textwidth=72:
diff --git a/src/roff/troff/hvunits.h b/src/roff/troff/hvunits.h
index 56e892553..2f78afc4b 100644
--- a/src/roff/troff/hvunits.h
+++ b/src/roff/troff/hvunits.h
@@ -41,6 +41,7 @@ public:
   friend inline int operator >=(const vunits&, const vunits&);
   friend inline int operator ==(const vunits&, const vunits&);
   friend inline int operator !=(const vunits&, const vunits&);
+  friend bool get_vunits(vunits *, unsigned char, vunits);
 };

 extern const vunits V0;
diff --git a/src/roff/troff/number.cpp b/src/roff/troff/number.cpp
index 1c83c5316..e43340144 100644
--- a/src/roff/troff/number.cpp
+++ b/src/roff/troff/number.cpp
@@ -16,6 +16,11 @@ 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/>. */

+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <stdckdint.h>

 #include "troff.h"
 #include "hvunits.h"
@@ -120,10 +125,12 @@ bool get_vunits(vunits *res, unsigned char si, vunits prev_value)
     *res = v;
     break;
   case INCREMENT:
-    *res = prev_value + v;
+    if (ckd_add(&(res->n), prev_value.n, v))
+      warning(WARN_RANGE, "integer addition saturated");
     break;
   case DECREMENT:
-    *res = prev_value - v;
+    if (ckd_sub(&(res->n), prev_value.n, v))
+      warning(WARN_RANGE, "integer subtraction saturated");
     break;
   default:
     assert(0 == "unhandled case returned by get_incr_number()");
@@ -162,10 +169,12 @@ bool get_number(units *res, unsigned char si, units prev_value)
     *res = u;
     break;
   case INCREMENT:
-    *res = prev_value + u;
+    if (ckd_add(res, prev_value, u))
+      warning(WARN_RANGE, "integer addition saturated");
     break;
   case DECREMENT:
-    *res = prev_value - u;
+    if (ckd_sub(res, prev_value, u))
+      warning(WARN_RANGE, "integer subtraction saturated");
     break;
   default:
     assert(0 == "unhandled case returned by get_incr_number()");
@@ -183,10 +192,12 @@ bool get_integer(int *res, int prev_value)
     *res = i;
     break;
   case INCREMENT:
-    *res = prev_value + int(i);
+    if (ckd_add(res, prev_value, i))
+      warning(WARN_RANGE, "integer addition saturated");
     break;
   case DECREMENT:
-    *res = prev_value - int(i);
+    if (ckd_sub(res, prev_value, i))
+      warning(WARN_RANGE, "integer subtraction saturated");
     break;
   default:
     assert(0 == "unhandled case returned by get_incr_number()");
@@ -296,7 +307,6 @@ static bool is_valid_expression(units *u, int scaling_unit,
     if (!is_valid_term(&u2, scaling_unit, is_parenthesized,
                       is_mandatory))
       return false;
-    bool had_overflow = false;
     switch (op) {
     case '<':
       *u = *u < u2;
@@ -328,57 +338,22 @@ static bool is_valid_expression(units *u, int scaling_unit,
       *u = *u > 0 || u2 > 0;
       break;
     case '+':
-      if (u2 < 0) {
-       if (*u < INT_MIN - u2)
-         had_overflow = true;
-      }
-      else if (u2 > 0) {
-       if (*u > INT_MAX - u2)
-         had_overflow = true;
-      }
-      if (had_overflow) {
+      if (ckd_add(u, *u, u2)) {
        error("addition overflow");
        return false;
       }
-      *u += u2;
       break;
     case '-':
-      if (u2 < 0) {
-       if (*u > INT_MAX + u2)
-         had_overflow = true;
-      }
-      else if (u2 > 0) {
-       if (*u < INT_MIN + u2)
-         had_overflow = true;
-      }
-      if (had_overflow) {
+      if (ckd_sub(u, *u, u2)) {
        error("subtraction overflow");
        return false;
       }
-      *u -= u2;
       break;
     case '*':
-      if (u2 < 0) {
-       if (*u > 0) {
-         if ((unsigned)*u > -(unsigned)INT_MIN / -(unsigned)u2)
-           had_overflow = true;
-       }
-       else if (-(unsigned)*u > INT_MAX / -(unsigned)u2)
-         had_overflow = true;
-      }
-      else if (u2 > 0) {
-       if (*u > 0) {
-         if (*u > INT_MAX / u2)
-           had_overflow = true;
-       }
-       else if (-(unsigned)*u > -(unsigned)INT_MIN / u2)
-         had_overflow = true;
-      }
-      if (had_overflow) {
+      if (ckd_mul(u, *u, u2)) {
        error("multiplication overflow");
        return false;
       }
-      *u *= u2;
       break;
     case '/':
       if (u2 == 0) {


This is enough to fix the core dump observed in comment #1, which I just failed to reproduce.

Unfortunately, things go badly to hell when I add the last piece:


diff --git a/src/roff/troff/hvunits.h b/src/roff/troff/hvunits.h
index 2f78afc4b..cd48360bf 100644
--- a/src/roff/troff/hvunits.h
+++ b/src/roff/troff/hvunits.h
@@ -71,6 +71,7 @@ public:
   friend inline int operator >=(const hunits&, const hunits&);
   friend inline int operator ==(const hunits&, const hunits&);
   friend inline int operator !=(const hunits&, const hunits&);
+  friend bool get_hunits(hunits *, unsigned char, hunits);
 };

 extern const hunits H0;
diff --git a/src/roff/troff/number.cpp b/src/roff/troff/number.cpp
index e43340144..2d277ef64 100644
--- a/src/roff/troff/number.cpp
+++ b/src/roff/troff/number.cpp
@@ -148,10 +148,12 @@ bool get_hunits(hunits *res, unsigned char si, hunits prev_value)
     *res = h;
     break;
   case INCREMENT:
-    *res = prev_value + h;
+    if (ckd_add(&(res->n), prev_value.n, h))
+      warning(WARN_RANGE, "integer addition saturated");
     break;
   case DECREMENT:
-    *res = prev_value - h;
+    if (ckd_sub(&(res->n), prev_value.n, h))
+      warning(WARN_RANGE, "integer subtraction saturated");
     break;
   default:
     assert(0 == "unhandled case returned by get_incr_number()");


Applying this causes 13 test failures.

I apparently have some work to do regarding overloaded operators.

 G. Branden Robinson <gbranden>
Group administrator
Sat 22 Jun 2024 02:28:36 AM UTC, comment #1: 

Fixing summary.  GNU troff is susceptible to integer overflow full stop.  It's `-ftrapv` that helps expose this fact.

 G. Branden Robinson <gbranden>
Group administrator
Tue 13 Jun 2023 09:52:33 PM UTC, original submission:  

If you build GNU troff with `-ftrapv`, you can construct input that will cause a core dump.

Apply this patch to tmac/pdf.tmac:


diff --git a/tmac/pdf.tmac b/tmac/pdf.tmac
index 6a2fa7bba..fdf1da7af 100644
--- a/tmac/pdf.tmac
+++ b/tmac/pdf.tmac
@@ -36,7 +36,13 @@ am solely responsible for any bugs I may have introduced into this file.
 .mso ps.tmac
 .
 .de pdf:SS
-.      fchar \\$1 \\S'16'\\$1\\S'0'
+.  ps
+.  nr pdf:prevps \\n[.ps]
+.  ps
+.  nr pdf:ps \\n[.ps]
+.  fchar \\$1 \s'\\n[pdf:ps]u*89u/100u'\S'16'\\$1\S'0'\s'\\n[pdf:prevps]u'
+.  ps \\n[pdf:prevps]
+.  ps \\n[pdf:ps]
 ..
 .pdf:SS \[+h]
 .pdf:SS \[ts]


(The above may have an error; that's not the point.)


$ cat EXPERIMENTS/oversized-greek.roff
.sp
.EQ
f ( pi ) + 2 pi int sin ( omega t ) dt + e sup { i pi sup { rho + 1 } }
.EN
$ ./build/test-groff -e -Tpdf -z EXPERIMENTS/oversized-greek.roff
troff:.../../tmac/pdf.tmac:74: error: numeric overflow
troff: backtrace: '.../build/../tmac/pdf.tmac':45: macro 'pdf:SS'
troff: backtrace: file '.../build/../tmac/pdf.tmac':74
troff: backtrace: file '.../build/../tmac/troffrc':33
troff:.../build/../tmac/pdf.tmac:74: error: numeric overflow
.../build/groff: error: troff: Illegal instruction (core dumped)
[running gdb]
Core was generated by `troff -b -ww -Tpdf'.
Program terminated with signal SIGILL, Illegal instruction.
#0  0x0000562d89c7114e in get_number (res=res@entry=0x7fffd477c434, si=si@entry=122 'z', prev_value=<optimized out>)
    at ../src/roff/troff/number.cpp:165
165         *res = prev_value + v;
##(gdb) list
160         return 0;
161       case ABSOLUTE:
162         *res = v;
163         break;
164       case INCREMENT:
165         *res = prev_value + v;
166         break;
167       case DECREMENT:
168         *res = prev_value - v;
169         break;


https://wiki.sei.cmu.edu/confluence/display/c/INT32-C.+Ensure+that+operations+on+signed+integers+do+not+result+in+overflow

 G. Branden Robinson <gbranden>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by barx (Posted a comment)
  • -email is unavailable- added by gbranden (Submitted the item)

    •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 13 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2024-07-25 gbranden StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2024-07-20 gbranden StatusFixed In Progress
        Open/ClosedClosed Open
    2024-07-19 gbranden StatusIn Progress Fixed
        Open/ClosedOpen Closed
        Planned ReleaseNone 1.24.0
    2024-07-17 gbranden DependenciesRemoved dependency from bugs #63587 -
    2024-07-14 gbranden StatusNone In Progress
        Assigned toNone gbranden
    2024-06-22 gbranden Summary[troff] susceptible to integer overflow when built with -ftrapv [troff] susceptible to integer overflow
    2023-08-10 gbranden Dependencies- bugs #63587 is dependent
    2023-06-22 gbranden Summarysusceptible to integer overflow when built with -ftrapv [troff] susceptible to integer overflow when built with -ftrapv

    Back to the top

    Copyright © 2024 Free Software Foundation, Inc.
    Verbatim copying and distribution of this entire article is permitted in any medium, provided this notice is preserved.
    The Levitating, Meditating, Flute-playing Gnu logo is a GNU GPL'ed image provided by the Nevrax Design Team.
    Page source code

    Powered by Savane 3.13-8a8d.
    Corresponding source code