From: Mark Jamsek Subject: got: cleanup failure leaks and set error on asprintf() failure To: Game of Trees Date: Wed, 11 Jan 2023 21:54:07 +1100 As per the subject, the following two diffs perform some minor housekeeping. First are some failure leaks spotted while implementing diffstat in lib/diff.c (some of which were also observed by op whose idea I went with instead of the original fix to free at every error return). The second is catching a couple instances I missed in got.c:cmd_diff() where error isn't set on asprintf() failure even though we goto done. ok? ----------------------------------------------- commit 43adae771e15759422dd9d9f229be7c77ffe4579 (main) from: Mark Jamsek date: Wed Jan 11 10:44:49 2023 UTC got: style(9) and cleanup failure leaks lib/diff.c Spotted while implementing diffstat plus one introduced with diffstat code. Don't leak 'change' on got_pathlist_append() error. And don't leak modestr{1,2} and l{1,2} char pointers in diff_blobs() and got_diff_tree(), respecitvely. Regarding modestr leaks, rather than free at all error return points, use op's suggestion to lift modestr vars to function scope. M lib/diff.c | 42+ 20- 1 file changed, 42 insertions(+), 20 deletions(-) diff 47504e80cba47264929a34e7311b2a98cea85b61 43adae771e15759422dd9d9f229be7c77ffe4579 commit - 47504e80cba47264929a34e7311b2a98cea85b61 commit + 43adae771e15759422dd9d9f229be7c77ffe4579 blob - 9ada3529102e62be359b43263e1847b0fc5a7ccc blob + 6f9c397cb5beed5d6de4ed2b82014650732b3318 --- lib/diff.c +++ lib/diff.c @@ -114,8 +114,10 @@ get_diffstat(struct got_diffstat_cb_arg *ds, const cha ++ds->nfiles; err = got_pathlist_append(ds->paths, path, change); - if (err) + if (err) { + free(change); return err; + } pe = TAILQ_LAST(ds->paths, got_pathlist_head); diffstat_field_width(&ds->max_path_len, &ds->add_cols, &ds->rm_cols, @@ -137,6 +139,7 @@ diff_blobs(struct got_diff_line **lines, size_t *nline char hex1[SHA1_DIGEST_STRING_LENGTH]; char hex2[SHA1_DIGEST_STRING_LENGTH]; const char *idstr1 = NULL, *idstr2 = NULL; + char *modestr1 = NULL, *modestr2 = NULL; off_t size1, size2; struct got_diffreg_result *result = NULL; off_t outoff = 0; @@ -185,8 +188,8 @@ diff_blobs(struct got_diff_line **lines, size_t *nline idstr2 = "/dev/null"; if (outfile) { - char *modestr1 = NULL, *modestr2 = NULL; int modebits; + if (mode1 && mode1 != mode2) { if (S_ISLNK(mode1)) modebits = S_IFLNK; @@ -232,9 +235,6 @@ diff_blobs(struct got_diff_line **lines, size_t *nline if (err) goto done; } - - free(modestr1); - free(modestr2); } err = got_diffreg(&result, f1, f2, diff_algo, ignore_whitespace, @@ -296,6 +296,8 @@ done: } done: + free(modestr1); + free(modestr2); if (resultp && err == NULL) *resultp = result; else if (result) { @@ -896,13 +898,16 @@ got_diff_tree(struct got_tree_object *tree1, struct go if (tree2) { te2 = got_object_tree_get_entry(tree2, 0); if (te2 && asprintf(&l2, "%s%s%s", label2, label2[0] ? "/" : "", - te2->name) == -1) - return got_error_from_errno("asprintf"); + te2->name) == -1) { + err = got_error_from_errno("asprintf"); + goto done; + } } do { if (te1) { struct got_tree_entry *te = NULL; + if (tree2) te = got_object_tree_find_entry(tree2, te1->name); @@ -910,10 +915,12 @@ got_diff_tree(struct got_tree_object *tree1, struct go free(l2); l2 = NULL; if (te && asprintf(&l2, "%s%s%s", label2, - label2[0] ? "/" : "", te->name) == -1) - return - got_error_from_errno("asprintf"); + label2[0] ? "/" : "", te->name) == -1) { + err = got_error_from_errno("asprintf"); + goto done; + } } + err = diff_entry_old_new(te1, te, f1, f2, fd1, fd2, l1, l2, repo, cb, cb_arg, diff_content); if (err) @@ -922,21 +929,27 @@ got_diff_tree(struct got_tree_object *tree1, struct go if (te2) { struct got_tree_entry *te = NULL; + if (tree1) te = got_object_tree_find_entry(tree1, te2->name); + free(l2); + l2 = NULL; if (te) { if (asprintf(&l2, "%s%s%s", label2, - label2[0] ? "/" : "", te->name) == -1) - return - got_error_from_errno("asprintf"); + label2[0] ? "/" : "", te->name) == -1) { + err = got_error_from_errno("asprintf"); + goto done; + } } else { if (asprintf(&l2, "%s%s%s", label2, - label2[0] ? "/" : "", te2->name) == -1) - return - got_error_from_errno("asprintf"); + label2[0] ? "/" : "", te2->name) == -1) { + err = got_error_from_errno("asprintf"); + goto done; + } } + err = diff_entry_new_old(te2, te, f1, f2, fd2, l2, repo, cb, cb_arg, diff_content); if (err) @@ -950,9 +963,12 @@ got_diff_tree(struct got_tree_object *tree1, struct go te1 = got_object_tree_get_entry(tree1, tidx1); if (te1 && asprintf(&l1, "%s%s%s", label1, - label1[0] ? "/" : "", te1->name) == -1) - return got_error_from_errno("asprintf"); + label1[0] ? "/" : "", te1->name) == -1) { + err = got_error_from_errno("asprintf"); + goto done; + } } + free(l2); l2 = NULL; if (te2) { @@ -960,11 +976,16 @@ got_diff_tree(struct got_tree_object *tree1, struct go te2 = got_object_tree_get_entry(tree2, tidx2); if (te2 && asprintf(&l2, "%s%s%s", label2, - label2[0] ? "/" : "", te2->name) == -1) - return got_error_from_errno("asprintf"); + label2[0] ? "/" : "", te2->name) == -1) { + err = got_error_from_errno("asprintf"); + goto done; + } } } while (te1 || te2); +done: + free(l1); + free(l2); return err; } @@ -1377,6 +1398,7 @@ done: *resultp = diffreg_result; else if (diffreg_result) { const struct got_error *free_err; + free_err = got_diffreg_result_free(diffreg_result); if (free_err && err == NULL) err = free_err; ----------------------------------------------- commit 47504e80cba47264929a34e7311b2a98cea85b61 from: Mark Jamsek date: Wed Jan 11 10:44:48 2023 UTC got: set error on asprintf() failure M got/got.c | 6+ 2- 1 file changed, 6 insertions(+), 2 deletions(-) diff 00b3e9ae14f04a45f1ca7445bade6b41a6e8a1c5 47504e80cba47264929a34e7311b2a98cea85b61 commit - 00b3e9ae14f04a45f1ca7445bade6b41a6e8a1c5 commit + 47504e80cba47264929a34e7311b2a98cea85b61 blob - 850056f73d0f8671f6fc63162e2753e92b8b8c41 blob + a20fdf567d2fb43a228a54708fee12c13f94796b --- got/got.c +++ got/got.c @@ -5249,8 +5249,10 @@ cmd_diff(int argc, char *argv[]) if (asprintf(&header, "diffstat %s%s", diff_staged ? "-s " : "", - got_worktree_get_root_path(worktree)) == -1) + got_worktree_get_root_path(worktree)) == -1) { + error = got_error_from_errno("asprintf"); goto done; + } error = print_diffstat(&dsa, &diffstat_paths, header); free(header); @@ -5421,8 +5423,10 @@ cmd_diff(int argc, char *argv[]) char *header = NULL; if (asprintf(&header, "diffstat %s %s", - labels[0], labels[1]) == -1) + labels[0], labels[1]) == -1) { + error = got_error_from_errno("asprintf"); goto done; + } error = print_diffstat(&dsa, &diffstat_paths, header); free(header); -- Mark Jamsek GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68