Re: [PATCH v2 4/9] bsd-user/syscall: Replace alloca() by g_new()

From: Warner Losh <imp@bsdimp.com>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: kvm-devel <kvm@vger.kernel.org>,
	"Kyle Evans" <kevans@freebsd.org>,
	"QEMU Developers" <qemu-devel@nongnu.org>,
	qemu-arm <qemu-arm@nongnu.org>, qemu-ppc <qemu-ppc@nongnu.org>,
	"Gerd Hoffmann" <kraxel@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>
Subject: Re: [PATCH v2 4/9] bsd-user/syscall: Replace alloca() by g_new()
Date: Thu, 6 May 2021 08:48:37 -0600	[thread overview]
Message-ID: <CANCZdfpXjDECHmZq55zP43g32OVhnfjf9W+ERtPMFeDs2MmvXQ@mail.gmail.com> (raw)
In-Reply-To: <CAFEAcA9VL_h8DdVwWWmOxs=mNWj-DEHQu-U4L6vb_H4cGMZpPA@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2664 bytes --]

On Thu, May 6, 2021 at 8:21 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Thu, 6 May 2021 at 15:17, Warner Losh <imp@bsdimp.com> 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?
>
> g_autofree, so it gets freed when it goes out of scope.
>
> 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.

>
> > 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...

Warner

[-- Attachment #2: Type: text/html, Size: 3924 bytes --]