From: Matthew Fernandez <matthew.fernandez@gmail.com>
Date: Sat, 12 Dec 2020 16:45:19 +0000 (-0800)
Subject: fix: unify and correct RectArea implementations
X-Git-Tag: 2.46.1~21^2
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=0a4a329d3fb5128cc358729d06d9387547c92ce2;p=graphviz

fix: unify and correct RectArea implementations

Two implementations of RectArea were provided that implemented the overflow
check based on whether there was a larger type than unsigned int available. It
is unnecessary to maintain multiple implementations of this as the overflow
check can be written in a width-independent way.

Moreover the `LLONG_MAX > UINT_MAX` alternative was incorrect. E.g. on x86-64
where this alternative is used, if `r->boundary[i + NUMDIMS] - r->boundary[i]`
is `UINT_MAX` and the accumulated value in `a_test` is `(long long)UINT_MAX`,
the multiplication (which is done on operands promoted to long long) overflows
causing undefined behavior. In practice, you would likely get a negative value,
that then erroneously passes the overflow check.

Related to #1898. Fixes #1906.
---

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 330a3e533..b1b8fa7d6 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - memory leak in label construction
 - gvedit compilation errors out, but works if manually compiled with qt5 #1862
 - incorrect HTML BR attribute parsing code #1913
+- broken overflow checks in RectArea #1906
 
 ## [2.46.0] - 2021-01-18
 
diff --git a/lib/label/rectangle.c b/lib/label/rectangle.c
index a3285895d..1f32f27af 100644
--- a/lib/label/rectangle.c
+++ b/lib/label/rectangle.c
@@ -120,7 +120,6 @@ void PrintRect(Rect_t * r)
 | Calculate the n-dimensional area of a rectangle
 -----------------------------------------------------------------------------*/
 
-#if LLONG_MAX > UINT_MAX
 unsigned int RectArea(Rect_t * r)
 {
   int i;
@@ -130,61 +129,18 @@ unsigned int RectArea(Rect_t * r)
     if (Undefined(r))
 	return 0;
 
-    /*
-     * XXX add overflow checks
-     */
     area = 1;
     for (i = 0; i < NUMDIMS; i++) {
-      long long a_test = area * (r->boundary[i + NUMDIMS] - r->boundary[i]);
-      if( a_test > UINT_MAX) {
+      unsigned int dim = r->boundary[i + NUMDIMS] - r->boundary[i];
+      if (dim == 0) return 0;
+      if (UINT_MAX / dim < area) {
 	agerr (AGERR, "label: area too large for rtree\n");
 	return UINT_MAX;
       }
-      area = a_test;
+      area *= dim;
     }
     return area;
 }
-#else
-unsigned int RectArea(Rect_t * r)
-{
-  int i;
-  unsigned int area=1, a=1;
-  assert(r);
-
-    if (Undefined(r)) return 0;
-
-    /*
-     * XXX add overflow checks
-     */
-    area = 1;
-    for (i = 0; i < NUMDIMS; i++) {
-      unsigned int b = r->boundary[i + NUMDIMS] - r->boundary[i];
-      if (b==0) return 0;
-      a *= b;
-      if( (a / b ) != area) {
-	agerr (AGERR, "label: area too large for rtree\n");
-	return UINT_MAX;
-      }
-      area = a;
-    }
-    return area;
-}
-#endif /*LLONG_MAX > UINT_MAX*/
-#if 0 /*original code*/
-int RectArea(Rect_t * r)
-{
-    int i, area=1;
-    assert(r);
-
-    if (Undefined(r))
-        return 0;
-    area = 1;
-    for (i = 0; i < NUMDIMS; i++) {
-        area *= r->boundary[i + NUMDIMS] - r->boundary[i];
-    }
-    return area;
-}
-#endif
 
 /*-----------------------------------------------------------------------------
 | Combine two rectangles, make one that includes both.
diff --git a/rtest/test_regression.py b/rtest/test_regression.py
index fa9f249ce..df27f1397 100644
--- a/rtest/test_regression.py
+++ b/rtest/test_regression.py
@@ -559,7 +559,6 @@ def test_1869(variant: int):
     assert 'style=dashed' in output, 'style=dashed not found in DOT output'
     assert 'penwidth=2' in output, 'penwidth=2 not found in DOT output'
 
-@pytest.mark.xfail(strict=True) # FIXME
 def test_1906():
     '''
     graphs that cause an overflow during rectangle calculation should result in