This function gracefully handled allocation failures by returning 0. Except its
caller does not check for this and assumes the returned pointer is non-null.
This also removes the conditional on `argc` in this function. The contained loop
is a no-op when `argc` is 0 and in this case `argv` is NULL, which it is well
defined to `free`.
fix: Revert "rewrite versionStr2Version to use strtoul"
This reverts commit 5aa56175c4715700cc8bf9fecad7b27665c04379. This commit
inadvertently changed the parsing of the `xdotversion` attribute to make
something like “1.7” parse as version 1.
By construction, `newopn` can only ever be non-negative here. Though in the long
term a more sweeping change should be done to count items in `size_t` variables
pervasively.
reallyroutespline: squash a -Wsign-conversion warning
By construction, `inpn` can only ever be non-negative here. Though in the long
term a more sweeping change should be done to count items in `size_t` variables
pervasively.
The substring construction in `gvplugin_graph` unfortunately cannot be easily
converted to `strview_t` because it passes pointers to `agxset` which expects a
`'\0'` terminated string. We can at least fix previously unnoticed allocation
failures though.
This fixes a memory leak that would occur when duplicate items were encountered.
The conditional block in this loop would not be entered and the memory that `q`
pointed to would be lost in the next iteration of the loop when `q` was set to a
new string.
Similar to the previous commit, this string is either `NULL` or terminated by a
`'\0'`, so there is little advantage in using a `strview_t` over a
`const char*`. But this will make it more consistent with surrounding code after
upcoming changes.
This string is always terminated by `'\0'`, not `':'`, so there is little
advantage in using a `strview_t` over a `const char*`. But upcoming changes will
refactor the surrounding code to use `strview_t`, so it will be more readable to
use `strview_t` consistently here.
2. b4947d67a4ebd48ca0105d44f92e47f044e51600 appears to have applied some
Coverity suggestions without investigating the underlying history that led
to the Coverity warnings.
When calling `merge_chain` from `interclexp`, this code apparently did not
anticipate that both the condition `ND_rank(agtail(e)) == ND_rank(aghead(e))`
and `ED_to_virt(prev) != NULL` could be true at once. In this case, the merge
can happen but the `.to_virt` member of `e` needs to be overwritten; it is not
`NULL` on entry to `merge_chain`.
This issue appears to have existed since the first revision of Graphviz.
These is a lot of uncertainty around what the expected behavior of this code is
and whether this fix is correct. So here is some of my background thoughts:¹
`merge_chain` has a leading `assert(ED_to_virt(e) == NULL)`, presumably trying
to express the assumption that it is not overwriting an existing back pointer.
But the code here in `interclexp` seems to be deliberately trying to overwrite
this pointer. There seemed to me two possible local fixes:
1. Remove the assertion
2. Write `NULL` to `ED_to_virt(e)` in advance
The first possibility seemed more risky. This would allow other functions to
accidentally call into merge_chain with a non-null `ED_to_virt(e)` and for
this to go undetected.
On the other hand, it's possible my change here is mistaken and what the
original author intended was for the true case of the branch on line 181 to
continue after updating `ED_to_virt(e) = prev`, making the call to
`merge_chain` not reached in this scenario. It certainly looks pretty weird
for the code after my changes to set `ED_to_virt(e)` on lines 181-184 and then
subsequently overwrite this value on line 187. But I was guessing the initial
write is relevant for the case where we continue.
As you can tell, all of this is (barely) educated guess work. I find the
intent of this code very hard to determine.
Gitlab: fixes #121
¹ Quoted from discussion on
https://gitlab.com/graphviz/graphviz/-/merge_requests/2724#note_1005393861.
Debian’s /etc/os-release does not include the field `ID_LIKE`, which is perhaps
reasonable as it is a base distribution not derived from another. A side effect
of this was that CPack picked the RPM back end when generating a Graphviz
package. This change teaches it that Debian should also get the DEB back end.
gvplugin_list: remove a second unnecessary duplication of 'typestr'
By managing an extent into the original string, we can avoid a dynamic
allocation in this code. This is similar in spirit to 5a13ab3f08b2066a795ea152f9594f6719d3aaa6.
gvplugin_list: remove unnecessary duplication of 'typestr'
By managing an extent into the original string, we can avoid a dynamic
allocation in this code. This is similar in spirit to 5a13ab3f08b2066a795ea152f9594f6719d3aaa6.
The code here actually looks wrong. It tries to pull out colon-separated
entries, but then uses the full string (`pnext->typestr`) in the call to
`agxbprint`. But this commit leaves this as-is for now.
gvplugin_list: remove unnecessary duplication of 'str'
By managing an extent into the original string, we can avoid a dynamic
allocation in this code. This is similar in spirit to 5a13ab3f08b2066a795ea152f9594f6719d3aaa6.
ortho: use 'UNUSED' to suppress warnings about debug functions
As an alternative to hiding these functions by guarding them with `DEBUG`, this
allows the compiler to see and parse these functions even when not building in
debug mode. This means we can catch syntax errors that slip into this code even
when it is not being emitted into the final binary.
This has little effect right now as debugging is forced on in this file, but
this may be a nice extra safe guard in future.
Magnus Jacobsson [Sun, 29 May 2022 09:04:49 +0000 (11:04 +0200)]
update "Starting development of the next version" in DEVELOPERS.md
The "returning to the development series" wording was a remnant from
when stable releases had even minor versions and development versions
had odd minor versions. There no longer are different stable and
development series so there's nothing to return to. We always go
forward. The development versions have the same version number as the
tentative next stable release version, with a ~dev suffix.
This reverts commit 05cbff4882df91d1b05a498b3b664061aefd3fce. Given this CMake
option does not work on all platforms (e.g. it adds compiler options that will
error out when using MSVC) and has inconsistent effects on supported platforms
(e.g. it enables leak detection on Linux, but not on macOS), it seems safer to
admit we cannot abstract over this feature and remove it from the build system.
Users who want to enable sanitizers can still do this selectively and with
greater control by exporting `CFLAGS` and `CXXFLAGS` or by using the
`CMAKE_C_FLAGS` and `CMAKE_CXX_FLAGS` to add toolchain-specific options.
When enabling ASan and UBSan, ASan defaults to aborting execution on a detected
error while UBSan defaults to a warning and then continuing. The effect of this
is that any UBSan issues go unnoticed in CI when no one is monitoring stderr.
This change makes UBSan issues also have abort behavior.
Continuing in the face of an overflow when calculating rectangle area was sort
of pointless. The test case for #1906 is an example of this, that runs for many
minutes _repeatedly_ detecting overflows in this code before finally erroring
out when trying to do follow on layout with values that make no sense. It is
simpler and cheaper to just error out immediately when this happens.
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.
projects / graphviz / log