From: Mark Jamsek Subject: plug a few small leaks in tog and repository.c:match_packed_object() To: gameoftrees@openbsd.org Date: Sun, 10 Dec 2023 02:00:45 +1100 Combining them all inline as they are very small changes in just two files: tog.c and repository.c (not including the last whitespace only change in pack.c, spotted by chance while hunting down the match_packed_object() leak). Apart from log messages, I'll briefly elaborate on a couple as the diffs themselves don't show the whole picture so it may save you having to explore the code further, and I'm not certain on the original intent behind the code producing the matched_ids leak, which we could fix a couple ways (regress is happy with either). For this reason, I've opted for the fix that maintains the current behaviour, but I want to draw your attention to it just in case the status quo is not intended. The id1 and id2 leak in the diff view (3rd commit) is because they are first allocated in cmd_diff, but then we dup the ids immediately to s->id1 and s->id2 in open_diff_view()--and only these copies are freed in close_diff_view(). So we need to free the original allocations in cmd_diff(). The matched_ids leak (5th commit) in repository.c:match_packed_object() is because we initialise the potentially populated matched_ids list upon entering pack.c:got_packidx_match_id_str_prefix(). We could zap that STAILQ_INIT(matched_ids) call instead (final XX-prepended diff) but then we would repeatedly loop over the same matched ids with each index file as we'd be appending unique ids to the list from each index file, which we are not presently doing because of that STAILQ_INIT(matched_ids) call (although I'm not sure if that was the original intent?). So rather than only free the list when the pack directory modification time changes, free it after each index file iteration instead. The log message of the remaining few commits should explain the fixes as they're simple and self-explanatory but if not, please let me know! ----------------------------------------------- commit b6fe41fdfb6219024a9afaea3da2065274a582ac from: Mark Jamsek date: Sat Dec 9 10:07:53 2023 UTC tog: plug colors memleak in log view M tog/tog.c | 3+ 6- 1 file changed, 3 insertions(+), 6 deletions(-) diff 8fd0d1965f6393cf1ae1776daee76543055e0dd8 b6fe41fdfb6219024a9afaea3da2065274a582ac commit - 8fd0d1965f6393cf1ae1776daee76543055e0dd8 commit + b6fe41fdfb6219024a9afaea3da2065274a582ac blob - ce92dab6cd549c3c215b39c9561173732788a67c blob + 42cbf71abb6e8b550dfb0a2331f340ca01f869de --- tog/tog.c +++ tog/tog.c @@ -3535,6 +3535,7 @@ close_log_view(struct tog_view *view) free_commits(&s->limit_commits); free_commits(&s->real_commits); + free_colors(&s->colors); free(s->in_repo_path); s->in_repo_path = NULL; free(s->start_id); @@ -3845,16 +3846,12 @@ open_log_view(struct tog_view *view, struct got_object goto done; err = add_color(&s->colors, "^$", TOG_COLOR_AUTHOR, get_color_value("TOG_COLOR_AUTHOR")); - if (err) { - free_colors(&s->colors); + if (err) goto done; - } err = add_color(&s->colors, "^$", TOG_COLOR_DATE, get_color_value("TOG_COLOR_DATE")); - if (err) { - free_colors(&s->colors); + if (err) goto done; - } } view->show = show_log_view; ----------------------------------------------- commit 3c33690882bfa409292b75db9719c655b9402515 from: Mark Jamsek date: Sat Dec 9 10:10:31 2023 UTC tog: plug commit object leak in 'tog tree' M tog/tog.c | 2+ 0- 1 file changed, 2 insertions(+), 0 deletions(-) diff b6fe41fdfb6219024a9afaea3da2065274a582ac 3c33690882bfa409292b75db9719c655b9402515 commit - b6fe41fdfb6219024a9afaea3da2065274a582ac commit + 3c33690882bfa409292b75db9719c655b9402515 blob - 42cbf71abb6e8b550dfb0a2331f340ca01f869de blob + 1c4cd78b65b4cbc3e25d26ec8ee9aa66ec424bcd --- tog/tog.c +++ tog/tog.c @@ -8355,6 +8355,8 @@ done: free(cwd); free(commit_id); free(label); + if (commit != NULL) + got_object_commit_close(commit); if (ref) got_ref_close(ref); if (worktree != NULL) ----------------------------------------------- commit 5dd5fa1db32f0c250d6ad41baeebdcb4b2a5795d from: Mark Jamsek date: Sat Dec 9 10:15:03 2023 UTC tog: plug object id leak in diff view M tog/tog.c | 2+ 2- 1 file changed, 2 insertions(+), 2 deletions(-) diff 3c33690882bfa409292b75db9719c655b9402515 5dd5fa1db32f0c250d6ad41baeebdcb4b2a5795d commit - 3c33690882bfa409292b75db9719c655b9402515 commit + 5dd5fa1db32f0c250d6ad41baeebdcb4b2a5795d blob - 1c4cd78b65b4cbc3e25d26ec8ee9aa66ec424bcd blob + e2326392e5c40d180f4f231973f5e38f672ac161 --- tog/tog.c +++ tog/tog.c @@ -5606,8 +5606,6 @@ open_diff_view(struct tog_view *view, struct got_objec s->last_displayed_line = view->nlines; s->selected_line = 1; s->repo = repo; - s->id1 = id1; - s->id2 = id2; s->label1 = label1; s->label2 = label2; @@ -6196,6 +6194,8 @@ done: free(keyword_idstr2); free(label1); free(label2); + free(id1); + free(id2); free(repo_path); free(cwd); if (repo) { ----------------------------------------------- commit 69e758a9c1e5024d68eac9094dc7691838af58db from: Mark Jamsek date: Sat Dec 9 11:42:11 2023 UTC plug leak of commit object in 'tog diff' error path Reduce the scope of the commit object too. M tog/tog.c | 9+ 7- 1 file changed, 9 insertions(+), 7 deletions(-) diff 5dd5fa1db32f0c250d6ad41baeebdcb4b2a5795d 69e758a9c1e5024d68eac9094dc7691838af58db commit - 5dd5fa1db32f0c250d6ad41baeebdcb4b2a5795d commit + 69e758a9c1e5024d68eac9094dc7691838af58db blob - e2326392e5c40d180f4f231973f5e38f672ac161 blob + cde9c404b1d17c48cf1f941386d251fc795137d2 --- tog/tog.c +++ tog/tog.c @@ -5357,7 +5357,6 @@ create_diff(struct tog_diff_view_state *s) case GOT_OBJ_TYPE_COMMIT: { const struct got_object_id_queue *parent_ids; struct got_object_qid *pid; - struct got_commit_object *commit2; struct got_reflist_head *refs; size_t nlines = 0; struct got_diffstat_cb_arg dsa = { @@ -5382,9 +5381,6 @@ create_diff(struct tog_diff_view_state *s) if (err) break; - err = got_object_open_as_commit(&commit2, s->repo, s->id2); - if (err) - goto done; refs = got_reflist_object_id_map_lookup(tog_refs_idmap, s->id2); /* Show commit info if we're diffing to a parent/root commit. */ if (s->id1 == NULL) { @@ -5394,6 +5390,12 @@ create_diff(struct tog_diff_view_state *s) if (err) goto done; } else { + struct got_commit_object *commit2; + + err = got_object_open_as_commit(&commit2, s->repo, + s->id2); + if (err) + goto done; parent_ids = got_object_commit_get_parent_ids(commit2); STAILQ_FOREACH(pid, parent_ids, entry) { if (got_object_id_cmp(s->id1, &pid->id) == 0) { @@ -5401,13 +5403,13 @@ create_diff(struct tog_diff_view_state *s) &s->nlines, s->id2, refs, s->repo, s->ignore_whitespace, s->force_text_diff, &dsa, s->f); - if (err) - goto done; break; } } + got_object_commit_close(commit2); + if (err) + goto done; } - got_object_commit_close(commit2); err = cat_diff(s->f, tmp_diff_file, &s->lines, &s->nlines, lines, nlines); ----------------------------------------------- commit 557f71d8d65ad007ecbe02338778f4911e49daa2 from: Mark Jamsek date: Sat Dec 9 11:48:32 2023 UTC plug object id queue leak when iterating pack index files We need to free the matched object id queue on each pack index iteration--not only when the pack file modification time has changed--otherwise the ids are leaked when we reinitialise the queue in got_packidx_match_id_str_prefix(). M lib/repository.c | 2+ 1- 1 file changed, 2 insertions(+), 1 deletion(-) diff 69e758a9c1e5024d68eac9094dc7691838af58db 557f71d8d65ad007ecbe02338778f4911e49daa2 commit - 69e758a9c1e5024d68eac9094dc7691838af58db commit + 557f71d8d65ad007ecbe02338778f4911e49daa2 blob - a81092fd0836c5d97b58ad7ed9f24312f5c27691 blob + e66c6a1a0136362f4a6319872bc9f45cf2686704 --- lib/repository.c +++ lib/repository.c @@ -1809,7 +1809,6 @@ retry: "modifications"); goto done; } - got_object_id_queue_free(&matched_ids); goto retry; } @@ -1854,6 +1853,8 @@ retry: goto done; } } + if (TAILQ_NEXT(pe, entry) != NULL) + got_object_id_queue_free(&matched_ids); } done: got_object_id_queue_free(&matched_ids); ----------------------------------------------- commit 828e5074bf59310d99fdc5be7e6c781ea4c8ba1a (main) from: Mark Jamsek date: Sat Dec 9 11:48:52 2023 UTC whitespace M lib/pack.c | 1+ 1- 1 file changed, 1 insertion(+), 1 deletion(-) diff 557f71d8d65ad007ecbe02338778f4911e49daa2 828e5074bf59310d99fdc5be7e6c781ea4c8ba1a commit - 557f71d8d65ad007ecbe02338778f4911e49daa2 commit + 828e5074bf59310d99fdc5be7e6c781ea4c8ba1a blob - 09136af9ef6031bc1ba4935ddaba53108806ba54 blob + 76eb2dde64f751aa977e37a5be06c8601f1fc0e3 --- lib/pack.c +++ lib/pack.c @@ -695,7 +695,7 @@ got_packidx_match_id_str_prefix(struct got_object_id_q int cmp; if (!got_sha1_digest_to_str(oid->sha1, id_str, sizeof(id_str))) - return got_error(GOT_ERR_NO_SPACE); + return got_error(GOT_ERR_NO_SPACE); cmp = strncmp(id_str, id_str_prefix, prefix_len); if (cmp < 0) { --[[ XXX Alternative fix to the matched_ids leak that _changes_ the current behaviour! XXdiff e881d04f6380990aa7a8fca766418f8b09837cef ba875b8cf46ca0a63466ddb66b3d2cfb37c875ef XXcommit - e881d04f6380990aa7a8fca766418f8b09837cef XXcommit + ba875b8cf46ca0a63466ddb66b3d2cfb37c875ef XXblob - 76eb2dde64f751aa977e37a5be06c8601f1fc0e3 XXblob + 762cf44cf20815499076425536178ffb4572414c XX--- lib/pack.c XX+++ lib/pack.c XX@@ -675,8 +675,6 @@ got_packidx_match_id_str_prefix(struct got_object_id_q XX struct got_packidx_object_id *oid; XX uint32_t i = 0; XX XX- STAILQ_INIT(matched_ids); XX- XX if (prefix_len < 2) XX return got_error_path(id_str_prefix, GOT_ERR_BAD_OBJ_ID_STR); XX --]] -- Mark Jamsek GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68