fix buffer under-read in fdp

When using fdp to process a graph, it would try to canonicalize all node names,
regardless of whether they were hosted in refstr_ts or not. The problem with
this is that the canonicalization logic assumes the character pointer passed
into it is within a refstr_t and that therefore it can do pointer subtraction to
get a pointer to the refstr_t itself. This was not true for internally
constructed node names like "%4".

We change this to always go through the refstr logic in agstrdup() when
canonicalizing names. This ensures that the pointer we pass into the
canonicalization logic *is* within a refstr_t. This is based on canon() in
lib/cgraph/output.c. Fixes #236, #1290, #1865.

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 015ee5afd52ca6e587722ebb6ab389dcb9e58de1..ca70fc6e9ef1abf6c8636401460c3ec8dc922cd6 100644 (file)
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -64,6 +64,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - buffer overflow in fdpgen
 - Crashes by VRML output when current directory is not writable #793
 - Segmentation fault when newrank=true #1221
+- sfdp craches #236
+- fdp segmentation fault with GK=0 #1290
+- fdp crash #1865
 
 ## [2.44.1] - 2020-06-29
 

diff --git a/lib/cgraph/write.c b/lib/cgraph/write.c
index dbfb57d4b188c10fcde06556489f7d01862cee4f..af920fab74e81911618d169dbe189f87503ae5a6 100644 (file)
--- a/lib/cgraph/write.c
+++ b/lib/cgraph/write.c
@@ -217,7 +217,18 @@ static int _write_canonstr(Agraph_t * g, iochan_t * ofile, char *str,
 
 static int write_canonstr(Agraph_t * g, iochan_t * ofile, char *str)
 {
-    return _write_canonstr(g, ofile, str, TRUE);
+    char *s;
+    int r;
+
+    /* str may not have been allocated by agstrdup, so we first need to turn it
+     * into a valid refstr
+     */
+    s = agstrdup(g, str);
+
+    r = _write_canonstr(g, ofile, s, TRUE);
+
+    agstrfree(g, s);
+    return r;
 }
 
 static int write_dict(Agraph_t * g, iochan_t * ofile, char *name,

diff --git a/rtest/1865.dot b/rtest/1865.dot
new file mode 100644 (file)
index 0000000..83e2b00
--- /dev/null
+++ b/rtest/1865.dot
@@ -0,0 +1,6 @@
+strict digraph "FTC/BTC" {
+subgraph cluster_Inputs {
+"Base (FTC) holding"
+}
+}
+

diff --git a/rtest/test_regression.py b/rtest/test_regression.py
index f43312e86ffae38ff90f88e7fae1506b1250509c..1daf11944234fb1c680b461e23c0bd802e590e7f 100644 (file)
--- a/rtest/test_regression.py
+++ b/rtest/test_regression.py
@@ -401,6 +401,22 @@ def test_1845():
     # generate a multipage PS file from this input
     subprocess.check_call(['dot', '-Tps', '-o', os.devnull, input])
 
+@pytest.mark.skipif(shutil.which('fdp') is None, reason='fdp not available')
+def test_1865():
+    '''
+    fdp should not read out of bounds when processing node names
+    https://gitlab.com/graphviz/graphviz/-/issues/1865
+    Note, the crash this test tries to provoke may only occur when run under
+    Address Sanitizer or Valgrind
+    '''
+
+    # locate our associated test case in this directory
+    input = os.path.join(os.path.dirname(__file__), '1865.dot')
+    assert os.path.exists(input), 'unexpectedly missing test case'
+
+    # fdp should not crash when processing this file
+    subprocess.check_call(['fdp', '-o', os.devnull, input])
+
 # root directory of this checkout
 ROOT = os.path.abspath(os.path.join(os.path.dirname(__file__), '..'))