From: Kyle Ackerman Subject: Re: got_blame memory leak To: Stefan Sperling Cc: gameoftrees@openbsd.org Date: Mon, 25 Mar 2024 15:30:24 -0500 Stefan Sperling writes: > On Sun, Mar 24, 2024 at 06:48:05PM -0500, Kyle Ackerman wrote: >> Here is alternative diff. Rather than checking after every message >> received, we can just allocate the got_object_id once we receive a >> GOT_IMSG_COMMIT_TRAVERSAL_DONE. This is here in case checking to see if >> changed_commit_id is not null after each imsg is ugly.` >> >> diff /home/kyle/src/got >> commit - 39910b637a9a53cc48b0c63766da691dec0af593 >> path + /home/kyle/src/got >> blob - 857c09bbb3a5d9425176ed93727a912b2242aac6 >> file + lib/privsep.c >> --- lib/privsep.c >> +++ lib/privsep.c >> @@ -2730,8 +2730,7 @@ got_privsep_recv_traversed_commits(struct got_commit_o >> >> /* The last commit may contain a change. */ >> if (i == icommits->ncommits - 1) { >> - *changed_commit_id = >> - got_object_id_dup(&qid->id); >> + *changed_commit_id = &qid->id; >> if (*changed_commit_id == NULL) { > > The pointer can no longer be NULL here. > >> err = got_error_from_errno( >> "got_object_id_dup"); >> @@ -2749,6 +2748,7 @@ got_privsep_recv_traversed_commits(struct got_commit_o >> datalen, ibuf); >> break; >> case GOT_IMSG_COMMIT_TRAVERSAL_DONE: >> + *changed_commit_id = got_object_id_dup(*changed_commit_id); > > And the above allocation failure check would now need to be here. > > Howveer, this whole mechanism is a bit silly. The caller receives the > full list of traversed commits, and *changed_commit points at a newly > allocated copy of the last commit on this list. > > Couldn't the caller use STAILQ_LAST to find the same commit, so we > can get rid of the unneccesary copy and **changed_commit_id in this > function entirely? > >> done = 1; >> break; >> default: >> >> Here is a diff with the rewritten logic. diff /home/kyle/src/got commit - 39910b637a9a53cc48b0c63766da691dec0af593 path + /home/kyle/src/got blob - d517f36f2ccbda4c48348016a20e6d6e87fbd1a4 file + lib/got_lib_privsep.h --- lib/got_lib_privsep.h +++ lib/got_lib_privsep.h @@ -787,8 +787,8 @@ const struct got_error *got_privsep_recv_gotconfig_rem const struct got_error *got_privsep_send_commit_traversal_request( struct imsgbuf *, struct got_object_id *, int, const char *); const struct got_error *got_privsep_recv_traversed_commits( - struct got_commit_object **, struct got_object_id **, - struct got_object_id_queue *, struct imsgbuf *); + struct got_commit_object **, struct got_object_id_queue *, + struct imsgbuf *); const struct got_error *got_privsep_send_enumerated_tree(size_t *, struct imsgbuf *, struct got_object_id *, const char *, struct got_parsed_tree_entry *, int); blob - c956fa4a7551d6ffe10a3bd5888209dde0a148b8 file + lib/object_open_privsep.c --- lib/object_open_privsep.c +++ lib/object_open_privsep.c @@ -24,6 +24,7 @@ #include #include +#include #include #include #include @@ -1214,7 +1215,7 @@ got_traverse_packed_commits(struct got_object_id_queue struct got_packidx *packidx = NULL; char *path_packfile = NULL; struct got_commit_object *changed_commit = NULL; - struct got_object_id *changed_commit_id = NULL; + struct got_object_qid *changed_commit_qid = NULL; int idx; err = got_repo_search_packidx(&packidx, &idx, repo, commit_id); @@ -1248,7 +1249,7 @@ got_traverse_packed_commits(struct got_object_id_queue goto done; err = got_privsep_recv_traversed_commits(&changed_commit, - &changed_commit_id, traversed_commits, pack->privsep_child->ibuf); + traversed_commits, pack->privsep_child->ibuf); if (err) goto done; @@ -1258,13 +1259,13 @@ got_traverse_packed_commits(struct got_object_id_queue * This commit might be opened again soon. */ changed_commit->refcnt++; - err = got_repo_cache_commit(repo, changed_commit_id, + changed_commit_qid = STAILQ_LAST(traversed_commits, got_object_qid, entry); + err = got_repo_cache_commit(repo, &changed_commit_qid->id, changed_commit); got_object_commit_close(changed_commit); } done: free(path_packfile); - free(changed_commit_id); return err; } blob - 857c09bbb3a5d9425176ed93727a912b2242aac6 file + lib/privsep.c --- lib/privsep.c +++ lib/privsep.c @@ -2691,7 +2691,6 @@ got_privsep_send_commit_traversal_request(struct imsgb const struct got_error * got_privsep_recv_traversed_commits(struct got_commit_object **changed_commit, - struct got_object_id **changed_commit_id, struct got_object_id_queue *commit_ids, struct imsgbuf *ibuf) { const struct got_error *err = NULL; @@ -2702,7 +2701,6 @@ got_privsep_recv_traversed_commits(struct got_commit_o int i, done = 0; *changed_commit = NULL; - *changed_commit_id = NULL; while (!done) { err = got_privsep_recv_imsg(&imsg, ibuf, 0); @@ -2728,23 +2726,9 @@ got_privsep_recv_traversed_commits(struct got_commit_o memcpy(&qid->id, &ids[i], sizeof(ids[i])); STAILQ_INSERT_TAIL(commit_ids, qid, entry); - /* The last commit may contain a change. */ - if (i == icommits->ncommits - 1) { - *changed_commit_id = - got_object_id_dup(&qid->id); - if (*changed_commit_id == NULL) { - err = got_error_from_errno( - "got_object_id_dup"); - break; - } - } } break; case GOT_IMSG_COMMIT: - if (*changed_commit_id == NULL) { - err = got_error(GOT_ERR_PRIVSEP_MSG); - break; - } err = get_commit_from_imsg(changed_commit, &imsg, datalen, ibuf); break; Thanks for your guidance, Kyle