| Message ID | 20250129-b4-pks-memory-leaks-v1-1-79e41299eb0c@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Plug two memory leaks exposed via Meson | expand |
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));
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
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
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.
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));
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(-)