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 ad3c895de5d8b487962d21199c8260297228443a..435840e794699b2bdf23195d3648f04931c1ee5b 100644 (file)
--- 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 cc6a3a41ea4714fcf1f5fb4e030ff8460defb653..ee04cbda332757d5d0906b4eb33c855eb4c94768 100644 (file)
--- 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 946526f98b28bc1760ea534623ca156798f6c32f..faa79a3650d40c62a450962b912378010598ec9b 100644 (file)
--- 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