From: Matthew Fernandez <matthew.fernandez@gmail.com>
Date: Sun, 10 Jul 2022 18:03:02 +0000 (-0700)
Subject: smyrna load_attributes: use a string view for 'ss'
X-Git-Tag: 5.0.1~43^2~4
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=83635cfc27ca95bbb79d43660c5447dfea3b2453;p=graphviz

smyrna load_attributes: use a string view for 'ss'

This code contained multiple memory leaks and unchecked allocations:¹

  1. `pch` was `strdup`-ed into `ss` on line 58. But `strdup`-ed again when
     being saved to an `attr` field. This lost the memory originating from the
     first `strdup`.

  2. Cases 0, 3, and 4 of the switch do not save the full contents of `ss` at
     all. This means naively removing the `strdup` calls in cases 1, 2, and
     default would not have solved the memory leak in (1) because cases 0, 3,
     and 4 would still leak memory.

  3. None of the `strdup` calls in this function were checked for failure.

This commit attempts to solve all the above. We now take a read-only reference
to the string data on line 58 and only `strdup` it when needed.

¹ It also assumes all lines of the input file are fewer characters than
  `BUFSIZ`, a platform-dependent constant. I do not know why this would be
  guaranteed. However, this problem seems orthogonal to the above.
---

diff --git a/cmd/smyrna/gui/gui.c b/cmd/smyrna/gui/gui.c
index a082672c7..c82863f9f 100644
--- a/cmd/smyrna/gui/gui.c
+++ b/cmd/smyrna/gui/gui.c
@@ -15,6 +15,7 @@
 #include <gdk/gdkkeysyms.h>
 #include <gdk/gdk.h>
 #include "viewport.h"
+#include <cgraph/strview.h>
 #include <common/memory.h>
 
 GladeXML *xml;			//global libglade vars
@@ -42,7 +43,6 @@ void load_attributes(void)
 {
     FILE *file;
     char line[BUFSIZ];
-    char *ss;
     char *pch;
     static char *smyrna_attrs;
 
@@ -55,22 +55,22 @@ void load_attributes(void)
 	for (int attrcount = 0; fgets(line, sizeof line, file) != NULL; ++attrcount) {
 	    pch = strtok(line, ",");
 	    for (int ind = 0; pch != NULL; ++ind) {
-		ss = strdup(pch);
+		strview_t ss = strview(pch, '\0');
 		pch = strtok(NULL, ",");
 		switch (ind) {
 		case 0:
-		    attr[attrcount].Type = ss[0];
+		    attr[attrcount].Type = ss.size > 0 ? ss.data[0] : '\0';
 		    break;
 		case 1:
-		    attr[attrcount].Name = strdup(ss);
+		    attr[attrcount].Name = strview_str(ss);
 		    break;
 		case 2:
-		    attr[attrcount].Default = strdup(ss);
+		    attr[attrcount].Default = strview_str(ss);
 		    break;
 		case 3:
 		    break;
 		case 4:
-		    if (strstr(ss, "ALL_ENGINES")) {
+		    if (strview_str_contains(ss, "ALL_ENGINES")) {
 			attr[attrcount].Engine[GVK_DOT] = 1;
 			attr[attrcount].Engine[GVK_NEATO] = 1;
 			attr[attrcount].Engine[GVK_TWOPI] = 1;
@@ -78,15 +78,15 @@ void load_attributes(void)
 			attr[attrcount].Engine[GVK_FDP] = 1;
 		    } else {
 			attr[attrcount].Engine[GVK_DOT] =
-			    strstr(ss, "DOT") ? 1 : 0;
+			    strview_str_contains(ss, "DOT") ? 1 : 0;
 			attr[attrcount].Engine[GVK_NEATO] =
-			    strstr(ss, "NEATO") ? 1 : 0;
+			    strview_str_contains(ss, "NEATO") ? 1 : 0;
 			attr[attrcount].Engine[GVK_TWOPI] =
-			    strstr(ss, "TWOPI") ? 1 : 0;
+			    strview_str_contains(ss, "TWOPI") ? 1 : 0;
 			attr[attrcount].Engine[GVK_CIRCO] =
-			    strstr(ss, "CIRCO") ? 1 : 0;
+			    strview_str_contains(ss, "CIRCO") ? 1 : 0;
 			attr[attrcount].Engine[GVK_FDP] =
-			    strstr(ss, "FDP") ? 1 : 0;
+			    strview_str_contains(ss, "FDP") ? 1 : 0;
 		    }
 		    break;
 		default:
@@ -94,8 +94,7 @@ void load_attributes(void)
 			RALLOC(attr[attrcount].ComboValuesCount,
 			       attr[attrcount].ComboValues, char *);
 		    attr[attrcount].ComboValues[attr[attrcount].
-						ComboValuesCount] =
-			strdup(ss);
+						ComboValuesCount] = strview_str(ss);
 		    attr[attrcount].ComboValuesCount++;
 		    break;
 		}