From: Matthew Fernandez <matthew.fernandez@gmail.com>
Date: Sun, 11 Dec 2022 19:42:01 +0000 (-0800)
Subject: fix out-of-bounds memory reads with large 'style' strings
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=256e23ce4e3ff6fc18a7c13025f3263aaf91bcab;p=graphviz

fix out-of-bounds memory reads with large 'style' strings

Parsing a large style would acquire pointers into the `agxbuf` it wrote into,
but then go on to append more data to the buffer, potentially triggering a
reallocation thus leaving those prior pointers dangling. Later attempts to
access the style would read from these (now invalid) pointers. This bug has
existed since the very first commit of Graphviz.

Note that this change does nothing to alter the NUL-separated-string design of
the return value of `parse_style`. This still remains because some callers rely
on it, despite it being known as problematic.

Gitlab: fixes #2325
---

diff --git a/CHANGELOG.md b/CHANGELOG.md
index ad3c895de..435840e79 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased (7.0.5)]
 
+### Fixed
+
+- Using `style` attributes in excess of 128 bytes and/or 63 individual styles no
+  longer results in out-of-bounds memory accesses. #2325
+
 ## [7.0.4] – 2022-12-03
 
 ### Fixed
diff --git a/lib/common/emit.c b/lib/common/emit.c
index cc6a3a41e..ee04cbda3 100644
--- a/lib/common/emit.c
+++ b/lib/common/emit.c
@@ -3809,6 +3809,7 @@ static agxbuf ps_xb;
 char **parse_style(char *s)
 {
     static char *parse[FUNLIMIT];
+    size_t parse_offsets[sizeof(parse) / sizeof(parse[0])];
     static bool is_first = true;
     size_t fun = 0;
     bool in_parens = false;
@@ -3852,7 +3853,7 @@ char **parse_style(char *s)
 		    return parse;
 		}
 		agxbputc(&ps_xb, '\0');	/* terminate previous */
-		parse[fun++] = agxbnext(&ps_xb);
+		parse_offsets[fun++] = agxblen(&ps_xb);
 	    }
 	    agxbput_n(&ps_xb, c.start, c.size);
 	    agxbputc(&ps_xb, '\0');
@@ -3864,8 +3865,15 @@ char **parse_style(char *s)
 	parse[0] = NULL;
 	return parse;
     }
+
+    char *base = agxbuse(&ps_xb); // add final '\0' to buffer
+
+    // construct list of style strings
+    for (size_t i = 0; i < fun; ++i) {
+        parse[i] = base + parse_offsets[i];
+    }
     parse[fun] = NULL;
-    (void)agxbuse(&ps_xb);		/* adds final '\0' to buffer */
+
     return parse;
 }
 
diff --git a/tests/test_regression.py b/tests/test_regression.py
index 946526f98..faa79a365 100644
--- a/tests/test_regression.py
+++ b/tests/test_regression.py
@@ -2032,7 +2032,6 @@ def test_2307():
     assert re.search(r"\bG2_", m.group("url")) is not None, \
       "ID G2 was not applied to polygon fill url"
 
-@pytest.mark.xfail()
 def test_2325():
   """
   using more than 63 styles and/or more than 128 style bytes should not trigger