[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-trivial] [Qemu-devel] [PATCH v3 1/5] qemu-char: fix parameter
From: |
Alex Bennée |
Subject: |
Re: [Qemu-trivial] [Qemu-devel] [PATCH v3 1/5] qemu-char: fix parameter check in some qemu_chr_parse_* functions |
Date: |
Tue, 04 Nov 2014 13:25:31 +0000 |
zhanghailiang <address@hidden> 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 <address@hidden>
> Signed-off-by: Michael Tokarev <address@hidden>
> ---
> 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
[Qemu-trivial] [PATCH v3 2/5] spice-qemu-char: fix parameter checks in qemu_chr_parse_* functions, zhanghailiang, 2014/11/04