[PATCH 0/2] Plug two memory leaks exposed via Meson

[PATCH 0/2] Plug two memory leaks exposed via Meson

[Patrick Steinhardt]

Hi,

I've had the need to play around with the memory leak sanitizer today
and for the first time used it with Meson. Interestingly enough, a test
run with Meson flags two memory leaks that our Makefile doesn't. I
haven't found the time yet to figure out why that is, but this small
patch series fixes both of these leaks.

Thanks!

Patrick

---
Patrick Steinhardt (2):
      unix-socket: fix memory leak when chdir(3p) fails
      scalar: free result of `remote_default_branch()`

 scalar.c      | 4 +++-
 unix-socket.c | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)


---
base-commit: da898a5c645ce9b6d72c2d39abe1bc3d48cb0fdb
change-id: 20250129-b4-pks-memory-leaks-2a318e5afec1

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

[Patrick Steinhardt]

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`:

    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(-)

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));

-- 
2.48.1.362.g079036d154.dirty

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> 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.

>     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.

> 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));

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

[Jeff King]

On Wed, Jan 29, 2025 at 09:21:39AM -0800, Junio C Hamano wrote:

> > 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.

I did:

  GIT_BUILD_DIR=$PWD/../build ./t0301-credential-cache.sh -v -i

but I don't know if there's an easier way from meson.

(The "b_" prefix on "sanitize" confused me as well after reading the
commit message).

-Peff

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

[Patrick Steinhardt]

On Wed, Jan 29, 2025 at 03:07:02PM -0500, Jeff King wrote:
> On Wed, Jan 29, 2025 at 09:21:39AM -0800, Junio C Hamano wrote:
> 
> > > 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

Oh, yes, I indeed forgot the `b_` prefix. Other than that I wanted to
abbreviate steps a bit so that I don't have to give the full sequence of
commands, but my attempt was somewhat lacking :)

> > 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.
> 
> I did:
> 
>   GIT_BUILD_DIR=$PWD/../build ./t0301-credential-cache.sh -v -i
> 
> but I don't know if there's an easier way from meson.

You can pass arbitrary arguments via `--test-args`:

    $ meson test -i --test-args=-vix t0301*

`-i` makes the test run interactively so that stdout/stderr remains
connected to your terminal, which also allows you to use `test_pause` et
al.

> (The "b_" prefix on "sanitize" confused me as well after reading the
> commit message).

You've probably been confused by the lack of "b_" in my commit message,
not by the prefix itself, which was a simple typo.

But regardless of that, in case anybody else reads this and wonders
what the prefix means: the "b_" prefix comes from "base options". These
are a couple of flags that Meson provides out of the box and control the
base mode that the compiler runs with. This includes e.g. sanitizers,
`-Wl,--as-needed`, LTO, precompiled headers and other stuff. These
options can be discovered by running `meson configure` in either a build
directory or the source directory.

Patrick

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> > Did you mean
>> > 
>> >     $ meson configure -Db_sanitize=leak
>> >     $ meson test t0301-credential-cache
>
> Oh, yes, I indeed forgot the `b_` prefix. Other than that I wanted to
> abbreviate steps a bit so that I don't have to give the full sequence of
> commands, but my attempt was somewhat lacking :)

Thanks.  It also confused me trying between setup and configure.  

As the use of meson in this project is a fairly recent development,
if we want to entice more people and interest those in the "make"
world, we should try to leave enough droppings for them, even the
meson-novice ones like myself, to try out themselves whenever we
have a chance, and the proposed log message of a commit that adds or
fixes meson related part of the system is one of the good place to
do so.

> You can pass arbitrary arguments via `--test-args`:
>
>     $ meson test -i --test-args=-vix t0301*
>
> `-i` makes the test run interactively so that stdout/stderr remains
> connected to your terminal, which also allows you to use `test_pause` et
> al.
>
>> (The "b_" prefix on "sanitize" confused me as well after reading the
>> commit message).
>
> You've probably been confused by the lack of "b_" in my commit message,
> not by the prefix itself, which was a simple typo.

For me, both were confusing equally ;-)

> ...
> options can be discovered by running `meson configure` in either a build
> directory or the source directory.

Very nice to know.

Thanks.

[PATCH 2/2] scalar: free result of `remote_default_branch()`

[Patrick Steinhardt]

We don't free the result of `remote_default_branch()`, leading to a
memory leak. This leak is exposed by t9211, but only when run with Meson
via `meson setup -Dsanitize=leak`:

    Direct leak of 5 byte(s) in 1 object(s) allocated from:
        #0 0x5555555cfb93 in malloc (scalar+0x7bb93)
        #1 0x5555556b05c2 in do_xmalloc ../wrapper.c:55:8
        #2 0x5555556b06c4 in do_xmallocz ../wrapper.c:89:8
        #3 0x5555556b0656 in xmallocz ../wrapper.c:97:9
        #4 0x5555556b0728 in xmemdupz ../wrapper.c:113:16
        #5 0x5555556b07a7 in xstrndup ../wrapper.c:119:9
        #6 0x5555555d3a4b in remote_default_branch ../scalar.c:338:14
        #7 0x5555555d20e6 in cmd_clone ../scalar.c:493:28
        #8 0x5555555d196b in cmd_main ../scalar.c:992:14
        #9 0x5555557c4059 in main ../common-main.c:64:11
        #10 0x7ffff7a2a1fb in __libc_start_call_main (/nix/store/h7zcxabfxa7v5xdna45y2hplj31ncf8a-glibc-2.40-36/lib/libc.so.6+0x2a1fb) (BuildId: 0a855678aa0cb573cecbb2bcc73ab8239ec472d0)
        #11 0x7ffff7a2a2b8 in __libc_start_main@GLIBC_2.2.5 (/nix/store/h7zcxabfxa7v5xdna45y2hplj31ncf8a-glibc-2.40-36/lib/libc.so.6+0x2a2b8) (BuildId: 0a855678aa0cb573cecbb2bcc73ab8239ec472d0)
        #12 0x555555592054 in _start (scalar+0x3e054)

    DEDUP_TOKEN: __interceptor_malloc--do_xmalloc--do_xmallocz--xmallocz--xmemdupz--xstrndup--remote_default_branch--cmd_clone--cmd_main--main--__libc_start_call_main--__libc_start_main@GLIBC_2.2.5--_start
    SUMMARY: LeakSanitizer: 5 byte(s) leaked in 1 allocation(s).

As the `branch` variable may contain a string constant obtained from
parsing command line arguments we cannot free the leaking variable
directly. Instead, introduce a new `branch_to_free` variable that only
ever gets assigned the allocated string and free that one to plug the
leak.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 scalar.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/scalar.c b/scalar.c
index f24bcd0169..da42b4be0c 100644
--- a/scalar.c
+++ b/scalar.c
@@ -409,6 +409,7 @@ void load_builtin_commands(const char *prefix UNUSED,
 static int cmd_clone(int argc, const char **argv)
 {
 	const char *branch = NULL;
+	char *branch_to_free = NULL;
 	int full_clone = 0, single_branch = 0, show_progress = isatty(2);
 	int src = 1, tags = 1;
 	struct option clone_options[] = {
@@ -490,7 +491,7 @@ static int cmd_clone(int argc, const char **argv)
 	/* common-main already logs `argv` */
 	trace2_def_repo(the_repository);
 
-	if (!branch && !(branch = remote_default_branch(url))) {
+	if (!branch && !(branch = branch_to_free = remote_default_branch(url))) {
 		res = error(_("failed to get default branch for '%s'"), url);
 		goto cleanup;
 	}
@@ -552,6 +553,7 @@ static int cmd_clone(int argc, const char **argv)
 	res = register_dir();
 
 cleanup:
+	free(branch_to_free);
 	free(enlistment);
 	free(dir);
 	strbuf_release(&buf);

-- 
2.48.1.362.g079036d154.dirty

Re: [PATCH 0/2] Plug two memory leaks exposed via Meson

[Jeff King]

On Wed, Jan 29, 2025 at 05:24:13PM +0100, Patrick Steinhardt wrote:

> I've had the need to play around with the memory leak sanitizer today
> and for the first time used it with Meson. Interestingly enough, a test
> run with Meson flags two memory leaks that our Makefile doesn't. I
> haven't found the time yet to figure out why that is, but this small
> patch series fixes both of these leaks.

At least for the first one, it depends on how long the path to your
trash directory is. Doing this:

  make SANITIZE=leak
  cd t
  ./t0301-credential-cache.sh --root=/tmp/this_is_a_very_long_path/the_size_of_sockaddr_un_sun_path_is_usually_108

will fail reliably (it's not 108, but with the trash directory and xdg
boilerplate tacked on, it is).  The failed chdir() triggers because it's
trying the xdg path to see if it exists.

With "make", my path is something like:

  /home/peff/compile/git/t/trash directory.t0301-credential-cache/.cache/git/credential/socket

which is 93 bytes. If I do an out-of-tree build into the "build"
directory, then I get 109 bytes, one too many:

  /home/peff/compile/git/build/test-output/trash directory.t0301-credential-cache/.cache/git/credential/socket

so it is mostly a matter of luck combined with your personal directory
layout.

This test would trigger it reliably, but it's weirdly specific:

diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
index dc30289f75..0ef8ce4e60 100755
--- a/t/t0301-credential-cache.sh
+++ b/t/t0301-credential-cache.sh
@@ -134,6 +134,13 @@ test_expect_success SYMLINKS 'use user socket if user directory is a symlink to
 	test_path_is_socket "$HOME/.git-credential-cache/socket"
 '
 
+test_expect_success 'error path for chdir of long socket name' '
+	A=aaaaaaaaaaaaaaaa &&
+	LONG=$A/$A/$A/$A/$A/$A/$A/$A &&
+	# do not create $LONG; we want to trigger the error
+	git credential-cache --socket "$PWD/$LONG/socket" exit
+'
+
 helper_test_timeout cache --timeout=1
 
 test_done

So I don't know if it's worth adding in to your patch. The fix itself is
obviously correct.

-Peff

Re: [PATCH 0/2] Plug two memory leaks exposed via Meson

[Patrick Steinhardt]

On Wed, Jan 29, 2025 at 03:05:09PM -0500, Jeff King wrote:
> On Wed, Jan 29, 2025 at 05:24:13PM +0100, Patrick Steinhardt wrote:
> 
> > I've had the need to play around with the memory leak sanitizer today
> > and for the first time used it with Meson. Interestingly enough, a test
> > run with Meson flags two memory leaks that our Makefile doesn't. I
> > haven't found the time yet to figure out why that is, but this small
> > patch series fixes both of these leaks.
> 
> At least for the first one, it depends on how long the path to your
> trash directory is. Doing this:
> 
>   make SANITIZE=leak
>   cd t
>   ./t0301-credential-cache.sh --root=/tmp/this_is_a_very_long_path/the_size_of_sockaddr_un_sun_path_is_usually_108
> 
> will fail reliably (it's not 108, but with the trash directory and xdg
> boilerplate tacked on, it is).  The failed chdir() triggers because it's
> trying the xdg path to see if it exists.

That makes sense indeed, thanks for digging. I'll add that info to the
commit message.

[snip]
> This test would trigger it reliably, but it's weirdly specific:
> 
> diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
> index dc30289f75..0ef8ce4e60 100755
> --- a/t/t0301-credential-cache.sh
> +++ b/t/t0301-credential-cache.sh
> @@ -134,6 +134,13 @@ test_expect_success SYMLINKS 'use user socket if user directory is a symlink to
>  	test_path_is_socket "$HOME/.git-credential-cache/socket"
>  '
>  
> +test_expect_success 'error path for chdir of long socket name' '
> +	A=aaaaaaaaaaaaaaaa &&
> +	LONG=$A/$A/$A/$A/$A/$A/$A/$A &&
> +	# do not create $LONG; we want to trigger the error
> +	git credential-cache --socket "$PWD/$LONG/socket" exit
> +'
> +
>  helper_test_timeout cache --timeout=1
>  
>  test_done
> 
> So I don't know if it's worth adding in to your patch. The fix itself is
> obviously correct.

Yeah, it doesn't really feel worth it from my perspective.

Patrick

[PATCH v2 0/2] Plug two memory leaks exposed via Meson

[Patrick Steinhardt]

Hi,

I've had the need to play around with the memory leak sanitizer today
and for the first time used it with Meson. Interestingly enough, a test
run with Meson flags two memory leaks that our Makefile doesn't. I
haven't found the time yet to figure out why that is, but this small
patch series fixes both of these leaks.

Changes in v2:
  - Add an explanation why t0301 only fails sometimes.
  - Fix commit messages to properly point out the `-Db_sanitize=leak`
    option.
  - Link to v1: https://lore.kernel.org/r/20250129-b4-pks-memory-leaks-v1-0-79e41299eb0c@pks.im

Thanks!

Patrick

---
Patrick Steinhardt (2):
      unix-socket: fix memory leak when chdir(3p) fails
      scalar: free result of `remote_default_branch()`

 scalar.c      | 4 +++-
 unix-socket.c | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

Range-diff versus v1:

1:  94205fa36f ! 1:  63fd407649 unix-socket: fix memory leak when chdir(3p) fails
    @@ Commit message
         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`:
    +    t0301, but only when running tests in a directory hierarchy whose path
    +    is long enough to make the socket name length exceed the maximum socket
    +    name length:
     
             Direct leak of 129 byte(s) in 1 object(s) allocated from:
                 #0 0x5555555e85c6 in realloc.part.0 lsan_interceptors.cpp.o
