zhanghailiang <zhang.zhanghaili...@huawei.com> 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 <zhang.zhanghaili...@huawei.com> > Signed-off-by: Michael Tokarev <m...@tls.msk.ru> > --- > 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