Re: [Qemu-devel] [PATCH v3 1/5] qemu-char: fix parameter check in some qemu_chr_parse_* functions

["Alex Bennée" <alex.bennee@linaro.org>]

zhanghailiang <zhang.zhanghailiang@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.zhanghailiang@huawei.com>
> Signed-off-by: Michael Tokarev <mjt@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