From: Claudio Jeker Subject: Re: [patch 1 of 3] iscsid(8): fix memory leak To: Jack Burton Cc: tech@openbsd.org Date: Thu, 16 Jan 2025 21:24:58 +0100 On Thu, Jan 16, 2025 at 01:54:20PM +1030, Jack Burton wrote: > On Wed, 15 Jan 2025 14:07:56 +0100 > Claudio Jeker 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. Diff is in. I guess we could really always allocate value and always use a const char for the key (I see no reason to support or enforce allocation there). -- :wq Claudio