[bug #64301] [troff] susceptible to integer overflow

[G. Branden Robinson]

Update of bug #64301 (group groff):

                  Status:                    None => In Progress            
             Assigned to:                    None => gbranden               

    _______________________________________________________

Follow-up 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.


    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/bugs/?64301>

_______________________________________________
Message sent via Savannah
https://savannah.gnu.org/

signature.asc
Description: PGP signature