Help with reviewing MR 1552 (dot.demo/neatopack.c)

This is mainly a question to @erg, but anyone can help.

I’ve supplied merge request https://gitlab.com/graphviz/graphviz/-/merge_requests/1552 which contains bug fixes for segmentation faults when running dot.demo/neatopack.c described in https://gitlab.com/graphviz/graphviz/-/issues/1800.

I wonder if you could help reviewing the patches and perhaps suggest a way to fix the last part of it which is just a workaround? The patches solves the problem, but I’m very uncertain if they are the best or even the correct way to solve them.

See also Good testcase for dot.demo/neatopack.c?

The neatopack program is meant to take an input graph, compute its connected components, lay out each using neato, pack each layout into a single graph, and render that graph in PostScript. The code is ancient. I suspect it worked at one time but we made some changes to libgvc and didn’t fix neatopack to conform. I find it odd that the tracebacks in the bug report indicate two separate failure points. Indeed, when I used lldb on the program, I came up with a third failure point. And the fix in neatopack.c just comments out the call to gvFreeLayout. The change in ccomps.c is not necessary; the documentation notes that ccomps assumes each node already has an Agnodeinfo_t structure attached. As for neatopack itself, I would just remove it. Fixing it to work correctly with libgvc probably isn’t worth it. Let me delve into the changes to gvlayout.c. Clearly, the assumption is that gvLayoutJobs should work with subgraphs. I need to check if this is really true and, if so, are there any assumptions tied to the subgraph. Thanks for looking into this.

1 Like

Thank you so much for the answers. I’m going to dig in a little deeper into the documentation. I do want to get neatopack.c working. I’ve taken this as an exercise to get better acquainted with the code and I don’t mind spending a lot more time to get it right. Also, we really need all test cases we can get.

Regarding the different backtraces; I could get different ones just by running the program many times. Also, compiling with -fsanitize=address affected the backtrace. Since the problems were caused by using uninitialized memory as a pointer, the consequences could be rather unpredictable. It was much more deterministic when using -fsanitize=address. Before fixing the first problem I got one backtrace, after that another and after fixing the second problem, a third. I wouldn’t pay that much attention to the exact backtrace.

Please note this comment in gvlayout.c: “Check that the root graph has been initialized. If not, initialize it” which was the reason I made the first fix there. Perhaps this wouldn’t be necessary either if some kind of initialization is made in neatopack.c before calling ccomps?

Also note that I consider the commented out call to gvFreeLayout in neatopack.c a workaround to get the program to finish and produce output, not a fix.

I think the call to gvLayoutJobs adds Agnodeinfo_t to the nodes but I’ll have to check.

I think you’re right, but that happens after the call to ccomps in neatopack.c

@erg I’ve updated the MR based on your input with what I think is the correct initialization. The gvFreeLayout workaround is still there tough since I don’t know how it’s supposed to work for subgraphs.