From: Omar Polo <op@omarpolo.com>
Subject: Re: leak in got_fetch_pack
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Sun, 13 Feb 2022 22:49:05 +0100


Stefan Sperling <stsp@stsp.name> writes:

> On Sun, Feb 13, 2022 at 09:33:38PM +0100, Omar Polo wrote:
>> Hello,
>> 
>> while studying a bit the code that wraps the various libexec helpers, I
>> stumbled across this.  fetchibuf is local to the function and it doesn't
>> seem to be used afterwards, same for imsg_fetchfds.  regress with the
>> following diff passes, and I've also fetched from a couple of
>> repositories successfully.
>> 
>> (the imsg_clear call is possibly useless if there aren't any pending
>> messages enqueued, but I thought that it reads better; I can drop it)
>> 
>> 
>> diff 32b5305fbed2d1a4c55b0eef6b93349b93ba7db0 /home/op/w/got-fetch-leak
>> blob - 6c0dbf4a0ba8148e5f95f882f51bda4b71ebc4f1
>> file + lib/fetch.c
>> --- lib/fetch.c
>> +++ lib/fetch.c
>> @@ -353,6 +353,11 @@ got_fetch_pack(struct got_object_id **pack_hash, struc
>>  			packfile_size = packfile_size_cur;
>>  		}
>>  	}
>> +	imsg_clear(&fetchibuf);
>
> Should the imsg not be cleared inside the above "while" loop body?
> Just like it is done idxibuf on line 475 (line offset without your patch)?
>
> Otherwise there is still a leak, isn't there?

Sure, but I fail to see how it's leaking if imsg_clear is called outside
of the loop.

If I'm reading correctly imsg{,-buf}.c all it does is to dequeue pending
ibufs and file descriptors and frees/closes them, so while I agree that
doing it inside the loop (possibly) frees these imsgbuf internal queues
more quickly, I don't see how it avoids leaking memory.

I'm clearly missing something obvious here, sorry, I just don't see it.

diff 32b5305fbed2d1a4c55b0eef6b93349b93ba7db0 /home/op/w/got-fetch-leak
blob - 6c0dbf4a0ba8148e5f95f882f51bda4b71ebc4f1
file + lib/fetch.c
--- lib/fetch.c
+++ lib/fetch.c
@@ -352,7 +352,12 @@ got_fetch_pack(struct got_object_id **pack_hash, struc
 				break;
 			packfile_size = packfile_size_cur;
 		}
+		imsg_clear(&fetchibuf);
 	}
+	if (close(imsg_fetchfds[0]) == -1) {
+		err = got_error_from_errno("close");
+		goto done;
+	}
 	if (waitpid(fetchpid, &fetchstatus, 0) == -1) {
 		err = got_error_from_errno("waitpid");
 		goto done;