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

[Laurent Vivier]

On 29/04/2019 16:46, Marc-André Lureau wrote:
> 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.
> 

ok, so:

Reviewed-by: Laurent Vivier <address@hidden>