bundle cmd: stop leaking memory from parse_options_cmd_bundle()

Fix a memory leak from the prefix_filename() function introduced with its use in 3b754eedd5 (bundle: use prefix_filename with bundle path, 2017-03-20). As noted in that commit the leak was intentional as a part of being sloppy about freeing resources just before we exit, I'm changing this because I'll be fixing other memory leaks in the bundle API (including the library version) in subsequent commits. It's easier to reason about those fixes if valgrind runs cleanly at the end without any leaks whatsoever. An earlier version of this change[1] went out of its way to not leak memory on the die() codepaths here, but doing so will only avoid reports of potential leaks under heap-only leak trackers such as valgrind, not the SANITIZE=leak mode. Avoiding those leaks as well might be useful to enable us to run cleanly under the likes of valgrind in the future. But for now the relative verbosity of the resulting code, and the fact that we don't have some valgrind or SANITIZE=leak mode as part of our CI (it's only run ad-hoc, see [2]), means we're not worrying about that for now. 1. https://lore.kernel.org/git/87v95vdxrc.fsf@evledraar.gmail.com/ 2. https://lore.kernel.org/git/87czsv2idy.fsf@evledraar.gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Acked-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Ævar Arnfjörð Bjarmason committed Jul 2, 2021 at 11:57 UTC db6bfb9fe8098b8795d8e6bb8c6d85fd39f2cdaf
1 file changed +41 -21
builtin/bundle.c
+41 -21
@@ -46,7 +46,7 @@ static int parse_options_cmd_bundle(int argc,
46 const char* prefix,
47 const char * const usagestr[],
48 const struct option options[],
49 - const char **bundle_file) {
49 + char **bundle_file) {
50 int newargc;
51 newargc = parse_options(argc, argv, NULL, options, usagestr,
52 PARSE_OPT_STOP_AT_NON_OPTION);
@@ -61,7 +61,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
61 int progress = isatty(STDERR_FILENO);
62 struct strvec pack_opts;
63 int version = -1;
64 -
64 + int ret;
65 struct option options[] = {
66 OPT_SET_INT('q', "quiet", &progress,
67 N_("do not show progress meter"), 0),
@@ -76,7 +76,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
76 N_("specify bundle format version")),
77 OPT_END()
78 };
79 - const char* bundle_file;
79 + char *bundle_file;
80
81 argc = parse_options_cmd_bundle(argc, argv, prefix,
82 builtin_bundle_create_usage, options, &bundle_file);
@@ -94,75 +94,95 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
94
95 if (!startup_info->have_repository)
96 die(_("Need a repository to create a bundle."));
97 - return !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
97 + ret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
98 + free(bundle_file);
99 + return ret;
100 }
101
102 static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
103 struct bundle_header header;
104 int bundle_fd = -1;
105 int quiet = 0;
104 -
106 + int ret;
107 struct option options[] = {
108 OPT_BOOL('q', "quiet", &quiet,
109 N_("do not show bundle details")),
110 OPT_END()
111 };
110 - const char* bundle_file;
112 + char *bundle_file;
113
114 argc = parse_options_cmd_bundle(argc, argv, prefix,
115 builtin_bundle_verify_usage, options, &bundle_file);
116 /* bundle internals use argv[1] as further parameters */
117
118 memset(&header, 0, sizeof(header));
117 - if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)
118 - return 1;
119 + if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
120 + ret = 1;
121 + goto cleanup;
122 + }
123 close(bundle_fd);
120 - if (verify_bundle(the_repository, &header, !quiet))
121 - return 1;
124 + if (verify_bundle(the_repository, &header, !quiet)) {
125 + ret = 1;
126 + goto cleanup;
127 + }
128 +
129 fprintf(stderr, _("%s is okay\n"), bundle_file);
123 - return 0;
130 + ret = 0;
131 +cleanup:
132 + free(bundle_file);
133 + return ret;
134 }
135
136 static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix) {
137 struct bundle_header header;
138 int bundle_fd = -1;
129 -
139 + int ret;
140 struct option options[] = {
141 OPT_END()
142 };
133 - const char* bundle_file;
143 + char *bundle_file;
144
145 argc = parse_options_cmd_bundle(argc, argv, prefix,
146 builtin_bundle_list_heads_usage, options, &bundle_file);
147 /* bundle internals use argv[1] as further parameters */
148
149 memset(&header, 0, sizeof(header));
140 - if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)
141 - return 1;
150 + if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
151 + ret = 1;
152 + goto cleanup;
153 + }
154 close(bundle_fd);
143 - return !!list_bundle_refs(&header, argc, argv);
155 + ret = !!list_bundle_refs(&header, argc, argv);
156 +cleanup:
157 + free(bundle_file);
158 + return ret;
159 }
160
161 static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) {
162 struct bundle_header header;
163 int bundle_fd = -1;
149 -
164 + int ret;
165 struct option options[] = {
166 OPT_END()
167 };
153 - const char* bundle_file;
168 + char *bundle_file;
169
170 argc = parse_options_cmd_bundle(argc, argv, prefix,
171 builtin_bundle_unbundle_usage, options, &bundle_file);
172 /* bundle internals use argv[1] as further parameters */
173
174 memset(&header, 0, sizeof(header));
160 - if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)
161 - return 1;
175 + if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
176 + ret = 1;
177 + goto cleanup;
178 + }
179 if (!startup_info->have_repository)
180 die(_("Need a repository to unbundle."));
164 - return !!unbundle(the_repository, &header, bundle_fd, 0) ||
181 + ret = !!unbundle(the_repository, &header, bundle_fd, 0) ||
182 list_bundle_refs(&header, argc, argv);
183 +cleanup:
184 + free(bundle_file);
185 + return ret;
186 }
187
188 int cmd_bundle(int argc, const char **argv, const char *prefix)