This was validated by doing an exhaustive comparison of all strlen ≤2 inputs to
both the before and after function. Not bulletproof, but it is a strong signal
that the new version is functionally identical.
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'
projects / graphviz / log