From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:32846) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xle7A-0002Ed-Ug for qemu-devel@nongnu.org; Tue, 04 Nov 2014 08:25:45 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xle76-00061Y-Mp for qemu-devel@nongnu.org; Tue, 04 Nov 2014 08:25:40 -0500 References: <1415098223-32404-1-git-send-email-zhang.zhanghailiang@huawei.com> <1415098223-32404-2-git-send-email-zhang.zhanghailiang@huawei.com> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <1415098223-32404-2-git-send-email-zhang.zhanghailiang@huawei.com> Date: Tue, 04 Nov 2014 13:25:31 +0000 Message-ID: <874mufumgk.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v3 1/5] qemu-char: fix parameter check in some qemu_chr_parse_* functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: zhanghailiang Cc: qemu-devel@nongnu.org, qemu-trivial@nongnu.org, mjt@tls.msk.ru, peter.huangpeng@huawei.com, armbru@redhat.com, kraxel@redhat.com zhanghailiang writes: > For some qemu_chr_parse_* functions, we just check whether the parameter > is NULL or not, but do not check if it is empty. > > For example: > qemu-system-x86_64 -chardev pipe,id=id,path= > It will pass the check of NULL but will not find the error until > trying to open it, while essentially missing and empty parameter > is the same thing. > > So check the parameters for emptiness too, and avoid emptiness > check at open time. > > Signed-off-by: zhanghailiang > Signed-off-by: Michael Tokarev > --- > qemu-char.c | 15 +++++---------- > 1 file changed, 5 insertions(+), 10 deletions(-) > > diff --git a/qemu-char.c b/qemu-char.c > index bd0709b..a09bbf6 100644 > --- a/qemu-char.c > +++ b/qemu-char.c > @@ -1084,11 +1084,6 @@ static CharDriverState *qemu_chr_open_pipe(ChardevHostdev *opts) > char filename_out[CHR_MAX_FILENAME_SIZE]; > const char *filename = opts->device; > > - if (filename == NULL) { > - fprintf(stderr, "chardev: pipe: no filename given\n"); > - return NULL; > - } > - You seem to have dropped a check here, are you sure all avenues into this code have validated filename? What if a new function gets added? At a minimum I'd replace it with a g_assert(filename) to make the calling contract clear. > snprintf(filename_in, CHR_MAX_FILENAME_SIZE, "%s.in", filename); > snprintf(filename_out, CHR_MAX_FILENAME_SIZE, "%s.out", > filename); We'll probably end up with "(null).in" as the filename which may be exploitation vector. > TFR(fd_in = qemu_open(filename_in, O_RDWR | O_BINARY)); > @@ -3419,7 +3414,7 @@ static void qemu_chr_parse_file_out(QemuOpts *opts, ChardevBackend *backend, > { > const char *path = qemu_opt_get(opts, "path"); > > - if (path == NULL) { > + if (path == NULL || !path[0]) { > error_setg(errp, "chardev: file: no filename given"); > return; > } > @@ -3453,7 +3448,7 @@ static void qemu_chr_parse_parallel(QemuOpts *opts, ChardevBackend *backend, > { > const char *device = qemu_opt_get(opts, "path"); > > - if (device == NULL) { > + if (device == NULL || !device[0]) { > error_setg(errp, "chardev: parallel: no device path given"); > return; > } > @@ -3466,7 +3461,7 @@ static void qemu_chr_parse_pipe(QemuOpts *opts, ChardevBackend *backend, > { > const char *device = qemu_opt_get(opts, "path"); > > - if (device == NULL) { > + if (device == NULL || !device[0]) { > error_setg(errp, "chardev: pipe: no device path given"); > return; > } > @@ -3515,11 +3510,11 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend, > SocketAddress *addr; > > if (!path) { > - if (!host) { > + if (!host || !host[0]) { > error_setg(errp, "chardev: socket: no host given"); > return; > } > - if (!port) { > + if (!port || !port[0]) { > error_setg(errp, "chardev: socket: no port given"); > return; > } All this boilerplate checking makes me think that either the qemu_opt machinery should be ensuring we get a valid option string? -- Alex Bennée