From: Matthew Fernandez <matthew.fernandez@gmail.com>
Date: Thu, 11 Aug 2022 02:14:08 +0000 (-0700)
Subject: cgraph agxbdisown: exit on allocation failure instead of returning NULL
X-Git-Tag: 5.0.1~12^2~2
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=369dfec375fdc2f66f4c84ec37a31ca7536e1fae;p=graphviz

cgraph agxbdisown: exit on allocation failure instead of returning NULL

82b7d2021a7faa565065804df8b730a3213a8356 abbreviated some xdot code into a call
to `agxbdisown`. But it failed to account for the fact that `agxbdisown` could
return `NULL` on `strdup` failure, leaving still-allocated memory in the agxbuf.
Thus this commit introduced a new code path that leaked memory. I.e. the removed
call to `agxbfree` should have been retained.

So the obvious fix would be to re-add the `agxbfree` call, right? Well closer
inspection of agxbuf reveals another incongruity. agxbuf takes design decisions
based on the idea that the caller either cannot handle failure or does not want
to handle failure. It calls the alloc.h wrappers that exit on out-of-memory,
alleviating the caller from having to think about such things. Except for one
case: calling `strdup` in `agxbdisown`. Here it assumes the caller _does_ want
to handle failure. Except actually not, because it previously calls `agxbputc`
that conditionally calls `agxbmore` that exits on allocation failure. This is a
bit convoluted, but putting this all together we can see that the caller of
`agxbdisown` actually _cannot_ robustly recover from failure because, even if
they try to, `agxbmore` can still exit before they have a chance.

The way out of this quagmire is to adhere to the original principle more
strongly in `agxbdisown`. We assume, here too, that the caller does not want to
or cannot handle failure. `agxbdisown` now exits on all allocation failure
paths.

This has two coincidental beneficial side effects:

  1. In one case, we can leverage `gv_strndup` to avoid writing a trailing
     `'\0'` that is only for the purposes of immediately copying it somewhere
     else.

  2. Any caller of `agxbdisown` no longer needs to call `agxbfree` afterwards.
     In an analogy to C++, `agxbdisown` is effectively a move operator and
     destructor in one.
---

diff --git a/lib/cgraph/agxbuf.h b/lib/cgraph/agxbuf.h
index b3d391bd4..1d8d7b60d 100644
--- a/lib/cgraph/agxbuf.h
+++ b/lib/cgraph/agxbuf.h
@@ -218,28 +218,24 @@ static inline char *agxbnext(agxbuf *xb) { return xb->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.
+ * this will first copy the data to a new heap buffer to then return. 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.
  */
 static inline char *agxbdisown(agxbuf *xb) {
   char *buf;
 
-  // terminate the existing string
-  agxbputc(xb, '\0');
-
   if (xb->stack_allocated) {
     // the buffer is not dynamically allocated, so we need to copy its contents
     // to heap memory
 
-    buf = strdup(xb->buf);
-    if (buf == NULL) {
-      return NULL;
-    }
+    buf = gv_strndup(xb->buf, agxblen(xb));
 
   } else {
-    // the buffer is already dynamically allocated, so take it as-is
+    // the buffer is already dynamically allocated, so terminate it and then
+    // take it as-is
+    agxbputc(xb, '\0');
     buf = xb->buf;
   }