promisor-remote: fix segfault when remote URL is missing
Using strvec_push() to push `NULL` into a 'strvec' results in a segfault, because `xstrdup(NULL)` crashes. So when an URL is missing from the config, let's not push the remote name and URL into the 'strvec's. While at it, let's also not push them in case the URL is empty. It's just not worth the trouble and it's consistent with how Git otherwise treats missing and empty URLs in the same way. Note that in case of missing or empty URL, Git uses the remote name to fetch, which can work if the remote is on the same filesystem. So configurations where the client, server and remote are all on the same filesystem may need URLs to be configured even if they are the same as the remote names. But this is a rare case, and the work around is easy enough. We leave improving the strvec API and/or xstrdup() for a future separate effort. While at it, let's also use git_config_get_string_tmp() instead of git_config_get_string() to simplify memory management. Helped-by: Jeff King <peff@peff.net> Signed-off-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Christian Couder committed
Mar 18, 2025 at 12:00 UTC
b059339bb32eb24c51378afd13814f45353fc0c4
2 files changed
+69
-8
promisor-remote.c
+8
-8
@@ -323,13 +323,15 @@ static void promisor_info_vecs(struct repository *repo,
323
promisor_remote_init(repo);
324
325
for (r = repo->promisor_remote_config->promisors; r; r = r->next) {
326
- char *url;
326
+ const char *url;
327
char *url_key = xstrfmt("remote.%s.url", r->name);
328
329
- strvec_push(names, r->name);
330
- strvec_push(urls, git_config_get_string(url_key, &url) ? NULL : url);
329
+ /* Only add remotes with a non empty URL */
330
+ if (!git_config_get_string_tmp(url_key, &url) && *url) {
331
+ strvec_push(names, r->name);
332
+ strvec_push(urls, url);
333
+ }
334
332
- free(url);
335
free(url_key);
336
}
337
}
@@ -356,10 +358,8 @@ char *promisor_remote_info(struct repository *repo)
358
strbuf_addch(&sb, ';');
359
strbuf_addstr(&sb, "name=");
360
strbuf_addstr_urlencode(&sb, names.v[i], allow_unsanitized);
359
- if (urls.v[i]) {
360
- strbuf_addstr(&sb, ",url=");
361
- strbuf_addstr_urlencode(&sb, urls.v[i], allow_unsanitized);
362
- }
361
+ strbuf_addstr(&sb, ",url=");
362
+ strbuf_addstr_urlencode(&sb, urls.v[i], allow_unsanitized);
363
}
364
365
strvec_clear(&names);
t/t5710-promisor-remote-capability.sh
+61
@@ -193,6 +193,25 @@ test_expect_success "clone with 'KnownName' and different remote names" '
193
initialize_server 1 "$oid"
194
'
195
196
+test_expect_success "clone with 'KnownName' and missing URL in the config" '
197
+ git -C server config promisor.advertise true &&
198
+ test_when_finished "rm -rf client" &&
199
+
200
+ # Clone from server to create a client
201
+ # Lazy fetching by the client from the LOP will fail because of the
202
+ # missing URL in the client config, so the server will have to lazy
203
+ # fetch from the LOP.
204
+ GIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \
205
+ -c promisor.acceptfromserver=KnownName \
206
+ --no-local --filter="blob:limit=5k" server client &&
207
+
208
+ # Check that the largest object is not missing on the server
209
+ check_missing_objects server 0 "" &&
210
+
211
+ # Reinitialize server so that the largest object is missing again
212
+ initialize_server 1 "$oid"
213
+'
214
+
215
test_expect_success "clone with promisor.acceptfromserver set to 'KnownUrl'" '
216
git -C server config promisor.advertise true &&
217
test_when_finished "rm -rf client" &&
@@ -228,6 +247,48 @@ test_expect_success "clone with 'KnownUrl' and different remote urls" '
247
initialize_server 1 "$oid"
248
'
249
250
+test_expect_success "clone with 'KnownUrl' and url not configured on the server" '
251
+ git -C server config promisor.advertise true &&
252
+ test_when_finished "rm -rf client" &&
253
+
254
+ test_when_finished "git -C server config set remote.lop.url \"file://$(pwd)/lop\"" &&
255
+ git -C server config unset remote.lop.url &&
256
+
257
+ # Clone from server to create a client
258
+ # It should fail because the client will reject the LOP as URLs are
259
+ # different, and the server cannot lazy fetch as the LOP URL is
260
+ # missing, so the remote name will be used instead which will fail.
261
+ test_must_fail env GIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \
262
+ -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
263
+ -c remote.lop.url="file://$(pwd)/lop" \
264
+ -c promisor.acceptfromserver=KnownUrl \
265
+ --no-local --filter="blob:limit=5k" server client &&
266
+
267
+ # Check that the largest object is still missing on the server
268
+ check_missing_objects server 1 "$oid"
269
+'
270
+
271
+test_expect_success "clone with 'KnownUrl' and empty url, so not advertised" '
272
+ git -C server config promisor.advertise true &&
273
+ test_when_finished "rm -rf client" &&
274
+
275
+ test_when_finished "git -C server config set remote.lop.url \"file://$(pwd)/lop\"" &&
276
+ git -C server config set remote.lop.url "" &&
277
+
278
+ # Clone from server to create a client
279
+ # It should fail because the client will reject the LOP as an empty URL is
280
+ # not advertised, and the server cannot lazy fetch as the LOP URL is empty,
281
+ # so the remote name will be used instead which will fail.
282
+ test_must_fail env GIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \
283
+ -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
284
+ -c remote.lop.url="file://$(pwd)/lop" \
285
+ -c promisor.acceptfromserver=KnownUrl \
286
+ --no-local --filter="blob:limit=5k" server client &&
287
+
288
+ # Check that the largest object is still missing on the server
289
+ check_missing_objects server 1 "$oid"
290
+'
291
+
292
test_expect_success "clone with promisor.advertise set to 'true' but don't delete the client" '
293
git -C server config promisor.advertise true &&
294