Re: [PATCH 03/24] format-patch: don't leak "extra_headers" or "ref_message_ids"
From: Taylor Blau <hidden>
Date: 2022-03-09 20:34:41
On Wed, Mar 09, 2022 at 02:16:33PM +0100, Ævar Arnfjörð Bjarmason wrote:
quoted hunk ↗ jump to hunk
@@ -1946,7 +1947,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) strbuf_addch(&buf, '\n'); } - rev.extra_headers = strbuf_detach(&buf, NULL); + extra_headers = strbuf_detach(&buf, NULL); + rev.extra_headers = extra_headers;
Small nit, these two assignments could be combined on the same line without wrapping. But obviously not a big deal.
quoted hunk ↗ jump to hunk
@@ -2173,8 +2175,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) prepare_bases(&bases, base, list, nr); } - if (in_reply_to || thread || cover_letter) - rev.ref_message_ids = xcalloc(1, sizeof(struct string_list)); + if (in_reply_to || thread || cover_letter) { + rev.ref_message_ids = xmalloc(sizeof(*rev.ref_message_ids)); + string_list_init_nodup(rev.ref_message_ids); + }
OK, and this is the "while we're at it..." part of your commit message. I did find this a little confusing to read at first, so wouldn't have minded to see this and the rest of the change split across two patches, but I think this is fine, too.
quoted hunk ↗ jump to hunk
@@ -2281,6 +2285,11 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) strbuf_release(&rdiff1); strbuf_release(&rdiff2); strbuf_release(&rdiff_title); + free(extra_headers); + if (rev.ref_message_ids) { + string_list_clear(rev.ref_message_ids, 0);
I was surprised to learn that string_list_clear() is not a noop when its
first argument is NULL.
Looking around, it probably wouldn't help that much. E.g., running
something like:
git grep -B1 'string_list_clear(' | grep if -A1
doesn't turn up many results like the above. So maybe some good
#leftoverbits there, although maybe not.
+ free(rev.ref_message_ids);
This could go in or out of the if-statement. Thanks, Taylor