fix: remove hard limit of 1000 boxes in dot spline code

The dot spline code used a static array of 1000 box data structures. It is not
clear to me why this limit was thought to be enough. I suspect the limit (added
in the initial Graphviz revision) was just thought to be something sufficiently
large that no user would ever hit it.

This is no longer true. The graph in issue #2095 exceeds this limit. Because
there are no bounds checks when moving through the boxes array, this resulted in
a crash due to a write beyond the end of the array.

In this commit, we remove this limit entirely and instead use a dynamically
allocated expanding array of boxes. This permits handling an arbitrary number of
boxes during computation. Fixes #2095.

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 6a8943e64d73819c0aa20dd0d83f2c493567e8bf..e2656b27f3a35b43b375d0996a126fe2741e01f4 100644 (file)
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -14,6 +14,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - incorrectly using the `layout` attribute on anything other than a graph now
   results in a warning about this being invalid #2078
 
+### Fixed
+
+- The attached dot file causes a segfault when processed #2095
+
 ## [2.48.0] - 2021-07-17
 
 ### Added

diff --git a/lib/dotgen/dotsplines.c b/lib/dotgen/dotsplines.c
index b8e83486b29aed0a74a65397a36d791b6a7ad4f6..6d9ac49edff53fff222dafa3c00fb4b1574df21d 100644 (file)
--- a/lib/dotgen/dotsplines.c
+++ b/lib/dotgen/dotsplines.c
@@ -14,9 +14,12 @@
  */
 
 #include <assert.h>
+#include <common/memory.h>
 #include <dotgen/dot.h>
+#include <limits.h>
 #include <math.h>
-#include <stddef.h>
+#include <stdlib.h>
+#include <string.h>
 
 #ifdef ORTHO
 #include <ortho/ortho.h>
@@ -51,7 +54,58 @@
        ED_to_orig(newp) = old; \
 }
 
