On 5/6/21 4:48 PM, Warner Losh wrote:
>
>
> On Thu, May 6, 2021 at 8:21 AM Peter Maydell <peter.maydell@linaro.org
> <mailto:peter.maydell@linaro.org>> wrote:
>
> On Thu, 6 May 2021 at 15:17, Warner Losh <imp@bsdimp.com
> <mailto:imp@bsdimp.com>> wrote:
> >
> >
> >
> > On Thu, May 6, 2021, 7:38 AM Philippe Mathieu-Daudé
> <philmd@redhat.com <mailto:philmd@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é <philmd@redhat.com
> <mailto:philmd@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 :)...