Number: 71
Title: Complicated digraphs cause dot to segfault
Submitter: Matthew Skala
Date: Sat Jan 12 11:52:01 2002
Subsys: Dot
Version: 1.7.15
System: x86-Linux-2.4.5
Severity: major
Problem:
Some input files of moderate to high complexity cause dot to segfault. I haven't been able to characterize exactly what triggers the bug. It seems to be quite fragile - almost any change to the digraph will cause the segfault to disappear. However, as my graph becomes more and more complicated, it gets harder and harder (eventually, impossible) to find a way of expressing the input that will contain the information I want to represent without triggering the bug. The bug does *not* seem to be affected by cosmetic changes like label text and node shape; it *is* affected by anything related to layout, such as edge weight, the concentrate= flag, etc. Note that the example file I give has concentrate=true, but I'm not sure that that's necessary to trigger the bug. Disabling concentrate= on this file makes the segfault go away but I'm pretty sure I've seen other files where it segfaulted even with concentrate= turned off.

Examination with gdb traced the segfault to line 374 of dotneato/dotgen/cluster.c, in which the variable "e" is dereferenced in the condition of a while() loop even if it's null. I modified the loop condition to terminate the loop if e is null, before derefencencing it, and then the same problem showed up in line 402. I made the same change there and dot now produces apparently-correct output on the sample file I attach below. I have not yet established whether this fix really solves the problem for all similar files, whether it has unintended consequences, etc. I don't understand the significance of the variable e or the data structure it points into, so I don't know that terminating the loop when e==NULL is really the right thing to do. It does seem to work so far, however.
Input file: b71.dot
Fix:
This patch is to be applied to dotneato/dotgen/cluster.c. It's meant for demonstration purposes only - it cleans up the problem I noted, but may well create some other, worse, problem, because I don't really understand what's going on in these loops. Also, it's not written in good style (should have more parentheses and use e==NULL instead of just e).


*** old-cluster.c       Sat Jan 12 11:34:49 2002
--- cluster.c   Sat Jan 12 11:22:34 2002
***************
*** 371,377 ****
                n->u.clust = NULL;
                for (orig = agfstout(root,n); orig; orig = agnxtout(root,orig)) {
                        if ((e = orig->u.to_virt)) {
!                               while ((vn = e->head)->u.node_type == VIRTUAL)  {
                                        vn->u.clust = NULL;
                                        e = e->head->u.out.list[0];
                                }
--- 371,377 ----
                n->u.clust = NULL;
                for (orig = agfstout(root,n); orig; orig = agnxtout(root,orig)) {
                        if ((e = orig->u.to_virt)) {
!                               while (e && (vn = e->head)->u.node_type == VIRTUAL)  {
                                        vn->u.clust = NULL;
                                        e = e->head->u.out.list[0];
                                }
***************
*** 399,405 ****
                if (n->u.clust == NULL) n->u.clust = g;
                for (orig = agfstout(g,n); orig; orig = agnxtout(g,orig)) {
                        if ((e = orig->u.to_virt)) {
!                               while ((vn = e->head)->u.node_type == VIRTUAL)  {
                                        if (vn->u.clust == NULL) vn->u.clust = g;
                                        e = e->head->u.out.list[0];
                                }
--- 399,405 ----
                if (n->u.clust == NULL) n->u.clust = g;
                for (orig = agfstout(g,n); orig; orig = agnxtout(g,orig)) {
                        if ((e = orig->u.to_virt)) {
!                               while (e && (vn = e->head)->u.node_type == VIRTUAL)  {
                                        if (vn->u.clust == NULL) vn->u.clust = g;
                                        e = e->head->u.out.list[0];
                                }

Owner: north
Status: Fixed