common fullColor: use an agxbuf instead of a static buffer
This improves safety by removing an `sprintf` usage as well as removing a long
lived static buffer that can prove problematic for tools like Valgrind and
Address Sanitizer.
mm2gv makeDotGraph: replace 'sprintf' with 'agxbprint'
Given this function already has an agxbuf that was not in use while needing to
construct these temporaries, we can reuse it to avoid allocating a separate
buffer on the stack. This also improves safety by avoiding `sprintf`.
neatogen shortest_path: use cgraph wrapper for allocation
The lib/cgraph/alloc.h wrappers are similar to the older lib/common/memory.h
wrappers except (1) they are header-only and (2) they live in a directory
(cgraph) that is at the root of the dependency tree. The long term plan is to
replace all use of lib/common/memory.h with lib/cgraph/alloc.h.
neatogen scan_graph_mode: use cgraph wrappers for allocation
The lib/cgraph/alloc.h wrappers are similar to the older lib/common/memory.h
wrappers except (1) they are header-only and (2) they live in a directory
(cgraph) that is at the root of the dependency tree. The long term plan is to
replace all use of lib/common/memory.h with lib/cgraph/alloc.h.
neatogen new_3array: use cgraph wrappers for allocation
The lib/cgraph/alloc.h wrappers are similar to the older lib/common/memory.h
wrappers except (1) they are header-only and (2) they live in a directory
(cgraph) that is at the root of the dependency tree. The long term plan is to
replace all use of lib/common/memory.h with lib/cgraph/alloc.h.
neatogen new_array: use cgraph wrappers for allocation
The lib/cgraph/alloc.h wrappers are similar to the older lib/common/memory.h
wrappers except (1) they are header-only and (2) they live in a directory
(cgraph) that is at the root of the dependency tree. The long term plan is to
replace all use of lib/common/memory.h with lib/cgraph/alloc.h.
This switch is exhaustive. This change squashes the following warnings:
draw.c:341:9: warning: switch missing default case [-Wswitch-default]
341 | switch (o->op.u.text.align)
| ^~~~~~
draw.c:374:9: warning: ‘x’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
374 | glCompDrawText3D(o->font,x,y,view->Topview->global_z…
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~…
This seems to be a false positive, but to figure this out requires non-trivial
reasoning about the control flow in this function. We may as well remove the
need to think about this. This squashes these warnings:
neatosplines.c: In function ‘makeObstacle’:
neatosplines.c:360:37: warning: ‘margin.y’ may be used uninitialized in this
function [-Wmaybe-uninitialized]
360 | ymargin = -margin.y;
| ~~~~~~~~^~~~~~~~~~~
neatosplines.c:351:37: warning: ‘margin.x’ may be used uninitialized in this
function [-Wmaybe-uninitialized]
351 | xmargin = -margin.x;
| ~~~~~~~~^~~~~~~~~~~
dotgen leftOf: perform computation in double range
It is not clear to me why this code was implicitly converting the result of its
computation to `int`. Doing the entire thing in `double` is more precise and
squashes a compiler warning.
dotgen _dot_splines: remove micro-optimization for single edge
This code was attempting to avoid heap allocation when dealing with a single
edge. This kind of micro-optimization is unnecessary with modern
allocators/compilers. The cost of anything here is most likely irrelevant
compared to the massive size of the containing function.
Labels can be either plain text or HTML-like labels (`<`, `>` delimited). When
parsing an HTML-like label, the lexer would return the same result for a warning
or an error. This meant the caller would attempt to fallback to a plain text
label in either case. But when the HTML lexer has errored, the input has been
determined unparseable. Falling back to parsing a plain text label is unlikely
to work, and even if it does it produces something that is certainly not what
the user intended. In most scenarios, this fallback behavior would go onto to
crash messily, now that labels were populated with garbage data.
This change simply teaches the calling code to notice the error and exit instead
of falling back. Exiting from within library code like this is not particularly
clean or desirable, but there is no easy elegant error path from this code.
Gitlab: fixes #1311 Reported-by: Google Autofuzz project
It is not clear why this was an `unsigned char*`. Nothing in this function
relies on unsigned semantics. Making it a `const char*` allows dropping various
casts.
Mysteriously c4205c6e132efe64f23211fe885ff37209bc6ac0 implemented this, but only
for GVPR programs. As a result, full HSVA colors could be used in GVPR programs
but specifying an HSVA color in DOT required manually translating the color to
RGBA.
This change aligns DOT color parsing with how GVPR color parsing works.
Gitlab: closes #510 Reported-by: Ryan Schmidt <gitlab@ryandesign.com>
gv2gxl createNodeId: replace char buffer with an agxbuf
This removes an assumption that `SMALLBUF` is large enough to fit the printed
string. Performance should be unaffected as agxbuf can store a string of the
length printed here inline.
gv2gxl createGraphId: replace char buffer with an agxbuf
This removes an assumption that `SMALLBUF` is large enough to fit the printed
string. Performance should be unaffected as agxbuf can store a string of the
length printed here inline.
This call to `free` looked like it was deallocating the earlier allocation
within this function. But at this point `bp` points to the return value of a
call to `addid`. This return value is a `name` member of an item stored in the
`idList` dictionary that is still in use.
The original allocated memory of `bp` was and still is leaked.
CMake: Similar to 3fcf0968, use gdlib.pc if available
The gdlib-config program is no longer installed with the newer versions
of the library as it is deprecated. The CMake build on CI for Fedora
prints the following, which is no longer the case with this patch:
CMake Warning at cmake/FindGD.cmake:63 (message):
gdlib-config not found; skipping feature checks
Call Stack (most recent call first):
contrib/diffimg/CMakeLists.txt:1 (find_package)
For posterity, the reported leak is:
=================================================================
==14034==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 256 byte(s) in 1 object(s) allocated from:
#0 0x7ff0354c190f in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69
#1 0x7ff03523e61f in FcPatternObjectInsertElt /root/fontconfig/src/fcpat.c:525
#2 0x7ff035240846 in FcPatternObjectAddWithBinding /root/fontconfig/src/fcpat.c:711
#3 0x7ff035245970 in FcPatternAppend /root/fontconfig/src/fcpat.c:1262
#4 0x7ff035270c77 in FcParsePattern /root/fontconfig/src/fcxml.c:3110
#5 0x7ff035271292 in FcEndElement /root/fontconfig/src/fcxml.c:3236
#6 0x7ff02b099354 (/lib/x86_64-linux-gnu/libxml2.so.2+0x17d354)
Indirect leak of 32 byte(s) in 1 object(s) allocated from:
#0 0x7ff0354c12d7 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:77
#1 0x7ff035239d8b in FcValueListCreate /root/fontconfig/src/fcpat.c:136
#2 0x7ff035240433 in FcPatternObjectAddWithBinding /root/fontconfig/src/fcpat.c:687
#3 0x7ff035245970 in FcPatternAppend /root/fontconfig/src/fcpat.c:1262
#4 0x7ff035270c77 in FcParsePattern /root/fontconfig/src/fcxml.c:3110
#5 0x7ff035271292 in FcEndElement /root/fontconfig/src/fcxml.c:3236
#6 0x7ff02b099354 (/lib/x86_64-linux-gnu/libxml2.so.2+0x17d354)
SUMMARY: AddressSanitizer: 288 byte(s) leaked in 2 allocation(s).
use 'which' when accessing gv2gxl, gxl2gv in the test suite
When running the Graphviz test suite on an installation that was built without
some optional components, calls to these missing components can incorrectly
resolve to those from a prior Graphviz installation that happens to be in your
`$PATH`. This issue was partly addressed in 515c86a923601db5cb704da93046800df7da030a. This change takes things further,
ensuring that invocations of `gv2gxl` and `gxl2gv` also land on the right
version under test.
This requires temporarily xfailing the test due to an unrelated issue with these
tools.
The definition of `Sfio_t` was structured to give it a public part and a private
part:
struct _sfio_s {
… public members …
_SFIO_PRIVATE; // ← only visible if sfio_t.h is included
};
This works only as long as no user ever allocates a `Sfio_t` themselves. If they
do, they will allocate too few bytes, based on their view of the size of
`Sfio_t` missing its private members.
A side effect of link-time optimization is that the compiler can see through
this trickery and witness both with-private and without-private definitions at
once. Creating objects of this type or pointers to objects of this type is a
violation of C’s strict aliasing rule and the compiler can now see this. This
can cause the compiler to make unsafe optimizations, like concluding any code
involving `Sfio_t*` variables must be unreachable.
The safer way to do a public/private class like this is with two structs, the
private one containing the public one as a member:
struct foo_public {
… public members …
};
struct foo_private {
struct foo_public *public;
… private members …
};
Public API functions then accept `struct foo_public*` parameters and internally
convert them to `struct foo_private*` variables in order to access the
internals:
int foo_do_something(struct foo_public *foo) {
// use something like the Linux kernel’s container_of
struct foo_private *f = container_of(foo, public);
…
}
But instead of this, we can observe that the definition of `Sfio_t` does not
need to have any public members at all, and we can make the entire type private.
We need an exception for libexpr which reaches into sfio internals.
sfio: remove macro implementations of character functions
These were implemented as an optimization to allow inlining. This is no longer
necessary on contemporary compilers with link-time optimization. Removing them
will unblock fixing some strict aliasing problems with the `Sfio_t` type.
These are present in the Autotools build system as a separate library, libsfiof,
linked into libsfio. They are also present in the MS Build build system inlined
into libsfio itself. This change copies the MS Build approach.
This did not cause problems because these sources only exist as fallback
implementations for functions that are also implemented as macros. All current
usages see the macro definitions so never call these functions.
Fixing this will allow removing the macro versions in an upcoming change.
gvmap make_map_internal: operate directly on 'xcombined' instead of 'xtemp'
Nothing appears to alias `xcombined` within this block. So we can save an
intermediate allocation and the cost of data movement by simply writing results
into their final destination.
projects / graphviz / log