The test case for #1906 triggers an overflow in this function, viewable with
UBSan:
$ dot -Kirco -Tgv -o /dev/null tests/graphs/root.gv
/builds/graphviz/graphviz/lib/label/xlabels.c:35:15: runtime error: signed
integer overflow: -1884993080 - 1219985688 cannot be represented in type
'int'
CI: manage ASan, UBSan enabling through env instead of 'use_sanitizers'
This will allow us more precise control over compiler flags. Managing these
through CMake settings is challenging when enabling these is toolchain dependent
(e.g. the existing `use_sanitizers` option does not work with MSVC) and
operating system dependent (e.g. ASan leak detection is enabled by default on
Linux but not on macOS).
fix: anticipate non-normal edges during path construction
When constructing a path, the `beginpath` and `endpath` functions assumed they
could follow a chain of `.to_orig` back pointers to eventually reach a normal
edge. However this is not necessarily true, a situation exemplified by
tests/graphs/b15.gv that caused these traversal loops to eventually reach the
start of this linked list and then dereference a null pointer.
The fix in this commit is to simply treat the head of the list as the original
edge if we have not encountered a normal edge before then. Whether this is
correct or not seems unimportant, as a graph that causes this scenario is
incorrect. This change turns a crash during path construction into a graceful
exit later when the lack of normal edges is discovered.
This fix has similarities with 84e468e775e1d1b293624f1c8e70c226eb4a6e41. Perhaps
the code base should be audited for all such traversal loops, which seem to have
been written prior to a time when constructing a non-normal-edge-containing list
became possible.
Unfortunately the xfail in this commit cannot be marked `strict=True` because
the ASan CI job turns the deterministic crash of this test case into a
`exit(1)`.
fix: use '-module -avoid-version' when compiling TCL packages
Quoting from #1285:
They are runtime loadable (dlopen, or equivalent, from tcl program, via
'load') rather than shared libraries for dynamic linking by others. On OS X,
these two concepts have different extensions (.so vs .dylib). It's confusing
when a runtime-loadable module has a dynamic-linker extension. In commit 40123aedcd2761e98d8c9917be6040ea6187c97f, the -module flag was added to
LDFLAGS in tclpkg/gv/Makefile.am, which fixes libgv_tcl.
Could the same change be applied to the other tclpkg/*/Makefile.am LDFLAGS?
fix: include Windows links in deployment-produced JSON
This is a second attempt at 6117abe680037824d134149b0de42f589fb24466. It seems
the previous attempt did not account for the fact that the source path of an
artifact has not yet been safely escaped into a filename and contains directory
separators (`/`). This change was made by studying the last deploy.py execution
logs and making a best guess. There is no easy way to test this other than
running the release deployment process, so we should do one soon to validate
this.
avoid dynamic allocation of token buffer during style parsing
The style parsing code repeatedly calls `style_token` to tokenize input. It was
extracting the token into a dynamic buffer, `xb`. However, this is unnecessary.
Instead we can yield tokens as base and offset into the original input string,
avoiding heap allocation altogether.
It is possible this approach could be pushed “outwards,” applying the same
optimization to construction of the style itself that is returned by
`parse_style`. This would remove what the leading comment describes as “one of
the worst internal designs in graphviz.” However, this would be an API break,
and a pretty subtle one. The style buffer of `'\0'` separated strings is
available to plugins and applications in the GVC `.rawstyle` field. Altering
how this field represents the style could lead to uncaught and confusing
downstream problems. If this kind of change is done in future, I would recommend
renaming the field entirely to make any external uses break loudly at
compilation time.
Costa Shulyupin [Mon, 16 May 2022 07:05:55 +0000 (10:05 +0300)]
fix ortho: add epsilon to floating point comparison
Thanks to good bug description #1408, I've compared logs of
bad and good version. I found that sometimes there is
difference ~1E-15 between t->hi.y and t->lo.y because of
precision issues. It caused false positive conditional statement.
The workaround is to tolerate insignificant deviations.
CMake: link -lm globally on Unix instead of fine-grained
It is simpler to express this dependency globally than to try to manage a
dependency on such a fundamental part of the C standard library on a
case-by-case basis.
Commit 4915673bd8826641c4227bb6e32e1d759f3b1df5 introduced a bug where
`gvprintf` was cast and stored into an incompatible pointer type, one returning
an `int` instead of `void`. On platforms like x86-64, this coincidentally works
out with no ill effects. But on platforms like Web Assembly this prevents using
plugins containing this code.
common: squash spurious -Wconversion warnings in 'new_virtual_edge'
By doing these assignments all in a single line, the compiler was fooled into
believing there was some unintentional narrowing happening:
fastgr.c: In function 'new_virtual_edge':
../../lib/common/types.h:592:22: warning: conversion from 'int' to 'short int'
may change value [-Wconversion]
592 | #define ED_weight(e) (((Agedgeinfo_t*)AGDATA(e))->weight)
| ^
fastgr.c:192:55: note: in expansion of macro 'ED_weight'
192 | ED_minlen(e) = ED_count(e) = ED_xpenalty(e) = ED_weight(e) = 1
| ^~~~~~~~~
../../lib/common/types.h:569:21: warning: conversion to 'short unsigned int'
from 'short int' may change the sign of the result [-Wsign-conversion]
569 | #define ED_count(e) (((Agedgeinfo_t*)AGDATA(e))->count)
| ^
fastgr.c:192:24: note: in expansion of macro 'ED_count'
192 | ED_minlen(e) = ED_count(e) = ED_xpenalty(e) = ED_weight(e) = 1
| ^~~~~~~~
We can make it clearer and avoid these warnings by separating the assignments.
smyrna: propagate and remove 'fullscreen' parameter to 'cb_glutinit'
This function did not properly support non-fullscreen mode, as evidenced by the
`x` and `y` parameters (removed in the previous commit) that it was ignoring.
dotgen: squash -Wsign-conversion warnings in 'medians'
The compiler says:
mincross.c: In function 'medians':
mincross.c:1803:25: warning: conversion to 'size_t' {aka 'long unsigned int'}
from 'int' may change the sign of the result [-Wsign-conversion]
1803 | qsort(list, j, sizeof(int), (qsort_cmpf) ordercmpf);
| ^
By doing array indexing in this function universally as `size_t` we can avoid
this.
mincross.c:1404:17: warning: declaration of 'n' shadows a previous local
[-Wshadow]
1404 | int n, ndiv2;
| ^
mincross.c:1359:13: note: shadowed declaration is here
1359 | node_t *n, *n0;
| ^
We can trivially rephrase the code to avoid this noise.
projects / graphviz / log