| Message ID | 20250207153112.3939799-12-alex.bennee@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | maintainer updates for feb25 (qtest, gdbstub, plugins) pre-PR | expand |
07.02.2025 18:31, Alex Bennée wrote: > From: Ilya Leoshkevich <iii@linux.ibm.com> > > In case an emulated process execve()s another emulated process, bind() > will fail, because the socket already exists. So try deleting it. Use > the existing unix_listen() function which does this. Link qemu-user > with qemu-sockets.c and add the monitor_get_fd() stub. > > Note that it is not possible to handle this in do_execv(): deleting > gdbserver_user_state.socket_path before safe_execve() is not correct, > because the latter may fail, and afterwards we may lose control. Please note: this is linux-user stuff, which is usually linked statically. By linking it with qemu-sockets, we basically broke static linking, because qemu-sockets uses getaddrinfo() &Co. The previous code, I think, was there for a reason, - to avoid this linkage. How do you think about reverting this one and addressing the original problem without using qemu-sockets? Alternatively, it might be possible to split qemu-sockets.c into unix-related stuff and generic stuff. Thanks, /mjt
Michael Tokarev <mjt@tls.msk.ru> writes: > 07.02.2025 18:31, Alex Bennée wrote: >> From: Ilya Leoshkevich <iii@linux.ibm.com> >> In case an emulated process execve()s another emulated process, >> bind() >> will fail, because the socket already exists. So try deleting it. Use >> the existing unix_listen() function which does this. Link qemu-user >> with qemu-sockets.c and add the monitor_get_fd() stub. >> Note that it is not possible to handle this in do_execv(): deleting >> gdbserver_user_state.socket_path before safe_execve() is not correct, >> because the latter may fail, and afterwards we may lose control. > > Please note: this is linux-user stuff, which is usually linked statically. > By linking it with qemu-sockets, we basically broke static linking, because > qemu-sockets uses getaddrinfo() &Co. The previous code, I think, was there > for a reason, - to avoid this linkage. Oops, how come we didn't notice? We do have a bunch of --static targets in the CI. > How do you think about reverting this one and addressing the original > problem without using qemu-sockets? > > Alternatively, it might be possible to split qemu-sockets.c into unix-related > stuff and generic stuff. You mean move all the unix_XXX() functions into a new unit that ensures we don't need getaddrinfo()? > > Thanks, > > /mjt
On Tue, 20 May 2025 at 16:53, Alex Bennée <alex.bennee@linaro.org> wrote: > > Michael Tokarev <mjt@tls.msk.ru> writes: > > > 07.02.2025 18:31, Alex Bennée wrote: > >> From: Ilya Leoshkevich <iii@linux.ibm.com> > >> In case an emulated process execve()s another emulated process, > >> bind() > >> will fail, because the socket already exists. So try deleting it. Use > >> the existing unix_listen() function which does this. Link qemu-user > >> with qemu-sockets.c and add the monitor_get_fd() stub. > >> Note that it is not possible to handle this in do_execv(): deleting > >> gdbserver_user_state.socket_path before safe_execve() is not correct, > >> because the latter may fail, and afterwards we may lose control. > > > > Please note: this is linux-user stuff, which is usually linked statically. > > By linking it with qemu-sockets, we basically broke static linking, because > > qemu-sockets uses getaddrinfo() &Co. The previous code, I think, was there > > for a reason, - to avoid this linkage. > > Oops, how come we didn't notice? We do have a bunch of --static targets > in the CI. We ignore these warnings because upstream glib means we always have a bunch of false positives when we link a static qemu-user executable. So this one (which is in our own code, not in glib) slipped through. I think it's in the same "annoying noise" category as the glib ones, because the unix_listen() function won't actually end up in a call to getaddrinfo(). But since this one's in our own code we ought to fix it, either by splitting the source file or by reverting the commit. thanks -- PMM
On 2025-05-20 15:50, Michael Tokarev wrote: > 07.02.2025 18:31, Alex Bennée wrote: >> From: Ilya Leoshkevich <iii@linux.ibm.com> >> >> In case an emulated process execve()s another emulated process, bind() >> will fail, because the socket already exists. So try deleting it. Use >> the existing unix_listen() function which does this. Link qemu-user >> with qemu-sockets.c and add the monitor_get_fd() stub. >> >> Note that it is not possible to handle this in do_execv(): deleting >> gdbserver_user_state.socket_path before safe_execve() is not correct, >> because the latter may fail, and afterwards we may lose control. > > Please note: this is linux-user stuff, which is usually linked > statically. > By linking it with qemu-sockets, we basically broke static linking, > because > qemu-sockets uses getaddrinfo() &Co. The previous code, I think, was > there > for a reason, - to avoid this linkage. > > How do you think about reverting this one and addressing the original > problem without using qemu-sockets? > > Alternatively, it might be possible to split qemu-sockets.c into > unix-related > stuff and generic stuff. > > Thanks, > > /mjt I can split it. However, wasn't it already broken in this regard? With fccb744f41c69fec6fd92225fe907c6e69de5d44^ I get: [2/2] Linking target qemu-s390x /usr/bin/ld: /usr/lib64/libglib-2.0.a(gutils.c.o): in function `g_get_user_database_entry': (.text+0xeb): warning: Using 'getpwnam_r' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking /usr/bin/ld: (.text+0x2be): warning: Using 'getpwuid' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking /usr/bin/ld: (.text+0x134): warning: Using 'getpwuid_r' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking This comes from glib, but the ultimate result is still the same. Also, what are the symptoms of the breakage? IIUC as long as execution does not reach getaddrinfo(), which it in this case should not, because it is used only on inet paths, there should not be any issues, right? In any case, the new warning is annoying and I better fix it for this reason alone.
Ilya Leoshkevich <iii@linux.ibm.com> writes: > On 2025-05-20 15:50, Michael Tokarev wrote: >> 07.02.2025 18:31, Alex Bennée wrote: >>> From: Ilya Leoshkevich <iii@linux.ibm.com> >>> In case an emulated process execve()s another emulated process, >>> bind() >>> will fail, because the socket already exists. So try deleting it. Use >>> the existing unix_listen() function which does this. Link qemu-user >>> with qemu-sockets.c and add the monitor_get_fd() stub. >>> Note that it is not possible to handle this in do_execv(): deleting >>> gdbserver_user_state.socket_path before safe_execve() is not correct, >>> because the latter may fail, and afterwards we may lose control. >> Please note: this is linux-user stuff, which is usually linked >> statically. >> By linking it with qemu-sockets, we basically broke static linking, >> because >> qemu-sockets uses getaddrinfo() &Co. The previous code, I think, >> was there >> for a reason, - to avoid this linkage. >> How do you think about reverting this one and addressing the >> original >> problem without using qemu-sockets? >> Alternatively, it might be possible to split qemu-sockets.c into >> unix-related >> stuff and generic stuff. >> Thanks, >> /mjt > > I can split it. > > However, wasn't it already broken in this regard? > With fccb744f41c69fec6fd92225fe907c6e69de5d44^ I get: > > [2/2] Linking target qemu-s390x > /usr/bin/ld: /usr/lib64/libglib-2.0.a(gutils.c.o): in function > `g_get_user_database_entry': > (.text+0xeb): warning: Using 'getpwnam_r' in statically linked > applications requires at runtime the shared libraries from the glibc > version used for linking > /usr/bin/ld: (.text+0x2be): warning: Using 'getpwuid' in statically > linked applications requires at runtime the shared libraries from the > glibc version used for linking > /usr/bin/ld: (.text+0x134): warning: Using 'getpwuid_r' in statically > linked applications requires at runtime the shared libraries from the > glibc version used for linking > > This comes from glib, but the ultimate result is still the same. > > Also, what are the symptoms of the breakage? IIUC as long as execution > does not reach getaddrinfo(), which it in this case should not, because > it is used only on inet paths, there should not be any issues, right? > > In any case, the new warning is annoying and I better fix it for this > reason alone. I sent: Message-Id: <20250520165706.3976971-1-alex.bennee@linaro.org> Date: Tue, 20 May 2025 17:57:06 +0100 Subject: [RFC PATCH] util: split unix socket functions out of qemu-sockets From: =?UTF-8?q?Alex=20Benn=C3=A9e?= <alex.bennee@linaro.org> Although I'm still seeing some warnings on my build: # QEMU configure log Tue 20 May 18:14:18 BST 2025 # Configured with: '../../configure' '--disable-docs' '--disable-tools' '--disable-system' '--disable-vhost-kernel' '--disable-vhost-user' '--disable-vhost-net' '--static' Gives: [142/142] Linking target qemu-xtensaeb /usr/bin/ld: /usr/lib/x86_64-linux-gnu/libglib-2.0.a(gutils.c.o): in function `g_get_user_database_entry': (.text+0x287): warning: Using 'getpwuid' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking /usr/bin/ld: (.text+0xe0): warning: Using 'getpwnam_r' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking /usr/bin/ld: (.text+0x11e): warning: Using 'getpwuid_r' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
On Tue, 20 May 2025 at 23:22, Ilya Leoshkevich <iii@linux.ibm.com> wrote: > However, wasn't it already broken in this regard? > With fccb744f41c69fec6fd92225fe907c6e69de5d44^ I get: > > [2/2] Linking target qemu-s390x > /usr/bin/ld: /usr/lib64/libglib-2.0.a(gutils.c.o): in function > `g_get_user_database_entry': > (.text+0xeb): warning: Using 'getpwnam_r' in statically linked > applications requires at runtime the shared libraries from the glibc > version used for linking > /usr/bin/ld: (.text+0x2be): warning: Using 'getpwuid' in statically > linked applications requires at runtime the shared libraries from the > glibc version used for linking > /usr/bin/ld: (.text+0x134): warning: Using 'getpwuid_r' in statically > linked applications requires at runtime the shared libraries from the > glibc version used for linking > > This comes from glib, but the ultimate result is still the same. Those are in upstream glib, as you note. We can't fix those (unless we have the enthusiasm to write patches for upstream glib: last time we asked, they were not against the idea, but nobody on either side had the time available to try to write the necessary patches). But we can and should fix the cases in our own code. > Also, what are the symptoms of the breakage? IIUC as long as execution > does not reach getaddrinfo(), which it in this case should not, because > it is used only on inet paths, there should not be any issues, right? Correct -- if we don't call the function it's fine. But the easiest way to be sure we don't call the function is to not link it in :-) Otherwise future code changes could result in a call without our realizing it. Also, mjt's packaging for Debian puts in some stubs for the offending getwpuid etc functions, which suppress the glib warnings (this is why he noticed this whereas none of the rest of us did): https://sources.debian.org/patches/qemu/1:10.0.0%2Bds-2/static-linux-user-stubs.diff/ thanks -- PMM
On Wed, May 21, 2025 at 04:34:24PM +0100, Peter Maydell wrote: > On Tue, 20 May 2025 at 23:22, Ilya Leoshkevich <iii@linux.ibm.com> wrote: > > However, wasn't it already broken in this regard? > > With fccb744f41c69fec6fd92225fe907c6e69de5d44^ I get: > > > > [2/2] Linking target qemu-s390x > > /usr/bin/ld: /usr/lib64/libglib-2.0.a(gutils.c.o): in function > > `g_get_user_database_entry': > > (.text+0xeb): warning: Using 'getpwnam_r' in statically linked > > applications requires at runtime the shared libraries from the glibc > > version used for linking > > /usr/bin/ld: (.text+0x2be): warning: Using 'getpwuid' in statically > > linked applications requires at runtime the shared libraries from the > > glibc version used for linking > > /usr/bin/ld: (.text+0x134): warning: Using 'getpwuid_r' in statically > > linked applications requires at runtime the shared libraries from the > > glibc version used for linking > > > > This comes from glib, but the ultimate result is still the same. > > Those are in upstream glib, as you note. We can't fix those (unless we > have the enthusiasm to write patches for upstream glib: last time > we asked, they were not against the idea, but nobody on either side > had the time available to try to write the necessary patches). > But we can and should fix the cases in our own code. > > > Also, what are the symptoms of the breakage? IIUC as long as execution > > does not reach getaddrinfo(), which it in this case should not, because > > it is used only on inet paths, there should not be any issues, right? > > Correct -- if we don't call the function it's fine. But the easiest > way to be sure we don't call the function is to not link it in :-) > Otherwise future code changes could result in a call without our > realizing it. > > Also, mjt's packaging for Debian puts in some stubs for the > offending getwpuid etc functions, which suppress the glib warnings > (this is why he noticed this whereas none of the rest of us did): > > https://sources.debian.org/patches/qemu/1:10.0.0%2Bds-2/static-linux-user-stubs.diff/ Oh interesting, I was conincidentally wondering if adding dummy stubs might suppress this. How about we pull that change upstream, and expand it to getaddrinfo too ? I like the stubs much more than artificially splitting up the source files With regards, Daniel
On Wed, 21 May 2025 at 16:38, Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Wed, May 21, 2025 at 04:34:24PM +0100, Peter Maydell wrote: > > Also, mjt's packaging for Debian puts in some stubs for the > > offending getwpuid etc functions, which suppress the glib warnings > > (this is why he noticed this whereas none of the rest of us did): > > > > https://sources.debian.org/patches/qemu/1:10.0.0%2Bds-2/static-linux-user-stubs.diff/ > > Oh interesting, I was conincidentally wondering if adding dummy > stubs might suppress this. If we do I think that g_assert_not_reached() is probably better than returning -1 etc. > How about we pull that change upstream, and expand it to getaddrinfo > too ? I like the stubs much more than artificially splitting up > the source files The stub is still effectively a runtime assert. Splitting the source file gives us a compile time guarantee that we don't call the function, so I prefer that. thanks -- PMM
diff --git a/gdbstub/user.c b/gdbstub/user.c index fd29d595f4..8225b70280 100644 --- a/gdbstub/user.c +++ b/gdbstub/user.c @@ -315,12 +315,10 @@ static bool gdb_accept_socket(int gdb_fd) return true; } -static int gdbserver_open_socket(const char *path) +static int gdbserver_open_socket(const char *path, Error **errp) { g_autoptr(GString) buf = g_string_new(""); - struct sockaddr_un sockaddr = {}; char *pid_placeholder; - int fd, ret; pid_placeholder = strstr(path, "%d"); if (pid_placeholder != NULL) { @@ -330,28 +328,7 @@ static int gdbserver_open_socket(const char *path) path = buf->str; } - fd = socket(AF_UNIX, SOCK_STREAM, 0); - if (fd < 0) { - perror("create socket"); - return -1; - } - - sockaddr.sun_family = AF_UNIX; - pstrcpy(sockaddr.sun_path, sizeof(sockaddr.sun_path) - 1, path); - ret = bind(fd, (struct sockaddr *)&sockaddr, sizeof(sockaddr)); - if (ret < 0) { - perror("bind socket"); - close(fd); - return -1; - } - ret = listen(fd, 1); - if (ret < 0) { - perror("listen socket"); - close(fd); - return -1; - } - - return fd; + return unix_listen(path, errp); } static bool gdb_accept_tcp(int gdb_fd) @@ -424,7 +401,7 @@ bool gdbserver_start(const char *port_or_path, Error **errp) if (port > 0) { gdb_fd = gdbserver_open_port(port, errp); } else { - gdb_fd = gdbserver_open_socket(port_or_path); + gdb_fd = gdbserver_open_socket(port_or_path, errp); } if (gdb_fd < 0) { diff --git a/stubs/monitor-fd.c b/stubs/monitor-fd.c new file mode 100644 index 0000000000..9bb6749885 --- /dev/null +++ b/stubs/monitor-fd.c @@ -0,0 +1,9 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#include "qemu/osdep.h" +#include "monitor/monitor.h" + +int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp) +{ + abort(); +} diff --git a/stubs/meson.build b/stubs/meson.build index a8b3aeb564..b0fee37e05 100644 --- a/stubs/meson.build +++ b/stubs/meson.build @@ -61,6 +61,8 @@ if have_user if not have_system stub_ss.add(files('qdev.c')) endif + + stub_ss.add(files('monitor-fd.c')) endif if have_system diff --git a/util/meson.build b/util/meson.build index 5d8bef9891..780b5977a8 100644 --- a/util/meson.build +++ b/util/meson.build @@ -84,6 +84,8 @@ if have_block or have_ga util_ss.add(files('qemu-coroutine.c', 'qemu-coroutine-lock.c', 'qemu-coroutine-io.c')) util_ss.add(files(f'coroutine-@coroutine_backend@.c')) util_ss.add(files('thread-pool.c', 'qemu-timer.c')) +endif +if have_block or have_ga or have_user util_ss.add(files('qemu-sockets.c')) endif if have_block