From 1d62dedf83b2a3a06c6e0d1783e37656f451931c Mon Sep 17 00:00:00 2001 From: Matthew Fernandez Date: Sat, 15 Aug 2020 13:06:43 -0700 Subject: [PATCH 1/4] add a new 'disown' API for agxbuf --- CHANGELOG.md | 6 ++++++ lib/cgraph/agxbuf.c | 34 ++++++++++++++++++++++++++++++++++ lib/cgraph/agxbuf.h | 15 ++++++++++++++- 3 files changed, 54 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ed978b725..a67677542e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] + +### Added +- Cgraph's agxbuf API gained a new function agxbdisown(), for dissociating + backing memory from the managed buffer + +### Changed - Cgraph's agheap() API has been removed ### Fixed diff --git a/lib/cgraph/agxbuf.c b/lib/cgraph/agxbuf.c index 4e320670aa..6c5bfeecbc 100644 --- a/lib/cgraph/agxbuf.c +++ b/lib/cgraph/agxbuf.c @@ -110,3 +110,37 @@ int agxbpop(agxbuf * xb) return -1; } + +char *agxbdisown(agxbuf * xb) { + + size_t size; + char *buf; + + /* terminate the existing string */ + agxbputc(xb, '\0'); + + size = (size_t)(xb->ptr - xb->buf); + + if (!xb->dyna) { + /* the buffer is not dynamically allocated, so we need to copy its contents + * to heap memory + */ + + buf = malloc(size); + if (buf == NULL) { + return NULL; + } + + memcpy(buf, xb->buf, size); + + } else { + /* the buffer is already dynamically allocated, so take it as-is */ + buf = (char*)xb->buf; + } + + /* reset xb to a state where it is usable */ + xb->buf = xb->ptr = xb->eptr = NULL; + xb->dyna = 1; + + return buf; +} diff --git a/lib/cgraph/agxbuf.h b/lib/cgraph/agxbuf.h index 7cbb22088a..6a7bada96e 100644 --- a/lib/cgraph/agxbuf.h +++ b/lib/cgraph/agxbuf.h @@ -77,7 +77,10 @@ extern "C" { #define agxbputc(X,C) ((((X)->ptr >= (X)->eptr) ? agxbmore(X,1) : 0), (void)(*(X)->ptr++ = ((unsigned char)C))) /* agxbuse: - * Null-terminates buffer; resets and returns pointer to data; + * Null-terminates buffer; resets and returns pointer to data. The buffer is + * still associated with the agxbuf and will be overwritten on the next, e.g., + * agxbput. If you want to retrieve and disassociate the buffer, use agxbdisown + * instead. * char* agxbuse(agxbuf* xb) */ #define agxbuse(X) ((void)agxbputc(X,'\0'),(char*)((X)->ptr = (X)->buf)) @@ -106,6 +109,16 @@ extern "C" { */ #define agxbnext(X) ((char*)((X)->ptr)) +/* agxbdisown: + * Disassociate the backing buffer from this agxbuf and return it. The buffer is + * NUL terminated before being returned. If the agxbuf is using stack memory, + * this will first copy the data to a new heap buffer to then return. If failure + * occurs, NULL is returned. If you want to temporarily access the string in the + * buffer, but have it overwritten and reused the next time, e.g., agxbput is + * called, use agxbuse instead of agxbdisown. + */ + AGXBUF_API char *agxbdisown(agxbuf * xb); + #endif #ifdef __cplusplus -- GitLab From 18eed4fb5b151b0be7d8934203b58ffccc7d0b94 Mon Sep 17 00:00:00 2001 From: Matthew Fernandez Date: Sat, 15 Aug 2020 13:30:48 -0700 Subject: [PATCH 2/4] replace agxbuse/alloc pattern with agxbdisown Instead of retrieving the contents of a (possibly dynamically allocated) buffer and then allocating new space for a copy, we can simply take ownership of the original buffer. This saves an extra unnecessary allocation. --- cmd/smyrna/gui/menucallbacks.c | 2 +- lib/common/htmlparse.y | 2 +- lib/common/utils.c | 6 +++--- plugin/pango/gvgetfontlist_pango.c | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/cmd/smyrna/gui/menucallbacks.c b/cmd/smyrna/gui/menucallbacks.c index 39aa13fc1b..48685c7f64 100644 --- a/cmd/smyrna/gui/menucallbacks.c +++ b/cmd/smyrna/gui/menucallbacks.c @@ -431,7 +431,7 @@ static char **splitArgs(const char *args, int *argcp) asize += 10; argv = ALLOC(asize, argv, char *); } - argv[argc++] = strdup(agxbuse(&xbuf)); + argv[argc++] = agxbdisown(&xbuf); } agxbfree(&xbuf); diff --git a/lib/common/htmlparse.y b/lib/common/htmlparse.y index 4d16565337..ef11e382d9 100644 --- a/lib/common/htmlparse.y +++ b/lib/common/htmlparse.y @@ -179,7 +179,7 @@ appendFItemList (agxbuf *ag) { fitem *fi = NEW(fitem); - fi->ti.str = strdup(agxbuse(ag)); + fi->ti.str = agxbdisown(ag); fi->ti.font = HTMLstate.fontstack->cfont; dtinsert(HTMLstate.fitemList, fi); } diff --git a/lib/common/utils.c b/lib/common/utils.c index 17ba7834c8..c71f72f6d9 100644 --- a/lib/common/utils.c +++ b/lib/common/utils.c @@ -1552,7 +1552,7 @@ char* htmlEntityUTF8 (char* s, graph_t* g) } agxbputc(&xb, c); } - ns = strdup (agxbuse(&xb)); + ns = agxbdisown(&xb); agxbfree(&xb); return ns; } @@ -1591,7 +1591,7 @@ char* latin1ToUTF8 (char* s) agxbputc(&xb, (v & 0x3F) | 0x80); } } - ns = strdup (agxbuse(&xb)); + ns = agxbdisown(&xb); agxbfree(&xb); return ns; } @@ -1622,7 +1622,7 @@ utf8ToLatin1 (char* s) agxbputc(&xb, outc); } } - ns = strdup (agxbuse(&xb)); + ns = agxbdisown(&xb); agxbfree(&xb); return ns; } diff --git a/plugin/pango/gvgetfontlist_pango.c b/plugin/pango/gvgetfontlist_pango.c index e9df119112..c8c90e20d6 100644 --- a/plugin/pango/gvgetfontlist_pango.c +++ b/plugin/pango/gvgetfontlist_pango.c @@ -481,7 +481,7 @@ static char *gv_get_font(availfont_t* gv_af_p, } } } - return strdup(agxbuse(xb2)); + return agxbdisown(xb2); } } return NULL; -- GitLab From 60de2664174bc5d6ab4c8831b3318189e3889e00 Mon Sep 17 00:00:00 2001 From: Matthew Fernandez Date: Sat, 15 Aug 2020 14:04:28 -0700 Subject: [PATCH 3/4] fix: use of size_t in agxbuf.h with no definition This worked so far because files #including this always included something else that brought in stddef.h prior to #including agxbuf.h --- lib/cgraph/agxbuf.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/cgraph/agxbuf.h b/lib/cgraph/agxbuf.h index 6a7bada96e..deb2e8e9fd 100644 --- a/lib/cgraph/agxbuf.h +++ b/lib/cgraph/agxbuf.h @@ -18,6 +18,8 @@ extern "C" { #ifndef AGXBUF_H #define AGXBUF_H +#include + #ifdef _WIN32 # ifdef EXPORT_AGXBUF # define AGXBUF_API __declspec(dllexport) -- GitLab From 31bab037c9bfde3bd18e06b5ab878c09de265ccf Mon Sep 17 00:00:00 2001 From: Matthew Fernandez Date: Sat, 15 Aug 2020 14:13:34 -0700 Subject: [PATCH 4/4] use a dynamic buffer when expanding text replacements This change is intended to guard against bugs like that fixed in fbefeb31989130c48b965cc9a2f0ad0cac07015c. The buffer used to construct the result string now dynamically expands so we do not need to manually calculate its extent in advance. Related to #1676. --- lib/common/labels.c | 127 ++++++++++++-------------------------------- 1 file changed, 34 insertions(+), 93 deletions(-) diff --git a/lib/common/labels.c b/lib/common/labels.c index e46c3955c6..9be55d809a 100644 --- a/lib/common/labels.c +++ b/lib/common/labels.c @@ -11,7 +11,7 @@ * Contributors: See CVS logs. Details at http://www.graphviz.org/ *************************************************************************/ - +#include "agxbuf.h" #include "render.h" #include "htmltable.h" #include @@ -288,172 +288,113 @@ void emit_label(GVJ_t * job, emit_state_t emit_state, textlabel_t * lp) */ static char *strdup_and_subst_obj0 (char *str, void *obj, int escBackslash) { - char c, *s, *p, *t, *newstr; + char c, *s, *newstr; char *tp_str = "", *hp_str = ""; char *g_str = "\\G", *n_str = "\\N", *e_str = "\\E", *h_str = "\\H", *t_str = "\\T", *l_str = "\\L"; - size_t g_len = 2, n_len = 2, e_len = 2, - h_len = 2, t_len = 2, l_len = 2, - tp_len = 0, hp_len = 0; - size_t newlen = 0; + boolean has_hp = FALSE; + boolean has_tp = FALSE; int isEdge = 0; textlabel_t *tl; port pt; + agxbuf buf; /* prepare substitution strings */ switch (agobjkind(obj)) { case AGRAPH: g_str = agnameof((graph_t *)obj); - g_len = strlen(g_str); tl = GD_label((graph_t *)obj); if (tl) { l_str = tl->text; - if (str) l_len = strlen(l_str); } break; case AGNODE: g_str = agnameof(agraphof((node_t *)obj)); - g_len = strlen(g_str); n_str = agnameof((node_t *)obj); - n_len = strlen(n_str); tl = ND_label((node_t *)obj); if (tl) { l_str = tl->text; - if (str) l_len = strlen(l_str); } break; case AGEDGE: isEdge = 1; g_str = agnameof(agroot(agraphof(agtail(((edge_t *)obj))))); - g_len = strlen(g_str); t_str = agnameof(agtail(((edge_t *)obj))); - t_len = strlen(t_str); pt = ED_tail_port((edge_t *)obj); if ((tp_str = pt.name)) - tp_len = strlen(tp_str); + has_tp = TRUE; h_str = agnameof(aghead(((edge_t *)obj))); - h_len = strlen(h_str); pt = ED_head_port((edge_t *)obj); if ((hp_str = pt.name)) - hp_len = strlen(hp_str); - h_len = strlen(h_str); + has_hp = TRUE; tl = ED_label((edge_t *)obj); if (tl) { l_str = tl->text; - if (str) l_len = strlen(l_str); } if (agisdirected(agroot(agraphof(agtail(((edge_t*)obj)))))) e_str = "->"; else e_str = "--"; - e_len = t_len + (tp_len?tp_len+1:0) + 2 + h_len + (hp_len?hp_len+1:0); break; } - /* two passes over str. - * - * first pass prepares substitution strings and computes - * total length for newstring required from malloc. - */ - for (s = str; (c = *s++);) { - if (c == '\\' && *s != '\0') { - switch (c = *s++) { - case 'G': - newlen += g_len; - break; - case 'N': - newlen += n_len; - break; - case 'E': - if (isEdge) { - newlen += t_len; - if (tp_len) { - newlen++; - newlen += tp_len; - } - newlen += e_len; - newlen += h_len;; - if (hp_len) { - newlen++; - newlen += hp_len; - } - } - break; - case 'H': - newlen += h_len; - break; - case 'T': - newlen += t_len; - break; - case 'L': - newlen += l_len; - break; - case '\\': - if (escBackslash) { - newlen += 1; - break; - } - /* Fall through */ - default: /* leave other escape sequences unmodified, e.g. \n \l \r */ - newlen += 2; - } - } else { - newlen++; - } - } - /* allocate new string */ - newstr = gmalloc(newlen + 1); + /* allocate a dynamic buffer that we will use to construct the result */ + agxbinit(&buf, 0, NULL); - /* second pass over str assembles new string */ - for (s = str, p = newstr; (c = *s++);) { + /* assemble new string */ + for (s = str; (c = *s++);) { if (c == '\\' && *s != '\0') { switch (c = *s++) { case 'G': - for (t = g_str; (*p = *t++); p++); + agxbput(&buf, g_str); break; case 'N': - for (t = n_str; (*p = *t++); p++); + agxbput(&buf, n_str); break; case 'E': if (isEdge) { - for (t = t_str; (*p = *t++); p++); - if (tp_len) { - *p++ = ':'; - for (t = tp_str; (*p = *t++); p++); + agxbput(&buf, t_str); + if (has_tp) { + agxbputc(&buf, ':'); + agxbput(&buf, tp_str); } - for (t = e_str; (*p = *t++); p++); - for (t = h_str; (*p = *t++); p++); - if (hp_len) { - *p++ = ':'; - for (t = hp_str; (*p = *t++); p++); + agxbput(&buf, e_str); + agxbput(&buf, h_str); + if (has_hp) { + agxbputc(&buf, ':'); + agxbput(&buf, hp_str); } } break; case 'T': - for (t = t_str; (*p = *t++); p++); + agxbput(&buf, t_str); break; case 'H': - for (t = h_str; (*p = *t++); p++); + agxbput(&buf, h_str); break; case 'L': - for (t = l_str; (*p = *t++); p++); + agxbput(&buf, l_str); break; case '\\': if (escBackslash) { - *p++ = '\\'; + agxbputc(&buf, '\\'); break; } /* Fall through */ default: /* leave other escape sequences unmodified, e.g. \n \l \r */ - *p++ = '\\'; - *p++ = c; + agxbputc(&buf, '\\'); + agxbputc(&buf, c); break; } } else { - *p++ = c; + agxbputc(&buf, c); } } - *p++ = '\0'; + + /* extract the final string with replacements applied */ + newstr = agxbdisown(&buf); + agxbfree(&buf); + return newstr; } -- GitLab