Sequence IDs are calculated using 64-bit counters in `Agclos_s`. But then the
field used to store sequence IDs, `Agtag_s.seq`, is `sizeof(unsigned) * 8 - 4`
bits wide, 28-bit on x86 and x86-64. As a result, the compiler believes IDs that
exceed 2²⁸ - 1 can occur and overflow `Agtag_s.seq`:
edge.c:213:30: warning: conversion from 'int' to 'unsigned int:28' may change
value [-Wconversion]
213 | AGSEQ(in) = AGSEQ(out) = seq;
| ^~~
...
graph.c: In function 'agopen1':
graph.c:77:20: warning: conversion from 'uint64_t' {aka 'long unsigned int'}
to 'unsigned int:28' may change value [-Wconversion]
77 | AGSEQ(g) = agnextseq(par, AGRAPH);
| ^~~~~~~~~
...
node.c: In function 'newnode':
node.c:76:16: warning: conversion from 'uint64_t' {aka 'long unsigned int'} to
'unsigned int:28' may change value [-Wconversion]
76 | AGSEQ(n) = seq;
| ^~~
...
node.c: In function 'agnodebefore':
node.c:359:22: warning: conversion from 'uint64_t' {aka 'long unsigned int'}
to 'unsigned int:28' may change value [-Wconversion]
359 | AGSEQ(snd) = (g->clos->seq[AGNODE] + 2);
| ^
In practice, ingesting a graph of this size is not achievable, so these
overflows cannot occur.
This change introduces assertions and casts in these cases to explain the
assumptions to the compiler. It squashes the above warnings. In future, perhaps
these fields should all be made to all consistently use the same type.
gv_trim_zeros: identify string extent instead of writing a '\0'
The buffer that this function was truncating is destined for `gvwrite`. So we
can make the whole thing read-only by identifying a string extent instead of
modifying the buffer in place. The compiler may have been able to identify the
intent of this code anyway¹ but if not these changes make it clearer how this
code can be optimized.
This looks like a bit of a strange change, when we now wrap the entire file in
`extern "C"`. However this has two key benefits:
1. `dot_builtins` and `dot_static` that include this source needed an
Autotools hack¹ to force compilation to use the C++ front end (`c++`)
instead of the C front end (`cc`) in order to link against the C++ standard
library. By moving this source into C++ we can remove this hack.
2. When trying to integrate `dot_builtins` into the CMake build system, MSVC
complains (correctly) that the initializers to the array in this file are
not compile-time constants. GCC and Clang apparently allow this by a
non-standard extension. By moving this into C++, we get more relaxed
initialization semantics that allow this on all compilers.
This code is not currently compiled and, in fact, will not compile if you try to
re-enable it. As an example issue, it uses `GD_inleaf`, a macro intended for
accessing `Agraphinfo_t` fields, on a `node_t`. This is sort of a double mistake
as `Agraphinfo_t` also has no `inleaf` field. This problem seems to have been
present in the very first Graphviz revision, 256ef66663ca0c072554ee3f5e7971911031b3c7. Fortunately the mistakes sort of
cancelled each other out because the `GD_*` marcos did no casting and
`Agnodeinfo_t` _does_ have an `inleaf` field. The outcome seems to be what the
author intended, even if the route by which they got there was not intended.
The above is only one of several issues with this code. Resurrecting it has
unknown cost and unknown benefit, so we remove it here to avoid the implication
that it can be easily switched back on.
Contrary to the X11 documentation,¹ it seems button values other than 1-5 can be
returned as button press events. The assertions altered in this commit were
introduced to guarantee the value does not exceed the limits of the type of the
parameter in the user’s callback (`int`). So we can safely relax this to just
the limit itself.
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.
projects / graphviz / log