| Message ID | 1488491046-2549-2-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <armbru@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(). 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> 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 <armbru@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.
diff --git a/block/sheepdog.c b/block/sheepdog.c index 860ba61..fe15723 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -1392,7 +1392,7 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags, if (local_err) { error_propagate(errp, local_err); ret = -EINVAL; - goto out; + goto out_no_fd; } filename = qemu_opt_get(opts, "filename"); @@ -1412,12 +1412,12 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags, } if (ret < 0) { error_setg(errp, "Can't parse filename"); - goto out; + goto out_no_fd; } s->fd = get_sheep_fd(s, errp); if (s->fd < 0) { ret = s->fd; - goto out; + goto out_no_fd; } ret = find_vdi_name(s, vdi, snapid, tag, &vid, true, errp); @@ -1472,6 +1472,7 @@ out: if (s->fd >= 0) { closesocket(s->fd); } +out_no_fd: qemu_opts_del(opts); g_free(buf); return ret;
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 <armbru@redhat.com> --- block/sheepdog.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)