From: Jack Burton <jack@saosce.com.au>
Subject: Re: [patch 1 of 3] iscsid(8): fix memory leak
To: Claudio Jeker <cjeker@diehard.n-r-g.com>
Cc: tech@openbsd.org
Date: Thu, 16 Jan 2025 13:54:20 +1030

On Wed, 15 Jan 2025 14:07:56 +0100
Claudio Jeker <cjeker@diehard.n-r-g.com> wrote:
> On Tue, Dec 31, 2024 at 06:30:02PM +1030, Jack Burton wrote:
> > Fix memory leaks in some members of struct kvp.

...

> > 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.

Yes absolutely; and same for the other example you pointed out too.

My patch #3 replaces both of those with a more generic approach anyway.

...

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

Perhaps.  If we adopt a convention that the kvp key & value are always
alloced individually, then we can dispense with the alloced-or-not
distinction (and the kvp flags) altogether.  That might help to
simplify things in a few places.  The cost is negligible, since we only
use kvps for text/login pdus (so it won't affect performance of scsi
command/data pdus).


> I will commit this diff with the above mentioned changes tomorrow.

Thanks; much appreciated.