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

[Eric Blake]

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

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature