From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: [patch 1 of 3] iscsid(8): fix memory leak
To: Jack Burton <jack@saosce.com.au>
Cc: tech@openbsd.org
Date: Wed, 15 Jan 2025 14:07:56 +0100

On Tue, Dec 31, 2024 at 06:30:02PM +1030, Jack Burton wrote:
> 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");

I think here it would make sense to kill the strdup and the
KVP_KEY_ALLOCED. Since the key can be a static string.

>  			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");

Same here.

>  			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);
> +}
> 

I wonder if pdu_to_text() should actually alloc the kvp key and value
instead of fiddling with the pdu buffer internals.

I will commit this diff with the above mentioned changes tomorrow.
-- 
:wq Claudio