threads / patch / 62869

patch, 2 partsPlug two memory leaks exposed via Meson

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

## tl;dr

13 messages between Jan 29, 2025 and Jan 30, 2025. Diffs are folded; open one to read it.

replies: 12people: 3as markdown or json

Patrick Steinhardt· Jan 29, 2025, 16:24 UTC · lore

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

Patrick Steinhardt· Jan 29, 2025, 16:24 UTC · re: Patrick Steinhardt · lore

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

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

Show changes to unix-socket.c +3 −1

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

Junio C Hamano· Jan 29, 2025, 17:21 UTC · re: Patrick Steinhardt · lore

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

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

Jeff King· Jan 29, 2025, 20:07 UTC · re: Junio C Hamano · lore

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

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

Show 16 quoted lines

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

Patrick Steinhardt· Jan 30, 2025, 06:02 UTC · re: Jeff King · lore

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

On Wed, Jan 29, 2025 at 03:07:02PM -0500, Jeff King wrote:

Show 12 quoted lines

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

Show 11 quoted lines

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

Junio C Hamano· Jan 30, 2025, 17:35 UTC · re: Patrick Steinhardt · lore

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

Patrick Steinhardt <ps@pks.im> writes:

Show 8 quoted lines

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

Show 13 quoted lines

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

Patrick Steinhardt· Jan 29, 2025, 16:24 UTC · re: Patrick Steinhardt · lore

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

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

Show changes to scalar.c +3 −1

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

Jeff King· Jan 29, 2025, 20:05 UTC · re: Patrick Steinhardt · lore

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

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

Show 5 quoted lines

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

Show changes to t/t0301-credential-cache.sh +7 −1

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

Patrick Steinhardt· Jan 30, 2025, 06:04 UTC · re: Jeff King · lore

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

On Wed, Jan 29, 2025 at 03:05:09PM -0500, Jeff King wrote:

Show 18 quoted lines

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

Show 23 quoted lines

> 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

Patrick Steinhardt· Jan 30, 2025, 06:17 UTC · re: Patrick Steinhardt · lore

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

Hi,

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

Patrick Steinhardt· Jan 30, 2025, 06:17 UTC · re: Patrick Steinhardt · lore

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

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

Show changes to unix-socket.c +3 −1

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

Patrick Steinhardt· Jan 30, 2025, 06:17 UTC · re: Patrick Steinhardt · lore

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

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

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

Show changes to scalar.c +3 −1

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

Junio C Hamano· Jan 30, 2025, 19:19 UTC · re: Patrick Steinhardt · lore

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

Patrick Steinhardt <ps@pks.im> writes:

Show 15 quoted lines

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