Re: [PATCH 1/2] unix-socket: fix memory leak when chdir(3p) fails

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 29, 2025, 17:21 UTC
Message-ID: <xmqq34h1k02k.fsf@gitster.g>
In-Reply-To: <20250129-b4-pks-memory-leaks-v1-1-79e41299eb0c@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 12 quoted lines

> When trying to create a Unix socket in a path that exceeds the maximum
> socket name length we try to first change the directory into the parent
> folder before creating the socket to reduce the length of the name. When
> this fails we error out of `unix_sockaddr_init()` with an error code,
> which indicates to the caller that the context has not been initialized.
> Consequently, they don't release that context.
>
> This leads to a memory leak: when we have already populated the context
> with the original directory that we need to chdir(3p) back into, but
> then the chdir(3p) into the socket's parent directory fails, then we
> won't release the original directory's path. The leak is exposed by
> t0301, but only via Meson with `meson setup -Dsanitize=leak`:

Did you mean

    $ meson configure -Db_sanitize=leak
    $ meson test t0301-credential-cache

I'll need to figure out how to make various tweaks at runtime working with meson based build tree. The next thing I need to figure out is to see how to get verbose error output from the tests, as I cannot just go back to the source tree and say "cd t && sh t0301-credential-cache -v -i -x" because the build is out of tree.

Show 28 quoted lines

>     Direct leak of 129 byte(s) in 1 object(s) allocated from:
>         #0 0x5555555e85c6 in realloc.part.0 lsan_interceptors.cpp.o
>         #1 0x55555590e3d6 in xrealloc ../wrapper.c:140:8
>         #2 0x5555558c8fc6 in strbuf_grow ../strbuf.c:114:2
>         #3 0x5555558cacab in strbuf_getcwd ../strbuf.c:605:3
>         #4 0x555555923ff6 in unix_sockaddr_init ../unix-socket.c:65:7
>         #5 0x555555923e42 in unix_stream_connect ../unix-socket.c:84:6
>         #6 0x55555562a984 in send_request ../builtin/credential-cache.c:46:11
>         #7 0x55555562a89e in do_cache ../builtin/credential-cache.c:108:6
>         #8 0x55555562a655 in cmd_credential_cache ../builtin/credential-cache.c:178:3
>         #9 0x555555700547 in run_builtin ../git.c:480:11
>         #10 0x5555556ff0e0 in handle_builtin ../git.c:740:9
>         #11 0x5555556ffee8 in run_argv ../git.c:807:4
>         #12 0x5555556fee6b in cmd_main ../git.c:947:19
>         #13 0x55555593f689 in main ../common-main.c:64:11
>         #14 0x7ffff7a2a1fb in __libc_start_call_main (/nix/store/h7zcxabfxa7v5xdna45y2hplj31ncf8a-glibc-2.40-36/lib/libc.so.6+0x2a1fb) (BuildId: 0a855678aa0cb573cecbb2bcc73ab8239ec472d0)
>         #15 0x7ffff7a2a2b8 in __libc_start_main@GLIBC_2.2.5 (/nix/store/h7zcxabfxa7v5xdna45y2hplj31ncf8a-glibc-2.40-36/lib/libc.so.6+0x2a2b8) (BuildId: 0a855678aa0cb573cecbb2bcc73ab8239ec472d0)
>         #16 0x5555555ad1d4 in _start (git+0x591d4)
>
>     DEDUP_TOKEN: ___interceptor_realloc.part.0--xrealloc--strbuf_grow--strbuf_getcwd--unix_sockaddr_init--unix_stream_connect--send_request--do_cache--cmd_credential_cache--run_builtin--handle_builtin--run_argv--cmd_main--main--__libc_start_call_main--__libc_start_main@GLIBC_2.2.5--_start
>     SUMMARY: LeakSanitizer: 129 byte(s) leaked in 1 allocation(s).
>
> Fix this leak.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  unix-socket.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Thanks.  The analysis and the fix looked superbly clear.

Will queue.

Show 16 quoted lines

> diff --git a/unix-socket.c b/unix-socket.c
> index 483c9c448c..8860203c3f 100644
> --- a/unix-socket.c
> +++ b/unix-socket.c
> @@ -65,8 +65,10 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,
>  		if (strbuf_getcwd(&cwd))
>  			return -1;
>  		ctx->orig_dir = strbuf_detach(&cwd, NULL);
> -		if (chdir_len(dir, slash - dir) < 0)
> +		if (chdir_len(dir, slash - dir) < 0) {
> +			FREE_AND_NULL(ctx->orig_dir);
>  			return -1;
> +		}
>  	}
>  
>  	memset(sa, 0, sizeof(*sa));