Re: [Qemu-devel] [PATCH] vl: fix -sandbox parsing crash when seccomp support is disabled

[Marc-André Lureau]

Hi

On Mon, Apr 29, 2019 at 4:26 PM Laurent Vivier <address@hidden> wrote:
>
> On 29/04/2019 15:47, Marc-André Lureau wrote:
> > $ ./x86_64-softmmu/qemu-system-x86_64 -sandbox off
> > qemu-system-x86_64: -sandbox off: There is no option group 'sandbox'
> > Segmentation fault
> >
> > Commit 5780760f5e ("seccomp: check TSYNC host capability") wrapped one
> > use of the sandbox option group to produce a sensible error, it didn't
> > do the same for another call to qemu_opts_parse_noisily():
> >
> > (gdb) bt
> >     at util/qemu-option.c:829
> >  #0  0x00000000105b36d8 in opts_parse (list=0x0, params=0x3ffffffffab5 
> > "off", permit_abbrev=true, defaults=false, errp=0x3ffffffff080)
> >      at util/qemu-option.c:829
> >  #1  0x00000000105b3b74 in qemu_opts_parse_noisily (list=<optimized out>, 
> > params=<optimized out>, permit_abbrev=<optimized out>) at 
> > util/qemu-option.c:890
> >  #2  0x0000000010024964 in main (argc=<optimized out>, argv=<optimized 
> > out>, envp=<optimized out>) at vl.c:3589
> >
> > Fixes: 5780760f5ea6163939a5dabe7427318b4f07d1a2
> > Cc: address@hidden
> > Cc: address@hidden
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> >  vl.c | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/vl.c b/vl.c
> > index 4019a4387d..5fc4994d3c 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -3866,17 +3866,19 @@ int main(int argc, char **argv, char **envp)
> >                  qtest_log = optarg;
> >                  break;
> >              case QEMU_OPTION_sandbox:
> > -#ifdef CONFIG_SECCOMP
> > -                opts = qemu_opts_parse_noisily(qemu_find_opts("sandbox"),
> > -                                               optarg, true);
> > +                olist = qemu_find_opts("sandbox");
> > +                if (!olist) {
> > +#ifndef CONFIG_SECCOMP
>
> Why do you move the #ifdef? We have two separate error cases here.
> And it seems better no to check for "-sandbox" when seccomp is disabled.

I tried to remove the #ifdef altogether to simplify the code, then
realized the error message could be useful.

I don't think it's a problem to lookup "-sandbox" when seccomp is disabled.

-- 
Marc-André Lureau