2:  a4b4b31084 ! 2:  9248925ca0 scalar: free result of `remote_default_branch()`
    @@ Commit message
     
         We don't free the result of `remote_default_branch()`, leading to a
         memory leak. This leak is exposed by t9211, but only when run with Meson
    -    via `meson setup -Dsanitize=leak`:
    +    with the `-Db_sanitize=leak` option:
     
             Direct leak of 5 byte(s) in 1 object(s) allocated from:
                 #0 0x5555555cfb93 in malloc (scalar+0x7bb93)
    @@ Commit message
         ever gets assigned the allocated string and free that one to plug the
         leak.
     
    +    It is unclear why the leak isn't flagged when running the test via our
    +    Makefile.
    +
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## scalar.c ##

---
base-commit: da898a5c645ce9b6d72c2d39abe1bc3d48cb0fdb
change-id: 20250129-b4-pks-memory-leaks-2a318e5afec1

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

[Patrick Steinhardt]

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 when running tests in a directory hierarchy whose path
is long enough to make the socket name length exceed the maximum socket
name length:

    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(-)

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));

-- 
2.48.1.468.gbf5f394be8.dirty

[PATCH v2 2/2] scalar: free result of `remote_default_branch()`

[Patrick Steinhardt]

We don't free the result of `remote_default_branch()`, leading to a
memory leak. This leak is exposed by t9211, but only when run with Meson
with the `-Db_sanitize=leak` option:

    Direct leak of 5 byte(s) in 1 object(s) allocated from:
        #0 0x5555555cfb93 in malloc (scalar+0x7bb93)
        #1 0x5555556b05c2 in do_xmalloc ../wrapper.c:55:8
        #2 0x5555556b06c4 in do_xmallocz ../wrapper.c:89:8
        #3 0x5555556b0656 in xmallocz ../wrapper.c:97:9
        #4 0x5555556b0728 in xmemdupz ../wrapper.c:113:16
        #5 0x5555556b07a7 in xstrndup ../wrapper.c:119:9
        #6 0x5555555d3a4b in remote_default_branch ../scalar.c:338:14
        #7 0x5555555d20e6 in cmd_clone ../scalar.c:493:28
        #8 0x5555555d196b in cmd_main ../scalar.c:992:14
        #9 0x5555557c4059 in main ../common-main.c:64:11
        #10 0x7ffff7a2a1fb in __libc_start_call_main (/nix/store/h7zcxabfxa7v5xdna45y2hplj31ncf8a-glibc-2.40-36/lib/libc.so.6+0x2a1fb) (BuildId: 0a855678aa0cb573cecbb2bcc73ab8239ec472d0)
        #11 0x7ffff7a2a2b8 in __libc_start_main@GLIBC_2.2.5 (/nix/store/h7zcxabfxa7v5xdna45y2hplj31ncf8a-glibc-2.40-36/lib/libc.so.6+0x2a2b8) (BuildId: 0a855678aa0cb573cecbb2bcc73ab8239ec472d0)
        #12 0x555555592054 in _start (scalar+0x3e054)

    DEDUP_TOKEN: __interceptor_malloc--do_xmalloc--do_xmallocz--xmallocz--xmemdupz--xstrndup--remote_default_branch--cmd_clone--cmd_main--main--__libc_start_call_main--__libc_start_main@GLIBC_2.2.5--_start
    SUMMARY: LeakSanitizer: 5 byte(s) leaked in 1 allocation(s).

