On Thu, May 6, 2021 at 8:57 AM Philippe Mathieu-Daudé <phi...@redhat.com> wrote:
> On 5/6/21 4:48 PM, Warner Losh wrote: > > > > > > On Thu, May 6, 2021 at 8:21 AM Peter Maydell <peter.mayd...@linaro.org > > <mailto:peter.mayd...@linaro.org>> wrote: > > > > On Thu, 6 May 2021 at 15:17, Warner Losh <i...@bsdimp.com > > <mailto:i...@bsdimp.com>> wrote: > > > > > > > > > > > > On Thu, May 6, 2021, 7:38 AM Philippe Mathieu-Daudé > > <phi...@redhat.com <mailto:phi...@redhat.com>> wrote: > > >> > > >> The ALLOCA(3) man-page mentions its "use is discouraged". > > >> > > >> Replace it by a g_new() call. > > >> > > >> Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com > > <mailto:phi...@redhat.com>> > > >> --- > > >> bsd-user/syscall.c | 3 +-- > > >> 1 file changed, 1 insertion(+), 2 deletions(-) > > >> > > >> diff --git a/bsd-user/syscall.c b/bsd-user/syscall.c > > >> index 4abff796c76..dbee0385ceb 100644 > > >> --- a/bsd-user/syscall.c > > >> +++ b/bsd-user/syscall.c > > >> @@ -355,9 +355,8 @@ abi_long do_freebsd_syscall(void *cpu_env, > > int num, abi_long arg1, > > >> case TARGET_FREEBSD_NR_writev: > > >> { > > >> int count = arg3; > > >> - struct iovec *vec; > > >> + g_autofree struct iovec *vec = g_new(struct iovec, > > count); > > > > > > > > > Where is this freed? > > > > g_autofree, so it gets freed when it goes out of scope. > > > https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#g-autofree > > < > https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#g-autofree > > > > > > > > Ah. I'd missed that feature and annotation... Too many years seeing > > patches like > > this in other projects where a feature like this wasn't there to save > > the day... > > > > This means you can ignore my other message as equally misinformed. > > This also shows my commit description is poor. > > > > Also, alloca just moves a stack pointer, where malloc has complex > > interactions. Are you sure that's a safe change here? > > > > alloca()ing something with size determined by the guest is > > definitely not safe :-) malloc as part of "handle this syscall" > > is pretty common, at least in linux-user. > > > > > > Well, since this is userland emulation, the normal process boundaries > > will fix that. allocating from > > the heap is little different..., so while unsafe, it's the domain of > > $SOMEONE_ELSE to enforce > > the safety. linux-user has many similar usages for both malloc and > > alloca, and it's ok for the > > same reason. > > > > Doing a quick grep on my blitz[*] branch in the bsd-user fork shows that > > alloca is used almost > > exclusively there. There's 24 calls to alloca in the bsd-user code. > > There's almost no malloc > > calls (7) in that at all outside the image loader: all but one of them > > are confined to sysctl > > emulation with the last one used to keep track of thread state in new > > threads... > > linux-user has a similar profile (20 alloca's and 14 mallocs outside the > > loader), > > but with more mallocs in other paths than just sysctl (which isn't > present). > > > > I had no plans on migrating to anything else... > > I considered excluding user emulation (both Linux/BSD) by enabling > CFLAGS=-Walloca for everything except user-mode, but Meson doesn't > support adding flags to a specific source set: > https://github.com/mesonbuild/meson/issues/1367#issuecomment-277929767 > > Q: Is it possible to add a flag to a specific file? I have some > generated code that's freaking the compiler out and I don't > feel like mucking with the generator. > > A: We don't support per-file compiler flags by design. It interacts > very poorly with other parts, especially precompiled headers. > The real solution is to fix the generator to not produce garbage. > Obviously this is often not possible so the solution to that is, > as mentioned above, build a static library with the specific > compiler args. This has the added benefit that it makes this > workaround explicit and visible rather than hiding things in > random locations. > > Then Paolo suggested to convert all alloca() calls instead. > I'm having trouble understanding how the emulated system calls can still be async safe if that's done. I might be missing something in the CPU emulation that would clear up the concern, though. How threads interact with each-other in emulation is an area I'm still learning. Warner P.S. Of course, no matter what you do, the current in-tree bsd-user dumps core right away, so it's hard to break it worse than it already is broken :)...
