From fbefeb31989130c48b965cc9a2f0ad0cac07015c Mon Sep 17 00:00:00 2001 From: Matthew Fernandez Date: Sun, 9 Aug 2020 16:48:30 -0700 Subject: [PATCH] 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. --- CHANGELOG.md | 1 + lib/common/labels.c | 14 +++++++++++++- rtest/1676.dot | 10 ++++++++++ rtest/test_regression.py | 15 +++++++++++++++ 4 files changed, 39 insertions(+), 1 deletion(-) create mode 100644 rtest/1676.dot diff --git a/CHANGELOG.md b/CHANGELOG.md index 5303ba2b9..9ed978b72 100644 --- 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 d2fab0176..e46c3955c 100644 --- 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 index 000000000..4a687dac1 --- /dev/null +++ b/rtest/1676.dot @@ -0,0 +1,10 @@ +digraph G{ +eoe =0nnodeÏő + newrank= +RD; noArtG_dba->i¼q_bÿÿ + ,zWdic->‘,UTF-8qdic->n:i; noArtG_dba->i¼q_bÿÿ + ,zWdic->‘,wÿÿadqdic->n:ie1.024W +w,n nshaxxxw_…saŒ›wÍ##ÜÜÜÜÜÜÜÜÜÜÜܜÜÜÜÜÜÜÜÜ B->digraph G{ +eoe =0pack=88  _2ÿÖåååååååååååååååååååååååååååååååååååååååååååååååååÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿ0 5rank=max}kw4ack=0rhph{PeÏő + newrank4W +w,n‘,wÿÿadqic!- \ No newline at end of file diff --git a/rtest/test_regression.py b/rtest/test_regression.py index 9db10d22c..2ef4f434a 100644 --- 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 -- 2.40.0