fix incorrect allocation computation in strdup_and_subst_obj0

The logic in this function does two passes through computing the construction of
a string, the first to determine how many bytes will be written and the second
to write those bytes. Unfortunately the logic in these two passes did not match,
and some cases would cause the first pass to underestimate how many bytes would
be written by the second pass. As a result, the second pass would overrun the
extent of allocated memory and begin writing out of bounds.

As far as I can tell, this bug has existed since at least 2010. I think this
code should be refactored in future to use a dynamically expanding string buffer
(e.g. agxbuf) instead of relying on this fragile pairing of logic.

The test case added in this commit was something found by Google Autofuzz
project that triggered a crash in Graphviz due to reading a NULL pointer. After
fixing the initial crash (the previous commit), this out of bounds write was
observable using ASan. This commit fixes #1676.

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5303ba2b994c1f906665406f04c4624038ea0927..9ed978b7257983a07a78172c8953d6d6f037c869 100644 (file)
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Segfault in dot #1783
 - Incorrect 'Arrow type "s" unknown' error #1444
 - segfault on reading 0x10 #1724
+- Null-dereference READ (144736912) #1676
 
 ## [2.44.1] - 2020-06-29
 

diff --git a/lib/common/labels.c b/lib/common/labels.c
index d2fab01762fe4b655d48421fb7ffcdb5e666e2b8..e46c3955c669b9c3b65b9e5676a2cce6b85aee91 100644 (file)
--- a/lib/common/labels.c
+++ b/lib/common/labels.c
@@ -365,7 +365,19 @@ static char *strdup_and_subst_obj0 (char *str, void *obj, int escBackslash)
                newlen += n_len;
                break;
            case 'E':
-               newlen += e_len;
+               if (isEdge) {
+                   newlen += t_len;
+                   if (tp_len) {
+                       newlen++;
+                       newlen += tp_len;
+                   }
+                   newlen += e_len;
+                   newlen += h_len;;
+                   if (hp_len) {
+                       newlen++;
+                       newlen += hp_len;
+                   }
+               }
                break;
            case 'H':
                newlen += h_len;

diff --git a/rtest/1676.dot b/rtest/1676.dot
new file mode 100644 (file)
index 0000000..4a687da
--- /dev/null
+++ b/rtest/1676.dot
@@ -0,0 +1,10 @@
+digraph G{
+eoe =0nnodeÏÅ\91
+       newrank=
+RD;    noArtG_dba->i\8d¼q_bÿÿ
+ ,zWdic->\91,UTF-8qdic->n:i;     noArtG_dba->i\8d¼q_bÿÿ
+ ,zWdic->\91,wÿÿadqdic->n:ie1.024W
+w,n nshaxxxw_\85sa\9d\9d\8c\9bwÍ##\eÜÜÜÜÜÜÜÜÜÜÜÜ\9cÜÜÜÜÜÜÜÜ B->digraph G{
+eoe =0pack=88 <a]ª[a selgraòÿÿÿÿÿÿpe sam-> _2ÿÖåååååååååååååååååååååååååååååååååååååååååååååååååÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿ0 5rank=max}kw4ack=0rhph{PeÏÅ\91
+       newrank4W
+w,n\91,wÿÿadq\1cic!-
\ No newline at end of file

diff --git a/rtest/test_regression.py b/rtest/test_regression.py
index 9db10d22c02c69d7932fed0a5cdbe790c98da4d8..2ef4f434a468061925465e1cd7843bdea1968eb9 100644 (file)
--- a/rtest/test_regression.py
+++ b/rtest/test_regression.py
@@ -219,6 +219,21 @@ def test_1594():
     assert 'line 3:' in stderr, \
       'GVPR did not identify correct line of syntax error'
 
+def test_1676():
+    '''
+    https://gitlab.com/graphviz/graphviz/-/issues/1676
+    '''
+
+    # locate our associated test case in this directory
+    input = os.path.join(os.path.dirname(__file__), '1676.dot')
+    assert os.path.exists(input), 'unexpectedly missing test case'
+
+    # run Graphviz with this input
+    ret = subprocess.call(['dot', '-Tsvg', '-o', os.devnull, input])
+
+    # this malformed input should not have caused Graphviz to crash
+    assert ret != -signal.SIGSEGV, 'Graphviz segfaulted'
+
 def test_1724():
     '''
     passing malformed node and newrank should not cause segfaults