On Thu, May 6, 2021 at 8:26 AM Eric Blake <eblake@redhat.com> wrote:

> On 5/6/21 9:16 AM, Warner Losh wrote:
> > On Thu, May 6, 2021, 7:38 AM Philippe Mathieu-Daudé <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>
> >> ---
> >>  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? Also, alloca just moves a stack pointer, where
> malloc
> > has complex interactions. Are you sure that's a safe change here?
>
> It's freed any time the g_autofree variable goes out of scope (that's
> what the g_autofree macro is for).  Yes, the change is safe, although
> you are right that switching to malloc is going to be a bit more
> heavyweight than what alloca used.  What's more, it adds safety: if
> count was under user control, a user could pass a value that could cause
> alloca to allocate more than 4k and accidentally mess up stack guard
> pages, while malloc() uses the heap and therefore cannot cause stack bugs.
>

I'm not sure I understand that argument, since we're not compiling bsd-user
with the stack-smash-protection stuff enabled, so there's no guard pages
involved. The stack can grow quite large and the unmapped page at
the end of the stack would catch any overflows. Since these allocations
are on the top of the stack, they won't overflow into any other frames
and subsequent calls are made with them already in place.

malloc, on the other hand, involves taking out a number of mutexes
and similar things to obtain the memory, which may not necessarily
be safe in all the contexts system calls can be called from. System
calls are, typically, async safe and can be called from signal handlers.
alloca() is async safe, while malloc() is not. So changing the calls
from alloca to malloc makes calls to system calls in signal handlers
unsafe and potentially introducing buggy behavior as a result.

Warner