Re: [Qemu-block] [Qemu-devel] [PATCH 01/15] sheepdog: Defuse time bomb in sd_open() error handling

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 03/02/2017 03:43 PM, Markus Armbruster wrote:
>> When qemu_opts_absorb_qdict() fails, sd_open() closes stdin, because
>> sd->fd is still zero.  Fortunately, qemu_opts_absorb_qdict() can't
>> fail, because:
>> 
>> 1. it only fails when qemu_opt_parse() fails, and
>> 2. the only member of runtime_opts.desc[] is a QEMU_OPT_STRING, and
>> 3. qemu_opt_parse() can't fail for QEMU_OPT_STRING.
>> 
>> Defuse this ticking time bomb by jumping behind the file descriptor
>> cleanup on error.
>> 
>> Also do that for the error paths where sd->fd is still -1.  The file
>> descriptor cleanup happens to do nothing then, but let's not rely on
>> that here.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  block/sheepdog.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>> 
>
>>      s->fd = get_sheep_fd(s, errp);
>>      if (s->fd < 0) {
>>          ret = s->fd;
>> -        goto out;
>> +        goto out_no_fd;
>>      }
>
> Thanks to this change...
>
>>  
>>      ret = find_vdi_name(s, vdi, snapid, tag, &vid, true, errp);
>> @@ -1472,6 +1472,7 @@ out:
>>      if (s->fd >= 0) {
>
> ...this 'if' is now always true, and you can unconditionally call
> closesocket().

Yup.

close(-1) is just fine anyway.

> For that matter, the 'out:' label is a bit of a misnomer, since it is
> only reached on errors.
>
>>          closesocket(s->fd);
>>      }
>> +out_no_fd:
>>      qemu_opts_del(opts);
>>      g_free(buf);
>>      return ret;
>> 
>
> The cleanup is correct, but looks like it can be more extensive.

I'll have another look at it.