{"thread":{"id":"62869","subject":"[PATCH 0/2] Plug two memory leaks exposed via Meson","startedAt":"2025-01-29T16:24:19Z","lastAt":"2025-01-30T19:19:16Z","messageCount":13,"participants":["Patrick Steinhardt","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"511415","messageId":"20250129-b4-pks-memory-leaks-v1-0-79e41299eb0c@pks.im","threadId":"62869","inReplyTo":null,"subject":"[PATCH 0/2] Plug two memory leaks exposed via Meson","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-29T16:24:13Z","receivedAt":"2025-01-29T16:24:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nI've had the need to play around with the memory leak sanitizer today\nand for the first time used it with Meson. Interestingly enough, a test\nrun with Meson flags two memory leaks that our Makefile doesn't. I\nhaven't found the time yet to figure out why that is, but this small\npatch series fixes both of these leaks.\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (2):\n      unix-socket: fix memory leak when chdir(3p) fails\n      scalar: free result of `remote_default_branch()`\n\n scalar.c      | 4 +++-\n unix-socket.c | 4 +++-\n 2 files changed, 6 insertions(+), 2 deletions(-)\n\n\n---\nbase-commit: da898a5c645ce9b6d72c2d39abe1bc3d48cb0fdb\nchange-id: 20250129-b4-pks-memory-leaks-2a318e5afec1\n\n"},{"id":"511416","messageId":"20250129-b4-pks-memory-leaks-v1-1-79e41299eb0c@pks.im","threadId":"62869","inReplyTo":"20250129-b4-pks-memory-leaks-v1-0-79e41299eb0c@pks.im","subject":"[PATCH 1/2] unix-socket: fix memory leak when chdir(3p) fails","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-29T16:24:14Z","receivedAt":"2025-01-29T16:24:20Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When trying to create a Unix socket in a path that exceeds the maximum\nsocket name length we try to first change the directory into the parent\nfolder before creating the socket to reduce the length of the name. When\nthis fails we error out of `unix_sockaddr_init()` with an error code,\nwhich indicates to the caller that the context has not been initialized.\nConsequently, they don't release that context.\n\nThis leads to a memory leak: when we have already populated the context\nwith the original directory that we need to chdir(3p) back into, but\nthen the chdir(3p) into the socket's parent directory fails, then we\nwon't release the original directory's path. The leak is exposed by\nt0301, but only via Meson with `meson setup -Dsanitize=leak`:\n\n    Direct leak of 129 byte(s) in 1 object(s) allocated from:\n        #0 0x5555555e85c6 in realloc.part.0 lsan_interceptors.cpp.o\n        #1 0x55555590e3d6 in xrealloc ../wrapper.c:140:8\n        #2 0x5555558c8fc6 in strbuf_grow ../strbuf.c:114:2\n        #3 0x5555558cacab in strbuf_getcwd ../strbuf.c:605:3\n        #4 0x555555923ff6 in unix_sockaddr_init ../unix-socket.c:65:7\n        #5 0x555555923e42 in unix_stream_connect ../unix-socket.c:84:6\n        #6 0x55555562a984 in send_request ../builtin/credential-cache.c:46:11\n        #7 0x55555562a89e in do_cache ../builtin/credential-cache.c:108:6\n        #8 0x55555562a655 in cmd_credential_cache ../builtin/credential-cache.c:178:3\n        #9 0x555555700547 in run_builtin ../git.c:480:11\n        #10 0x5555556ff0e0 in handle_builtin ../git.c:740:9\n        #11 0x5555556ffee8 in run_argv ../git.c:807:4\n        #12 0x5555556fee6b in cmd_main ../git.c:947:19\n        #13 0x55555593f689 in main ../common-main.c:64:11\n        #14 0x7ffff7a2a1fb in __libc_start_call_main (/nix/store/h7zcxabfxa7v5xdna45y2hplj31ncf8a-glibc-2.40-36/lib/libc.so.6+0x2a1fb) (BuildId: 0a855678aa0cb573cecbb2bcc73ab8239ec472d0)\n        #15 0x7ffff7a2a2b8 in __libc_start_main@GLIBC_2.2.5 (/nix/store/h7zcxabfxa7v5xdna45y2hplj31ncf8a-glibc-2.40-36/lib/libc.so.6+0x2a2b8) (BuildId: 0a855678aa0cb573cecbb2bcc73ab8239ec472d0)\n        #16 0x5555555ad1d4 in _start (git+0x591d4)\n\n    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\n    SUMMARY: LeakSanitizer: 129 byte(s) leaked in 1 allocation(s).\n\nFix this leak.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n unix-socket.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/unix-socket.c b/unix-socket.c\nindex 483c9c448c..8860203c3f 100644\n--- a/unix-socket.c\n+++ b/unix-socket.c\n@@ -65,8 +65,10 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,\n \t\tif (strbuf_getcwd(&cwd))\n \t\t\treturn -1;\n \t\tctx->orig_dir = strbuf_detach(&cwd, NULL);\n-\t\tif (chdir_len(dir, slash - dir) < 0)\n+\t\tif (chdir_len(dir, slash - dir) < 0) {\n+\t\t\tFREE_AND_NULL(ctx->orig_dir);\n \t\t\treturn -1;\n+\t\t}\n \t}\n \n \tmemset(sa, 0, sizeof(*sa));\n\n-- \n2.48.1.362.g079036d154.dirty\n\n"},{"id":"511417","messageId":"20250129-b4-pks-memory-leaks-v1-2-79e41299eb0c@pks.im","threadId":"62869","inReplyTo":"20250129-b4-pks-memory-leaks-v1-0-79e41299eb0c@pks.im","subject":"[PATCH 2/2] scalar: free result of `remote_default_branch()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-29T16:24:15Z","receivedAt":"2025-01-29T16:24:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We don't free the result of `remote_default_branch()`, leading to a\nmemory leak. This leak is exposed by t9211, but only when run with Meson\nvia `meson setup -Dsanitize=leak`:\n\n    Direct leak of 5 byte(s) in 1 object(s) allocated from:\n        #0 0x5555555cfb93 in malloc (scalar+0x7bb93)\n        #1 0x5555556b05c2 in do_xmalloc ../wrapper.c:55:8\n        #2 0x5555556b06c4 in do_xmallocz ../wrapper.c:89:8\n        #3 0x5555556b0656 in xmallocz ../wrapper.c:97:9\n        #4 0x5555556b0728 in xmemdupz ../wrapper.c:113:16\n        #5 0x5555556b07a7 in xstrndup ../wrapper.c:119:9\n        #6 0x5555555d3a4b in remote_default_branch ../scalar.c:338:14\n        #7 0x5555555d20e6 in cmd_clone ../scalar.c:493:28\n        #8 0x5555555d196b in cmd_main ../scalar.c:992:14\n        #9 0x5555557c4059 in main ../common-main.c:64:11\n        #10 0x7ffff7a2a1fb in __libc_start_call_main (/nix/store/h7zcxabfxa7v5xdna45y2hplj31ncf8a-glibc-2.40-36/lib/libc.so.6+0x2a1fb) (BuildId: 0a855678aa0cb573cecbb2bcc73ab8239ec472d0)\n        #11 0x7ffff7a2a2b8 in __libc_start_main@GLIBC_2.2.5 (/nix/store/h7zcxabfxa7v5xdna45y2hplj31ncf8a-glibc-2.40-36/lib/libc.so.6+0x2a2b8) (BuildId: 0a855678aa0cb573cecbb2bcc73ab8239ec472d0)\n        #12 0x555555592054 in _start (scalar+0x3e054)\n\n    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\n    SUMMARY: LeakSanitizer: 5 byte(s) leaked in 1 allocation(s).\n\nAs the `branch` variable may contain a string constant obtained from\nparsing command line arguments we cannot free the leaking variable\ndirectly. Instead, introduce a new `branch_to_free` variable that only\never gets assigned the allocated string and free that one to plug the\nleak.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n scalar.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/scalar.c b/scalar.c\nindex f24bcd0169..da42b4be0c 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -409,6 +409,7 @@ void load_builtin_commands(const char *prefix UNUSED,\n static int cmd_clone(int argc, const char **argv)\n {\n \tconst char *branch = NULL;\n+\tchar *branch_to_free = NULL;\n \tint full_clone = 0, single_branch = 0, show_progress = isatty(2);\n \tint src = 1, tags = 1;\n \tstruct option clone_options[] = {\n@@ -490,7 +491,7 @@ static int cmd_clone(int argc, const char **argv)\n \t/* common-main already logs `argv` */\n \ttrace2_def_repo(the_repository);\n \n-\tif (!branch && !(branch = remote_default_branch(url))) {\n+\tif (!branch && !(branch = branch_to_free = remote_default_branch(url))) {\n \t\tres = error(_(\"failed to get default branch for '%s'\"), url);\n \t\tgoto cleanup;\n \t}\n@@ -552,6 +553,7 @@ static int cmd_clone(int argc, const char **argv)\n \tres = register_dir();\n \n cleanup:\n+\tfree(branch_to_free);\n \tfree(enlistment);\n \tfree(dir);\n \tstrbuf_release(&buf);\n\n-- \n2.48.1.362.g079036d154.dirty\n\n"},{"id":"511424","messageId":"xmqq34h1k02k.fsf@gitster.g","threadId":"62869","inReplyTo":"20250129-b4-pks-memory-leaks-v1-1-79e41299eb0c@pks.im","subject":"Re: [PATCH 1/2] unix-socket: fix memory leak when chdir(3p) fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-29T17:21:39Z","receivedAt":"2025-01-29T17:21:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> When trying to create a Unix socket in a path that exceeds the maximum\n> socket name length we try to first change the directory into the parent\n> folder before creating the socket to reduce the length of the name. When\n> this fails we error out of `unix_sockaddr_init()` with an error code,\n> which indicates to the caller that the context has not been initialized.\n> Consequently, they don't release that context.\n>\n> This leads to a memory leak: when we have already populated the context\n> with the original directory that we need to chdir(3p) back into, but\n> then the chdir(3p) into the socket's parent directory fails, then we\n> won't release the original directory's path. The leak is exposed by\n> t0301, but only via Meson with `meson setup -Dsanitize=leak`:\n\nDid you mean\n\n    $ meson configure -Db_sanitize=leak\n    $ meson test t0301-credential-cache\n\nI'll need to figure out how to make various tweaks at runtime\nworking with meson based build tree.  The next thing I need to\nfigure out is to see how to get verbose error output from the tests,\nas I cannot just go back to the source tree and say \"cd t && sh\nt0301-credential-cache -v -i -x\" because the build is out of tree.\n\n>     Direct leak of 129 byte(s) in 1 object(s) allocated from:\n>         #0 0x5555555e85c6 in realloc.part.0 lsan_interceptors.cpp.o\n>         #1 0x55555590e3d6 in xrealloc ../wrapper.c:140:8\n>         #2 0x5555558c8fc6 in strbuf_grow ../strbuf.c:114:2\n>         #3 0x5555558cacab in strbuf_getcwd ../strbuf.c:605:3\n>         #4 0x555555923ff6 in unix_sockaddr_init ../unix-socket.c:65:7\n>         #5 0x555555923e42 in unix_stream_connect ../unix-socket.c:84:6\n>         #6 0x55555562a984 in send_request ../builtin/credential-cache.c:46:11\n>         #7 0x55555562a89e in do_cache ../builtin/credential-cache.c:108:6\n>         #8 0x55555562a655 in cmd_credential_cache ../builtin/credential-cache.c:178:3\n>         #9 0x555555700547 in run_builtin ../git.c:480:11\n>         #10 0x5555556ff0e0 in handle_builtin ../git.c:740:9\n>         #11 0x5555556ffee8 in run_argv ../git.c:807:4\n>         #12 0x5555556fee6b in cmd_main ../git.c:947:19\n>         #13 0x55555593f689 in main ../common-main.c:64:11\n>         #14 0x7ffff7a2a1fb in __libc_start_call_main (/nix/store/h7zcxabfxa7v5xdna45y2hplj31ncf8a-glibc-2.40-36/lib/libc.so.6+0x2a1fb) (BuildId: 0a855678aa0cb573cecbb2bcc73ab8239ec472d0)\n>         #15 0x7ffff7a2a2b8 in __libc_start_main@GLIBC_2.2.5 (/nix/store/h7zcxabfxa7v5xdna45y2hplj31ncf8a-glibc-2.40-36/lib/libc.so.6+0x2a2b8) (BuildId: 0a855678aa0cb573cecbb2bcc73ab8239ec472d0)\n>         #16 0x5555555ad1d4 in _start (git+0x591d4)\n>\n>     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\n>     SUMMARY: LeakSanitizer: 129 byte(s) leaked in 1 allocation(s).\n>\n> Fix this leak.\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  unix-socket.c | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n\nThanks.  The analysis and the fix looked superbly clear.\n\nWill queue.\n\n> diff --git a/unix-socket.c b/unix-socket.c\n> index 483c9c448c..8860203c3f 100644\n> --- a/unix-socket.c\n> +++ b/unix-socket.c\n> @@ -65,8 +65,10 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,\n>  \t\tif (strbuf_getcwd(&cwd))\n>  \t\t\treturn -1;\n>  \t\tctx->orig_dir = strbuf_detach(&cwd, NULL);\n> -\t\tif (chdir_len(dir, slash - dir) < 0)\n> +\t\tif (chdir_len(dir, slash - dir) < 0) {\n> +\t\t\tFREE_AND_NULL(ctx->orig_dir);\n>  \t\t\treturn -1;\n> +\t\t}\n>  \t}\n>  \n>  \tmemset(sa, 0, sizeof(*sa));\n"},{"id":"511427","messageId":"20250129200509.GA2331283@coredump.intra.peff.net","threadId":"62869","inReplyTo":"20250129-b4-pks-memory-leaks-v1-0-79e41299eb0c@pks.im","subject":"Re: [PATCH 0/2] Plug two memory leaks exposed via Meson","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-29T20:05:09Z","receivedAt":"2025-01-29T20:05:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 29, 2025 at 05:24:13PM +0100, Patrick Steinhardt wrote:\n\n> I've had the need to play around with the memory leak sanitizer today\n> and for the first time used it with Meson. Interestingly enough, a test\n> run with Meson flags two memory leaks that our Makefile doesn't. I\n> haven't found the time yet to figure out why that is, but this small\n> patch series fixes both of these leaks.\n\nAt least for the first one, it depends on how long the path to your\ntrash directory is. Doing this:\n\n  make SANITIZE=leak\n  cd t\n  ./t0301-credential-cache.sh --root=/tmp/this_is_a_very_long_path/the_size_of_sockaddr_un_sun_path_is_usually_108\n\nwill fail reliably (it's not 108, but with the trash directory and xdg\nboilerplate tacked on, it is).  The failed chdir() triggers because it's\ntrying the xdg path to see if it exists.\n\nWith \"make\", my path is something like:\n\n  /home/peff/compile/git/t/trash directory.t0301-credential-cache/.cache/git/credential/socket\n\nwhich is 93 bytes. If I do an out-of-tree build into the \"build\"\ndirectory, then I get 109 bytes, one too many:\n\n  /home/peff/compile/git/build/test-output/trash directory.t0301-credential-cache/.cache/git/credential/socket\n\nso it is mostly a matter of luck combined with your personal directory\nlayout.\n\nThis test would trigger it reliably, but it's weirdly specific:\n\ndiff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh\nindex dc30289f75..0ef8ce4e60 100755\n--- a/t/t0301-credential-cache.sh\n+++ b/t/t0301-credential-cache.sh\n@@ -134,6 +134,13 @@ test_expect_success SYMLINKS 'use user socket if user directory is a symlink to\n \ttest_path_is_socket \"$HOME/.git-credential-cache/socket\"\n '\n \n+test_expect_success 'error path for chdir of long socket name' '\n+\tA=aaaaaaaaaaaaaaaa &&\n+\tLONG=$A/$A/$A/$A/$A/$A/$A/$A &&\n+\t# do not create $LONG; we want to trigger the error\n+\tgit credential-cache --socket \"$PWD/$LONG/socket\" exit\n+'\n+\n helper_test_timeout cache --timeout=1\n \n test_done\n\nSo I don't know if it's worth adding in to your patch. The fix itself is\nobviously correct.\n\n-Peff\n"},{"id":"511428","messageId":"20250129200702.GB2331283@coredump.intra.peff.net","threadId":"62869","inReplyTo":"xmqq34h1k02k.fsf@gitster.g","subject":"Re: [PATCH 1/2] unix-socket: fix memory leak when chdir(3p) fails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-29T20:07:02Z","receivedAt":"2025-01-29T20:07:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 29, 2025 at 09:21:39AM -0800, Junio C Hamano wrote:\n\n> > This leads to a memory leak: when we have already populated the context\n> > with the original directory that we need to chdir(3p) back into, but\n> > then the chdir(3p) into the socket's parent directory fails, then we\n> > won't release the original directory's path. The leak is exposed by\n> > t0301, but only via Meson with `meson setup -Dsanitize=leak`:\n> \n> Did you mean\n> \n>     $ meson configure -Db_sanitize=leak\n>     $ meson test t0301-credential-cache\n> \n> I'll need to figure out how to make various tweaks at runtime\n> working with meson based build tree.  The next thing I need to\n> figure out is to see how to get verbose error output from the tests,\n> as I cannot just go back to the source tree and say \"cd t && sh\n> t0301-credential-cache -v -i -x\" because the build is out of tree.\n\nI did:\n\n  GIT_BUILD_DIR=$PWD/../build ./t0301-credential-cache.sh -v -i\n\nbut I don't know if there's an easier way from meson.\n\n(The \"b_\" prefix on \"sanitize\" confused me as well after reading the\ncommit message).\n\n-Peff\n"},{"id":"511465","messageId":"Z5sWCxEF3J7t8WvW@pks.im","threadId":"62869","inReplyTo":"20250129200702.GB2331283@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] unix-socket: fix memory leak when chdir(3p) fails","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-30T06:02:51Z","receivedAt":"2025-01-30T06:03:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jan 29, 2025 at 03:07:02PM -0500, Jeff King wrote:\n> On Wed, Jan 29, 2025 at 09:21:39AM -0800, Junio C Hamano wrote:\n> \n> > > This leads to a memory leak: when we have already populated the context\n> > > with the original directory that we need to chdir(3p) back into, but\n> > > then the chdir(3p) into the socket's parent directory fails, then we\n> > > won't release the original directory's path. The leak is exposed by\n> > > t0301, but only via Meson with `meson setup -Dsanitize=leak`:\n> > \n> > Did you mean\n> > \n> >     $ meson configure -Db_sanitize=leak\n> >     $ meson test t0301-credential-cache\n\nOh, yes, I indeed forgot the `b_` prefix. Other than that I wanted to\nabbreviate steps a bit so that I don't have to give the full sequence of\ncommands, but my attempt was somewhat lacking :)\n\n> > I'll need to figure out how to make various tweaks at runtime\n> > working with meson based build tree.  The next thing I need to\n> > figure out is to see how to get verbose error output from the tests,\n> > as I cannot just go back to the source tree and say \"cd t && sh\n> > t0301-credential-cache -v -i -x\" because the build is out of tree.\n> \n> I did:\n> \n>   GIT_BUILD_DIR=$PWD/../build ./t0301-credential-cache.sh -v -i\n> \n> but I don't know if there's an easier way from meson.\n\nYou can pass arbitrary arguments via `--test-args`:\n\n    $ meson test -i --test-args=-vix t0301*\n\n`-i` makes the test run interactively so that stdout/stderr remains\nconnected to your terminal, which also allows you to use `test_pause` et\nal.\n\n> (The \"b_\" prefix on \"sanitize\" confused me as well after reading the\n> commit message).\n\nYou've probably been confused by the lack of \"b_\" in my commit message,\nnot by the prefix itself, which was a simple typo.\n\nBut regardless of that, in case anybody else reads this and wonders\nwhat the prefix means: the \"b_\" prefix comes from \"base options\". These\nare a couple of flags that Meson provides out of the box and control the\nbase mode that the compiler runs with. This includes e.g. sanitizers,\n`-Wl,--as-needed`, LTO, precompiled headers and other stuff. These\noptions can be discovered by running `meson configure` in either a build\ndirectory or the source directory.\n\nPatrick\n"},{"id":"511466","messageId":"Z5sWbg_0ri3V_yDm@pks.im","threadId":"62869","inReplyTo":"20250129200509.GA2331283@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] Plug two memory leaks exposed via Meson","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-30T06:04:30Z","receivedAt":"2025-01-30T06:04:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jan 29, 2025 at 03:05:09PM -0500, Jeff King wrote:\n> On Wed, Jan 29, 2025 at 05:24:13PM +0100, Patrick Steinhardt wrote:\n> \n> > I've had the need to play around with the memory leak sanitizer today\n> > and for the first time used it with Meson. Interestingly enough, a test\n> > run with Meson flags two memory leaks that our Makefile doesn't. I\n> > haven't found the time yet to figure out why that is, but this small\n> > patch series fixes both of these leaks.\n> \n> At least for the first one, it depends on how long the path to your\n> trash directory is. Doing this:\n> \n>   make SANITIZE=leak\n>   cd t\n>   ./t0301-credential-cache.sh --root=/tmp/this_is_a_very_long_path/the_size_of_sockaddr_un_sun_path_is_usually_108\n> \n> will fail reliably (it's not 108, but with the trash directory and xdg\n> boilerplate tacked on, it is).  The failed chdir() triggers because it's\n> trying the xdg path to see if it exists.\n\nThat makes sense indeed, thanks for digging. I'll add that info to the\ncommit message.\n\n[snip]\n> This test would trigger it reliably, but it's weirdly specific:\n> \n> diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh\n> index dc30289f75..0ef8ce4e60 100755\n> --- a/t/t0301-credential-cache.sh\n> +++ b/t/t0301-credential-cache.sh\n> @@ -134,6 +134,13 @@ test_expect_success SYMLINKS 'use user socket if user directory is a symlink to\n>  \ttest_path_is_socket \"$HOME/.git-credential-cache/socket\"\n>  '\n>  \n> +test_expect_success 'error path for chdir of long socket name' '\n> +\tA=aaaaaaaaaaaaaaaa &&\n> +\tLONG=$A/$A/$A/$A/$A/$A/$A/$A &&\n> +\t# do not create $LONG; we want to trigger the error\n> +\tgit credential-cache --socket \"$PWD/$LONG/socket\" exit\n> +'\n> +\n>  helper_test_timeout cache --timeout=1\n>  \n>  test_done\n> \n> So I don't know if it's worth adding in to your patch. The fix itself is\n> obviously correct.\n\nYeah, it doesn't really feel worth it from my perspective.\n\nPatrick\n"},{"id":"511467","messageId":"20250130-b4-pks-memory-leaks-v2-0-fc29dc7d4b19@pks.im","threadId":"62869","inReplyTo":"20250129-b4-pks-memory-leaks-v1-0-79e41299eb0c@pks.im","subject":"[PATCH v2 0/2] Plug two memory leaks exposed via Meson","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-30T06:17:37Z","receivedAt":"2025-01-30T06:17:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nI've had the need to play around with the memory leak sanitizer today\nand for the first time used it with Meson. Interestingly enough, a test\nrun with Meson flags two memory leaks that our Makefile doesn't. I\nhaven't found the time yet to figure out why that is, but this small\npatch series fixes both of these leaks.\n\nChanges in v2:\n  - Add an explanation why t0301 only fails sometimes.\n  - Fix commit messages to properly point out the `-Db_sanitize=leak`\n    option.\n  - Link to v1: https://lore.kernel.org/r/20250129-b4-pks-memory-leaks-v1-0-79e41299eb0c@pks.im\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (2):\n      unix-socket: fix memory leak when chdir(3p) fails\n      scalar: free result of `remote_default_branch()`\n\n scalar.c      | 4 +++-\n unix-socket.c | 4 +++-\n 2 files changed, 6 insertions(+), 2 deletions(-)\n\nRange-diff versus v1:\n\n1:  94205fa36f ! 1:  63fd407649 unix-socket: fix memory leak when chdir(3p) fails\n    @@ Commit message\n         with the original directory that we need to chdir(3p) back into, but\n         then the chdir(3p) into the socket's parent directory fails, then we\n         won't release the original directory's path. The leak is exposed by\n    -    t0301, but only via Meson with `meson setup -Dsanitize=leak`:\n    +    t0301, but only when running tests in a directory hierarchy whose path\n    +    is long enough to make the socket name length exceed the maximum socket\n    +    name length:\n     \n             Direct leak of 129 byte(s) in 1 object(s) allocated from:\n                 #0 0x5555555e85c6 in realloc.part.0 lsan_interceptors.cpp.o\n2:  a4b4b31084 ! 2:  9248925ca0 scalar: free result of `remote_default_branch()`\n    @@ Commit message\n     \n         We don't free the result of `remote_default_branch()`, leading to a\n         memory leak. This leak is exposed by t9211, but only when run with Meson\n    -    via `meson setup -Dsanitize=leak`:\n    +    with the `-Db_sanitize=leak` option:\n     \n             Direct leak of 5 byte(s) in 1 object(s) allocated from:\n                 #0 0x5555555cfb93 in malloc (scalar+0x7bb93)\n    @@ Commit message\n         ever gets assigned the allocated string and free that one to plug the\n         leak.\n     \n    +    It is unclear why the leak isn't flagged when running the test via our\n    +    Makefile.\n    +\n         Signed-off-by: Patrick Steinhardt <ps@pks.im>\n     \n      ## scalar.c ##\n\n---\nbase-commit: da898a5c645ce9b6d72c2d39abe1bc3d48cb0fdb\nchange-id: 20250129-b4-pks-memory-leaks-2a318e5afec1\n\n"},{"id":"511468","messageId":"20250130-b4-pks-memory-leaks-v2-1-fc29dc7d4b19@pks.im","threadId":"62869","inReplyTo":"20250130-b4-pks-memory-leaks-v2-0-fc29dc7d4b19@pks.im","subject":"[PATCH v2 1/2] unix-socket: fix memory leak when chdir(3p) fails","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-30T06:17:38Z","receivedAt":"2025-01-30T06:17:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When trying to create a Unix socket in a path that exceeds the maximum\nsocket name length we try to first change the directory into the parent\nfolder before creating the socket to reduce the length of the name. When\nthis fails we error out of `unix_sockaddr_init()` with an error code,\nwhich indicates to the caller that the context has not been initialized.\nConsequently, they don't release that context.\n\nThis leads to a memory leak: when we have already populated the context\nwith the original directory that we need to chdir(3p) back into, but\nthen the chdir(3p) into the socket's parent directory fails, then we\nwon't release the original directory's path. The leak is exposed by\nt0301, but only when running tests in a directory hierarchy whose path\nis long enough to make the socket name length exceed the maximum socket\nname length:\n\n    Direct leak of 129 byte(s) in 1 object(s) allocated from:\n        #0 0x5555555e85c6 in realloc.part.0 lsan_interceptors.cpp.o\n        #1 0x55555590e3d6 in xrealloc ../wrapper.c:140:8\n        #2 0x5555558c8fc6 in strbuf_grow ../strbuf.c:114:2\n        #3 0x5555558cacab in strbuf_getcwd ../strbuf.c:605:3\n        #4 0x555555923ff6 in unix_sockaddr_init ../unix-socket.c:65:7\n        #5 0x555555923e42 in unix_stream_connect ../unix-socket.c:84:6\n        #6 0x55555562a984 in send_request ../builtin/credential-cache.c:46:11\n        #7 0x55555562a89e in do_cache ../builtin/credential-cache.c:108:6\n        #8 0x55555562a655 in cmd_credential_cache ../builtin/credential-cache.c:178:3\n        #9 0x555555700547 in run_builtin ../git.c:480:11\n        #10 0x5555556ff0e0 in handle_builtin ../git.c:740:9\n        #11 0x5555556ffee8 in run_argv ../git.c:807:4\n        #12 0x5555556fee6b in cmd_main ../git.c:947:19\n        #13 0x55555593f689 in main ../common-main.c:64:11\n        #14 0x7ffff7a2a1fb in __libc_start_call_main (/nix/store/h7zcxabfxa7v5xdna45y2hplj31ncf8a-glibc-2.40-36/lib/libc.so.6+0x2a1fb) (BuildId: 0a855678aa0cb573cecbb2bcc73ab8239ec472d0)\n        #15 0x7ffff7a2a2b8 in __libc_start_main@GLIBC_2.2.5 (/nix/store/h7zcxabfxa7v5xdna45y2hplj31ncf8a-glibc-2.40-36/lib/libc.so.6+0x2a2b8) (BuildId: 0a855678aa0cb573cecbb2bcc73ab8239ec472d0)\n        #16 0x5555555ad1d4 in _start (git+0x591d4)\n\n    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\n    SUMMARY: LeakSanitizer: 129 byte(s) leaked in 1 allocation(s).\n\nFix this leak.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n unix-socket.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/unix-socket.c b/unix-socket.c\nindex 483c9c448c..8860203c3f 100644\n--- a/unix-socket.c\n+++ b/unix-socket.c\n@@ -65,8 +65,10 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,\n \t\tif (strbuf_getcwd(&cwd))\n \t\t\treturn -1;\n \t\tctx->orig_dir = strbuf_detach(&cwd, NULL);\n-\t\tif (chdir_len(dir, slash - dir) < 0)\n+\t\tif (chdir_len(dir, slash - dir) < 0) {\n+\t\t\tFREE_AND_NULL(ctx->orig_dir);\n \t\t\treturn -1;\n+\t\t}\n \t}\n \n \tmemset(sa, 0, sizeof(*sa));\n\n-- \n2.48.1.468.gbf5f394be8.dirty\n\n"},{"id":"511469","messageId":"20250130-b4-pks-memory-leaks-v2-2-fc29dc7d4b19@pks.im","threadId":"62869","inReplyTo":"20250130-b4-pks-memory-leaks-v2-0-fc29dc7d4b19@pks.im","subject":"[PATCH v2 2/2] scalar: free result of `remote_default_branch()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-30T06:17:39Z","receivedAt":"2025-01-30T06:17:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We don't free the result of `remote_default_branch()`, leading to a\nmemory leak. This leak is exposed by t9211, but only when run with Meson\nwith the `-Db_sanitize=leak` option:\n\n    Direct leak of 5 byte(s) in 1 object(s) allocated from:\n        #0 0x5555555cfb93 in malloc (scalar+0x7bb93)\n        #1 0x5555556b05c2 in do_xmalloc ../wrapper.c:55:8\n        #2 0x5555556b06c4 in do_xmallocz ../wrapper.c:89:8\n        #3 0x5555556b0656 in xmallocz ../wrapper.c:97:9\n        #4 0x5555556b0728 in xmemdupz ../wrapper.c:113:16\n        #5 0x5555556b07a7 in xstrndup ../wrapper.c:119:9\n        #6 0x5555555d3a4b in remote_default_branch ../scalar.c:338:14\n        #7 0x5555555d20e6 in cmd_clone ../scalar.c:493:28\n        #8 0x5555555d196b in cmd_main ../scalar.c:992:14\n        #9 0x5555557c4059 in main ../common-main.c:64:11\n        #10 0x7ffff7a2a1fb in __libc_start_call_main (/nix/store/h7zcxabfxa7v5xdna45y2hplj31ncf8a-glibc-2.40-36/lib/libc.so.6+0x2a1fb) (BuildId: 0a855678aa0cb573cecbb2bcc73ab8239ec472d0)\n        #11 0x7ffff7a2a2b8 in __libc_start_main@GLIBC_2.2.5 (/nix/store/h7zcxabfxa7v5xdna45y2hplj31ncf8a-glibc-2.40-36/lib/libc.so.6+0x2a2b8) (BuildId: 0a855678aa0cb573cecbb2bcc73ab8239ec472d0)\n        #12 0x555555592054 in _start (scalar+0x3e054)\n\n    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\n    SUMMARY: LeakSanitizer: 5 byte(s) leaked in 1 allocation(s).\n\nAs the `branch` variable may contain a string constant obtained from\nparsing command line arguments we cannot free the leaking variable\ndirectly. Instead, introduce a new `branch_to_free` variable that only\never gets assigned the allocated string and free that one to plug the\nleak.\n\nIt is unclear why the leak isn't flagged when running the test via our\nMakefile.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n scalar.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/scalar.c b/scalar.c\nindex f24bcd0169..da42b4be0c 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -409,6 +409,7 @@ void load_builtin_commands(const char *prefix UNUSED,\n static int cmd_clone(int argc, const char **argv)\n {\n \tconst char *branch = NULL;\n+\tchar *branch_to_free = NULL;\n \tint full_clone = 0, single_branch = 0, show_progress = isatty(2);\n \tint src = 1, tags = 1;\n \tstruct option clone_options[] = {\n@@ -490,7 +491,7 @@ static int cmd_clone(int argc, const char **argv)\n \t/* common-main already logs `argv` */\n \ttrace2_def_repo(the_repository);\n \n-\tif (!branch && !(branch = remote_default_branch(url))) {\n+\tif (!branch && !(branch = branch_to_free = remote_default_branch(url))) {\n \t\tres = error(_(\"failed to get default branch for '%s'\"), url);\n \t\tgoto cleanup;\n \t}\n@@ -552,6 +553,7 @@ static int cmd_clone(int argc, const char **argv)\n \tres = register_dir();\n \n cleanup:\n+\tfree(branch_to_free);\n \tfree(enlistment);\n \tfree(dir);\n \tstrbuf_release(&buf);\n\n-- \n2.48.1.468.gbf5f394be8.dirty\n\n"},{"id":"511512","messageId":"xmqqldusfbn2.fsf@gitster.g","threadId":"62869","inReplyTo":"Z5sWCxEF3J7t8WvW@pks.im","subject":"Re: [PATCH 1/2] unix-socket: fix memory leak when chdir(3p) fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-30T17:35:13Z","receivedAt":"2025-01-30T17:35:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>> > Did you mean\n>> > \n>> >     $ meson configure -Db_sanitize=leak\n>> >     $ meson test t0301-credential-cache\n>\n> Oh, yes, I indeed forgot the `b_` prefix. Other than that I wanted to\n> abbreviate steps a bit so that I don't have to give the full sequence of\n> commands, but my attempt was somewhat lacking :)\n\nThanks.  It also confused me trying between setup and configure.  \n\nAs the use of meson in this project is a fairly recent development,\nif we want to entice more people and interest those in the \"make\"\nworld, we should try to leave enough droppings for them, even the\nmeson-novice ones like myself, to try out themselves whenever we\nhave a chance, and the proposed log message of a commit that adds or\nfixes meson related part of the system is one of the good place to\ndo so.\n\n> You can pass arbitrary arguments via `--test-args`:\n>\n>     $ meson test -i --test-args=-vix t0301*\n>\n> `-i` makes the test run interactively so that stdout/stderr remains\n> connected to your terminal, which also allows you to use `test_pause` et\n> al.\n>\n>> (The \"b_\" prefix on \"sanitize\" confused me as well after reading the\n>> commit message).\n>\n> You've probably been confused by the lack of \"b_\" in my commit message,\n> not by the prefix itself, which was a simple typo.\n\nFor me, both were confusing equally ;-)\n\n> ...\n> options can be discovered by running `meson configure` in either a build\n> directory or the source directory.\n\nVery nice to know.\n\nThanks.\n"},{"id":"511520","messageId":"xmqqfrl0cdot.fsf@gitster.g","threadId":"62869","inReplyTo":"20250130-b4-pks-memory-leaks-v2-0-fc29dc7d4b19@pks.im","subject":"Re: [PATCH v2 0/2] Plug two memory leaks exposed via Meson","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-30T19:19:14Z","receivedAt":"2025-01-30T19:19:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> I've had the need to play around with the memory leak sanitizer today\n> and for the first time used it with Meson. Interestingly enough, a test\n> run with Meson flags two memory leaks that our Makefile doesn't. I\n> haven't found the time yet to figure out why that is, but this small\n> patch series fixes both of these leaks.\n>\n> Changes in v2:\n>   - Add an explanation why t0301 only fails sometimes.\n>   - Fix commit messages to properly point out the `-Db_sanitize=leak`\n>     option.\n>   - Link to v1: https://lore.kernel.org/r/20250129-b4-pks-memory-leaks-v1-0-79e41299eb0c@pks.im\n>\n> Thanks!\n>\n> Patrick\n\nThanks, will queue.  Looking good.\n"}]}