From: Omar Polo Subject: Re: privsep.c memory leak To: Kyle Ackerman Cc: gameoftrees@openbsd.org Date: Sat, 18 Nov 2023 09:15:58 +0100 On 2023/11/18 00:48:43 -0600, Kyle Ackerman wrote: > Here is a diff for lib/privsep.c that fixes a memory leak spotted by > Omar. Any suggestions? Seems correct to me, thank you! Two nitpics: - we use tabs (8 columns) instead of spaces for indentations > [...] > datalen = imsg.hdr.len - IMSG_HEADER_SIZE; > if (imsg.hdr.type == GOT_IMSG_COMMIT_PAINTING_DONE) > break; - here it leaks too. I've added an imsg_free() call here too before committing. Reviewing your diff I've also spotted another small issue: 3540 for (i = 0; i < icommits.ncommits; i++) { .... 3545 if (icommits.present_in_pack) { .... 3548 err = cb(cb_arg, &id, icommit.color); 3549 if (err) 3550 break; err is set here 3551 } else { 3552 struct got_object_qid *qid; 3553 err = got_object_qid_alloc_partial(&qid); 3554 if (err) 3555 break; and here too .... 3562 3563 imsg_free(&imsg); 3564 } but then is ignored since we continue looping. diff /home/op/w/got commit - 6d0030ba1d9985fc8b8a9a2dc0f0c412c2c678da path + /home/op/w/got blob - 2109bc13bcb9c7dbfb07021a640c5fa125e74954 file + lib/privsep.c --- lib/privsep.c +++ lib/privsep.c @@ -3518,7 +3518,7 @@ got_privsep_recv_painted_commits(struct got_object_id_ datalen = imsg.hdr.len - IMSG_HEADER_SIZE; if (imsg.hdr.type == GOT_IMSG_COMMIT_PAINTING_DONE) { imsg_free(&imsg); - break; + return NULL; } if (imsg.hdr.type != GOT_IMSG_PAINTED_COMMITS){ imsg_free(&imsg); @@ -3561,9 +3561,9 @@ got_privsep_recv_painted_commits(struct got_object_id_ } imsg_free(&imsg); + if (err) + return err; } - - return err; } const struct got_error *