From: Jack Burton <jack@saosce.com.au>
Subject: [patch 1 of 3] iscsid(8): fix memory leak
To: tech@openbsd.org
Date: Tue, 31 Dec 2024 18:30:02 +1030

Fix memory leaks in some members of struct kvp.

It looks like this is what claudio@ originally intended the otherwise
unused flags member (which has been in the tree since iscsid was first
added 14 years ago) to be used for.

This patch will make very little difference as things stand (as in a
typical run the leaks are tiny), but this approach is necessary for
the third patch in my set, which makes much heavier use of struct kvp
members allocated on the heap.

Almost all of this patch is also included in my third patch (all
bar the bits the third patch makes obsolete), but I'm posting this
one on its own first as it has a different purpose (this one's a
bug fix; whereas patch 3 introduces new functionality).

cvs server: Diffing .
Index: connection.c
===================================================================
RCS file: /cvs/src/usr.sbin/iscsid/connection.c,v
diff -u -p -r1.21 connection.c
--- connection.c	5 Dec 2015 06:38:18 -0000	1.21
+++ connection.c	30 Dec 2024 05:56:32 -0000
@@ -327,11 +327,13 @@ conn_gen_kvp(struct connection *c, struc
 			kvp[i].key = strdup("MaxConnections");
 			if (kvp[i].key == NULL)
 				return -1;
+			kvp[i].flags |= KVP_KEY_ALLOCED;
 			if (asprintf(&kvp[i].value, "%hu",
 			    s->mine.MaxConnections) == -1) {
 				kvp[i].value = NULL;
 				return -1;
 			}
+			kvp[i].flags |= KVP_VALUE_ALLOCED;
 		}
 		i++;
 	}
@@ -341,11 +343,13 @@ conn_gen_kvp(struct connection *c, struc
 			kvp[i].key = strdup("MaxRecvDataSegmentLength");
 			if (kvp[i].key == NULL)
 				return -1;
+			kvp[i].flags |= KVP_KEY_ALLOCED;
 			if (asprintf(&kvp[i].value, "%u",
 			    c->mine.MaxRecvDataSegmentLength) == -1) {
 				kvp[i].value = NULL;
 				return -1;
 			}
+			kvp[i].flags |= KVP_VALUE_ALLOCED;
 		}
 		i++;
 	}
Index: initiator.c
===================================================================
RCS file: /cvs/src/usr.sbin/iscsid/initiator.c,v
diff -u -p -r1.15 initiator.c
--- initiator.c	16 Jan 2015 15:57:06 -0000	1.15
+++ initiator.c	30 Dec 2024 05:56:32 -0000
@@ -280,7 +280,7 @@ initiator_login_kvp(struct connection *c
 		if (!(kvp = calloc(nkvp, sizeof(*kvp))))
 			return NULL;
 		if (conn_gen_kvp(c, kvp, &nkvp) == -1) {
-			free(kvp);
+			kvp_free(kvp);
 			return NULL;
 		}
 		break;
@@ -327,10 +327,10 @@ initiator_login_build(struct connection 
 		return NULL;
 	}
 	if ((n = text_to_pdu(kvp, p)) == -1) {
-		free(kvp);
+		kvp_free(kvp);
 		return NULL;
 	}
-	free(kvp);
+	kvp_free(kvp);
 
 	if (n > 8192) {
 		log_warn("initiator_login_build: help, I'm too verbose");
@@ -409,11 +409,11 @@ initiator_login_cb(struct connection *c,
 		}
 
 		if (conn_parse_kvp(c, kvp) == -1) {
-			free(kvp);
+			kvp_free(kvp);
 			conn_fail(c);
 			goto done;
 		}
-		free(kvp);
+		kvp_free(kvp);
 	}
 
 	/* advance FSM if possible */
@@ -480,7 +480,7 @@ initiator_discovery_cb(struct connection
 		for (k = kvp; k->key; k++) {
 			log_debug("%s\t=>\t%s", k->key, k->value);
 		}
-		free(kvp);
+		kvp_free(kvp);
 		session_shutdown(c->session);
 		break;
 	default:
Index: iscsid.h
===================================================================
RCS file: /cvs/src/usr.sbin/iscsid/iscsid.h,v
diff -u -p -r1.19 iscsid.h
--- iscsid.h	21 May 2024 05:00:48 -0000	1.19
+++ iscsid.h	30 Dec 2024 05:56:33 -0000
@@ -385,6 +385,7 @@ void	*pdu_getbuf(struct pdu *, size_t *,
 void	pdu_free(struct pdu *);
 int	socket_setblockmode(int, int);
 const char *log_sockaddr(void *);
+void	kvp_free(struct kvp *);
 
 void	task_init(struct task *, struct session *, int, void *,
 	    void (*)(struct connection *, void *, struct pdu *),
Index: util.c
===================================================================
RCS file: /cvs/src/usr.sbin/iscsid/util.c,v
diff -u -p -r1.9 util.c
--- util.c	12 Apr 2021 10:03:33 -0000	1.9
+++ util.c	30 Dec 2024 05:56:33 -0000
@@ -189,3 +189,19 @@ fail:
 	pdu_free(pdu);
 	return -1;
 }
+
+void
+kvp_free(struct kvp *kvp)
+{
+	struct kvp *k;
+
+	if (kvp == NULL)
+		return;
+	for (k = kvp; k->key; k++) {
+		if (k->flags & KVP_KEY_ALLOCED)
+			free(k->key);
+		if (k->flags & KVP_VALUE_ALLOCED)
+			free(k->value);
+	}
+	free(kvp);
+}