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

[G. Branden Robinson]

Follow-up Comment #4, bug #64301 (group groff):

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()");




    _______________________________________________________

Reply to this item at:

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

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

signature.asc
Description: PGP signature