Eric Blake <ebl...@redhat.com> 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 <arm...@redhat.com>
>> ---
>> 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.
