Skip to content

Commit 48176f9

Browse files
committed
connect: plug protocol capability leak
When pushing to a set of remotes using a nickname for the group, the client initializes the connection to each remote, talks to the remote and reads and parses capabilities line, and holds the capabilities in a file-scope static variable server_capabilities_v1. There are a few other such file-scope static variables, and these connections cannot be parallelized until they are refactored to a structure that keeps track of active connections. Which is *not* the theme of this patch ;-) For a single connection, the server_capabilities_v1 variable is initialized to NULL (at the program initialization), populated when we talk to the other side, used to look up capabilities of the other side possibly multiple times, and the memory is held by the variable until program exit, without leaking. When talking to multiple remotes, however, the server capabilities from the second connection overwrites without freeing the one from the first connection, which leaks. ==1080970==ERROR: LeakSanitizer: detected memory leaks Direct leak of 421 byte(s) in 2 object(s) allocated from: #0 0x5615305f849e in strdup (/home/gitster/g/git-jch/bin/bin/git+0x2b349e) (BuildId: 54d149994c9e85374831958f694bd0aa3b8b1e26) #1 0x561530e76cc4 in xstrdup /home/gitster/w/build/wrapper.c:43:14 #2 0x5615309cd7fa in process_capabilities /home/gitster/w/build/connect.c:243:27 #3 0x5615309cd502 in get_remote_heads /home/gitster/w/build/connect.c:366:4 #4 0x561530e2cb0b in handshake /home/gitster/w/build/transport.c:372:3 #5 0x561530e29ed7 in get_refs_via_connect /home/gitster/w/build/transport.c:398:9 #6 0x561530e26464 in transport_push /home/gitster/w/build/transport.c:1421:16 #7 0x561530800bec in push_with_options /home/gitster/w/build/builtin/push.c:387:8 #8 0x5615307ffb99 in do_push /home/gitster/w/build/builtin/push.c:442:7 #9 0x5615307fe926 in cmd_push /home/gitster/w/build/builtin/push.c:664:7 #10 0x56153065673f in run_builtin /home/gitster/w/build/git.c:506:11 #11 0x56153065342f in handle_builtin /home/gitster/w/build/git.c:779:9 #12 0x561530655b89 in run_argv /home/gitster/w/build/git.c:862:4 #13 0x561530652cba in cmd_main /home/gitster/w/build/git.c:984:19 #14 0x5615308dda0a in main /home/gitster/w/build/common-main.c:9:11 #15 0x7f051651bca7 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16 SUMMARY: AddressSanitizer: 421 byte(s) leaked in 2 allocation(s). Free the capablities data for the previous server before overwriting it with the next server to plug this leak. The added test fails without the freeing with SANITIZE=leak; I somehow couldn't get it fail reliably with SANITIZE=leak,address though. Signed-off-by: Junio C Hamano <gitster@pobox.com>
1 parent 9a2fb14 commit 48176f9

File tree

3 files changed

+42
-0
lines changed

3 files changed

+42
-0
lines changed

connect.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,8 @@ static void process_capabilities(struct packet_reader *reader, size_t *linelen)
240240
size_t nul_location = strlen(line);
241241
if (nul_location == *linelen)
242242
return;
243+
244+
free(server_capabilities_v1);
243245
server_capabilities_v1 = xstrdup(line + nul_location + 1);
244246
*linelen = nul_location;
245247

t/meson.build

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -688,6 +688,7 @@ integration_tests = [
688688
't5562-http-backend-content-length.sh',
689689
't5563-simple-http-auth.sh',
690690
't5564-http-proxy.sh',
691+
't5565-push-multiple.sh',
691692
't5570-git-daemon.sh',
692693
't5571-pre-push-hook.sh',
693694
't5572-pull-submodule.sh',

t/t5565-push-multiple.sh

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
#!/bin/sh
2+
3+
test_description='push to group'
4+
5+
. ./test-lib.sh
6+
7+
test_expect_success setup '
8+
for i in 1 2 3
9+
do
10+
git init dest-$i &&
11+
git -C dest-$i symbolic-ref HEAD refs/heads/not-a-branch ||
12+
return 1
13+
done &&
14+
test_tick &&
15+
git commit --allow-empty -m "initial" &&
16+
git config set --append remote.them.pushurl "file://$(pwd)/dest-1" &&
17+
git config set --append remote.them.pushurl "file://$(pwd)/dest-2" &&
18+
git config set --append remote.them.pushurl "file://$(pwd)/dest-3" &&
19+
git config set --append remote.them.push "+refs/heads/*:refs/heads/*"
20+
'
21+
22+
test_expect_success 'push to group' '
23+
git push them &&
24+
j= &&
25+
for i in 1 2 3
26+
do
27+
git -C dest-$i for-each-ref >actual-$i &&
28+
if test -n "$j"
29+
then
30+
test_cmp actual-$j actual-$i
31+
else
32+
cat actual-$i
33+
fi &&
34+
j=$i ||
35+
return 1
36+
done
37+
'
38+
39+
test_done

0 commit comments

Comments
 (0)