fix incorrect reference counting of HTML-like strings in gvpack
When duplicating graph attributes, gvpack was calling agstrdup_html to construct
an HTML-like string and then agattr/agset to assign this to an attribute. It did
not anticipate that *both* of these increment the reference count for the string
that has been interned in the graph’s dictionary. The result was a reference
count of 2 for a new HTML-like string, despite only 1 reference to the string
existing.
This was not directly visible to users (hence no changelog entry for this
commit) and the extra reference counts are disposed of when the graph itself is
destroyed. However, this persistent off-by-one issue internally confused some
debugging and potentially retained memory longer than was necessary. The issue
was visible with the following patch applied:
diff --git cmd/tools/gvpack.c cmd/tools/gvpack.c
index
16a4ffa5b..
8e8fbf3b9 100644
--- cmd/tools/gvpack.c
+++ cmd/tools/gvpack.c
@@ -352,9 +352,13 @@ static void cloneAttrs(void *old, void *new)
for (a = agnxtattr(g, kind, 0); a; a = agnxtattr(g, kind, a)) {
s = agxget (old, a);
- if (aghtmlstr(s))
- agset(new, a->name, agstrdup_html(ng, s));
- else
+ if (aghtmlstr(s)) {
+ printf("creating HTML string...\n");
+ char *scopy = agstrdup_html(ng, s);
+ agrefcntprint(scopy);
+ agset(new, a->name, scopy);
+ agrefcntprint(scopy);
+ } else
agset(new, a->name, s);
}
}
diff --git lib/cgraph/cgraph.h lib/cgraph/cgraph.h
index
a043fb26d..
e2b065e16 100644
--- lib/cgraph/cgraph.h
+++ lib/cgraph/cgraph.h
@@ -305,6 +305,7 @@ CGRAPH_API int agobjkind(void *);
/* strings */
CGRAPH_API char *agstrdup(Agraph_t *, char *);
CGRAPH_API char *agstrdup_html(Agraph_t *, char *);
+CGRAPH_API void *agrefcntprint(const char *);
CGRAPH_API int aghtmlstr(char *);
CGRAPH_API char *agstrbind(Agraph_t * g, char *);
CGRAPH_API int agstrfree(Agraph_t *, char *);
diff --git lib/cgraph/refstr.c lib/cgraph/refstr.c
index
b524649ff..
d8d7ca468 100644
--- lib/cgraph/refstr.c
+++ lib/cgraph/refstr.c
@@ -9,6 +9,7 @@
*************************************************************************/
#include <cgraph/cghdr.h>
+#include <inttypes.h>
#include <stddef.h>
#include <stdio.h>
@@ -139,6 +140,11 @@ char *agstrdup_html(Agraph_t * g, char *s)
return r->s;
}
+void *agrefcntprint(const char *s) {
+ refstr_t *key = (refstr_t *) (s - offsetof(refstr_t, store[0]));
+ printf("reference count: %" PRIu64 "\n", (uint64_t)key->refcnt);
+}
+
int agstrfree(Agraph_t * g, char *s)
{
refstr_t *r;
With these changes, attempting to gvpack a graph with an HTML-like string
attribute results in the following output:
creating HTML string...
reference count: 1
reference count: 2
The present commit fixes this issue by replicating the correct pattern for doing
this kind of duplication from lib/gvpr/actions.c:copyAttr.