-static boxf boxes[1000];
+// a dynamically expanding array of boxes
+typedef struct {
+  boxf *data;
+  size_t size;
+  size_t capacity;
+} boxes_t;
+
+/** Add an entry to the end of a boxes array.
+ *
+ * This may expand the array if it is not already large enough to contain the
+ * new element.
+ *
+ * \param boxes Array to append to.
+ * \param item Element to append.
+ */
+static void boxes_append(boxes_t *boxes, boxf item) {
+
+  assert(boxes != NULL);
+
+  // do we need to expand the array?
+  if (boxes->size == boxes->capacity) {
+    size_t c = boxes->capacity == 0 ? 128 : (boxes->capacity * 2);
+    boxes->data = grealloc(boxes->data, c * sizeof(boxes->data[0]));
+    boxes->capacity = c;
+  }
+
+  boxes->data[boxes->size] = item;
+  ++boxes->size;
+}
+
+/** Remove all entries from a boxes array.
+ *
+ * \param boxes Array to clear.
+ */
+static void boxes_clear(boxes_t *boxes) {
+  assert(boxes != NULL);
+  boxes->size = 0;
+}
+
+/** Deallocate memory associated with a boxes array.
+ *
+ * Following a call to this function, the array is reusable as if it had just
+ * been initialized.
+ *
+ * \param boxes Array to deallocate.
+ */
+static void boxes_free(boxes_t *boxes) {
+  assert(boxes != NULL);
+  free(boxes->data);
+  memset(boxes, 0, sizeof(*boxes));
+}
+
 typedef struct {
     int LeftBound, RightBound, Splinesep, Multisep;
     boxf* Rank_box;
@@ -1735,7 +1789,7 @@ make_regular_edge(graph_t* g, spline_info_t* sp, path * P, edge_t ** edges, int
     pointf *ps;
     pathend_t tend, hend;
     boxf b;
-    int boxn, sl, si, smode, i, j, dx, pn, hackflag, longedge;
+    int sl, si, smode, i, j, dx, pn, hackflag, longedge;
     static pointf* pointfs;
     static pointf* pointfs2;
     static int numpts;
@@ -1795,7 +1849,7 @@ make_regular_edge(graph_t* g, spline_info_t* sp, path * P, edge_t ** edges, int
     }
     else {
        int splines = et == ET_SPLINE;
-       boxn = 0;
+       boxes_t boxes = {0};
        pointn = 0;
        segfirst = e;
        tn = agtail(e);
@@ -1812,7 +1866,7 @@ make_regular_edge(graph_t* g, spline_info_t* sp, path * P, edge_t ** edges, int
        smode = FALSE, si = -1;
        while (ND_node_type(hn) == VIRTUAL && !sinfo.splineMerge(hn)) {
            longedge = 1;
-           boxes[boxn++] = rank_box(sp, g, ND_rank(tn));
+           boxes_append(&boxes, rank_box(sp, g, ND_rank(tn)));
            if (!smode
                && ((sl = straight_len(hn)) >=
                ((GD_has_labels(g->root) & EDGE_LABEL) ? 4 + 1 : 2 + 1))) {
@@ -1821,7 +1875,7 @@ make_regular_edge(graph_t* g, spline_info_t* sp, path * P, edge_t ** edges, int
            }
            if (!smode || si > 0) {
                si--;
-               boxes[boxn++] = maximal_bbox(g, sp, hn, e, ND_out(hn).list[0]);
+               boxes_append(&boxes, maximal_bbox(g, sp, hn, e, ND_out(hn).list[0]));
                e = ND_out(hn).list[0];
                tn = agtail(e);
                hn = aghead(e);
@@ -1834,7 +1888,9 @@ make_regular_edge(graph_t* g, spline_info_t* sp, path * P, edge_t ** edges, int
            if (b.LL.x < b.UR.x && b.LL.y < b.UR.y)
                hend.boxes[hend.boxn++] = b;
            P->end.theta = M_PI / 2, P->end.constrained = TRUE;
-           completeregularpath(P, segfirst, e, &tend, &hend, boxes, boxn, 1);
+           assert(boxes.size <= (size_t)INT_MAX && "integer overflow");
+           completeregularpath(P, segfirst, e, &tend, &hend, boxes.data,
+                               (int)boxes.size, 1);
            if (splines) ps = routesplines(P, &pn);
            else {
                ps = routepolylines (P, &pn);
@@ -1844,8 +1900,10 @@ make_regular_edge(graph_t* g, spline_info_t* sp, path * P, edge_t ** edges, int
                    pn = 4;
                }
            }
-           if (pn == 0)
+           if (pn == 0) {
+               boxes_free(&boxes);
                return;
+           }
        
            if (pointn + pn > numpts) {
                 /* This should be enough to include 3 extra points added by
@@ -1862,7 +1920,7 @@ make_regular_edge(graph_t* g, spline_info_t* sp, path * P, edge_t ** edges, int
            segfirst = e;
            tn = agtail(e);
            hn = aghead(e);
-           boxn = 0;
+           boxes_clear(&boxes);
            tend.nb = maximal_bbox(g, sp, tn, ND_in(tn).list[0], e);
            beginpath(P, e, REGULAREDGE, &tend, spline_merge(tn));
            b = makeregularend(tend.boxes[tend.boxn - 1], BOTTOM,
@@ -1872,7 +1930,7 @@ make_regular_edge(graph_t* g, spline_info_t* sp, path * P, edge_t ** edges, int
            P->start.theta = -M_PI / 2, P->start.constrained = TRUE;
            smode = FALSE;
        }
-       boxes[boxn++] = rank_box(sp, g, ND_rank(tn));
+       boxes_append(&boxes, rank_box(sp, g, ND_rank(tn)));
        b = hend.nb = maximal_bbox(g, sp, hn, e, NULL);
        endpath(P, hackflag ? &fwdedgeb.out : e, REGULAREDGE, &hend, spline_merge(aghead(e)));
        b.UR.y = hend.boxes[hend.boxn - 1].UR.y;
@@ -1881,8 +1939,10 @@ make_regular_edge(graph_t* g, spline_info_t* sp, path * P, edge_t ** edges, int
                   ND_coord(hn).y + GD_rank(g)[ND_rank(hn)].ht2);
        if (b.LL.x < b.UR.x && b.LL.y < b.UR.y)
            hend.boxes[hend.boxn++] = b;
-       completeregularpath(P, segfirst, e, &tend, &hend, boxes, boxn,
-                       longedge);
+       assert(boxes.size <= (size_t)INT_MAX && "integer overflow");
+       completeregularpath(P, segfirst, e, &tend, &hend, boxes.data, (int)boxes.size,
+                           longedge);
+       boxes_free(&boxes);
        if (splines) ps = routesplines(P, &pn);
        else ps = routepolylines (P, &pn);
        if ((et == ET_LINE) && (pn > 4)) {

diff --git a/rtest/test_regression.py b/rtest/test_regression.py
index 8492764718c408af81e64ad18a95daa13e303786..4f9b2cde7dee36961c73a217351dc627a1fcdd32 100644 (file)
--- a/rtest/test_regression.py
+++ b/rtest/test_regression.py
@@ -1142,7 +1142,10 @@ def test_2089_2():
   sys.stderr.write(stderr)
   assert ret == 0
 
-@pytest.mark.xfail() # FIXME
+@pytest.mark.skipif(os.environ.get("build_system") == "msbuild" and
+                    os.environ.get("configuration") == "Debug",
+                    reason="Graphviz built with MSBuild in Debug mode has an "
+                           "insufficient stack size for this test")
 def test_2095():
   """
   Exceeding 1000 boxes during computation should not cause a crash