As the `branch` variable may contain a string constant obtained from
parsing command line arguments we cannot free the leaking variable
directly. Instead, introduce a new `branch_to_free` variable that only
ever gets assigned the allocated string and free that one to plug the
leak.

It is unclear why the leak isn't flagged when running the test via our
Makefile.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 scalar.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/scalar.c b/scalar.c
index f24bcd0169..da42b4be0c 100644
--- a/scalar.c
+++ b/scalar.c
@@ -409,6 +409,7 @@ void load_builtin_commands(const char *prefix UNUSED,
 static int cmd_clone(int argc, const char **argv)
 {
 	const char *branch = NULL;
+	char *branch_to_free = NULL;
 	int full_clone = 0, single_branch = 0, show_progress = isatty(2);
 	int src = 1, tags = 1;
 	struct option clone_options[] = {
@@ -490,7 +491,7 @@ static int cmd_clone(int argc, const char **argv)
 	/* common-main already logs `argv` */
 	trace2_def_repo(the_repository);
 
-	if (!branch && !(branch = remote_default_branch(url))) {
+	if (!branch && !(branch = branch_to_free = remote_default_branch(url))) {
 		res = error(_("failed to get default branch for '%s'"), url);
 		goto cleanup;
 	}
@@ -552,6 +553,7 @@ static int cmd_clone(int argc, const char **argv)
 	res = register_dir();
 
 cleanup:
+	free(branch_to_free);
 	free(enlistment);
 	free(dir);
 	strbuf_release(&buf);

-- 
2.48.1.468.gbf5f394be8.dirty

Re: [PATCH v2 0/2] Plug two memory leaks exposed via Meson

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> I've had the need to play around with the memory leak sanitizer today
> and for the first time used it with Meson. Interestingly enough, a test
> run with Meson flags two memory leaks that our Makefile doesn't. I
> haven't found the time yet to figure out why that is, but this small
> patch series fixes both of these leaks.
>
> Changes in v2:
>   - Add an explanation why t0301 only fails sometimes.
>   - Fix commit messages to properly point out the `-Db_sanitize=leak`
>     option.
>   - Link to v1: https://lore.kernel.org/r/20250129-b4-pks-memory-leaks-v1-0-79e41299eb0c@pks.im
>
> Thanks!
>
> Patrick

Thanks, will queue.  Looking good.