[PATCH v2 0/4] vhost-user: avoid g_return_val_if() in get/set_config()

[PATCH v2 0/4] vhost-user: avoid g_return_val_if() in get/set_config()

[Stefan Hajnoczi]

v2:
 * Print errors [Marc-André]

Markus Armbruster pointed out that g_return_val_if() is meant for programming
errors. It must not be used for input validation since it can be compiled out.
Use explicit if statements instead.

This patch series converts vhost-user device backends that use
g_return_val_if() in get/set_config().

Stefan Hajnoczi (4):
  contrib/vhost-user-blk: avoid g_return_val_if() input validation
  contrib/vhost-user-gpu: avoid g_return_val_if() input validation
  contrib/vhost-user-input: avoid g_return_val_if() input validation
  block/export: avoid g_return_val_if() input validation

 block/export/vhost-user-blk-server.c    | 6 +++++-
 contrib/vhost-user-blk/vhost-user-blk.c | 6 +++++-
 contrib/vhost-user-gpu/vhost-user-gpu.c | 6 +++++-
 contrib/vhost-user-input/main.c         | 6 +++++-
 4 files changed, 20 insertions(+), 4 deletions(-)

-- 
2.28.0

[PATCH v2 1/4] contrib/vhost-user-blk: avoid g_return_val_if() input validation

[Stefan Hajnoczi]

Do not validate input with g_return_val_if(). This API is intended for
checking programming errors and is compiled out with -DG_DISABLE_CHECKS.

Use an explicit if statement for input validation so it cannot
accidentally be compiled out.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 contrib/vhost-user-blk/vhost-user-blk.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/contrib/vhost-user-blk/vhost-user-blk.c b/contrib/vhost-user-blk/vhost-user-blk.c
index dc981bf945..60e3c9ed37 100644
--- a/contrib/vhost-user-blk/vhost-user-blk.c
+++ b/contrib/vhost-user-blk/vhost-user-blk.c
@@ -404,7 +404,11 @@ vub_get_config(VuDev *vu_dev, uint8_t *config, uint32_t len)
     VugDev *gdev;
     VubDev *vdev_blk;
 
-    g_return_val_if_fail(len <= sizeof(struct virtio_blk_config), -1);
+    if (len > sizeof(struct virtio_blk_config)) {
+        fprintf(stderr, "Invalid get_config len %u, expected <= %zu\n",
+                len, sizeof(struct virtio_blk_config));
+        return -1;
+    }
 
     gdev = container_of(vu_dev, VugDev, parent);
     vdev_blk = container_of(gdev, VubDev, parent);
-- 
2.28.0

[PATCH v2 2/4] contrib/vhost-user-gpu: avoid g_return_val_if() input validation

[Stefan Hajnoczi]

Do not validate input with g_return_val_if(). This API is intended for
checking programming errors and is compiled out with -DG_DISABLE_CHECKS.

Use an explicit if statement for input validation so it cannot
accidentally be compiled out.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 contrib/vhost-user-gpu/vhost-user-gpu.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c b/contrib/vhost-user-gpu/vhost-user-gpu.c
index a019d0a9ac..534bad24d1 100644
--- a/contrib/vhost-user-gpu/vhost-user-gpu.c
+++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
@@ -1044,7 +1044,11 @@ vg_get_config(VuDev *dev, uint8_t *config, uint32_t len)
 {
     VuGpu *g = container_of(dev, VuGpu, dev.parent);
 
-    g_return_val_if_fail(len <= sizeof(struct virtio_gpu_config), -1);
+    if (len > sizeof(struct virtio_gpu_config)) {
+        g_critical("%s: len %u is larger than %zu",
+                   __func__, len, sizeof(struct virtio_gpu_config));
+        return -1;
+    }
 
     if (opt_virgl) {
         g->virtio_config.num_capsets = vg_virgl_get_num_capsets();
-- 
2.28.0

[PATCH v2 3/4] contrib/vhost-user-input: avoid g_return_val_if() input validation

[Stefan Hajnoczi]

Do not validate input with g_return_val_if(). This API is intended for
checking programming errors and is compiled out with -DG_DISABLE_CHECKS.

Use an explicit if statement for input validation so it cannot
accidentally be compiled out.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 contrib/vhost-user-input/main.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/contrib/vhost-user-input/main.c b/contrib/vhost-user-input/main.c
index 6020c6f33a..1d79c61200 100644
--- a/contrib/vhost-user-input/main.c
+++ b/contrib/vhost-user-input/main.c
@@ -212,7 +212,11 @@ static int vi_get_config(VuDev *dev, uint8_t *config, uint32_t len)
 {
     VuInput *vi = container_of(dev, VuInput, dev.parent);
 
-    g_return_val_if_fail(len <= sizeof(*vi->sel_config), -1);
+    if (len > sizeof(*vi->sel_config)) {
+        g_critical("%s: len %u is larger than %zu",
+                   __func__, len, sizeof(*vi->sel_config));
+        return -1;
+    }
 
     if (vi->sel_config) {
         memcpy(config, vi->sel_config, len);
-- 
2.28.0

[PATCH v2 4/4] block/export: avoid g_return_val_if() input validation

[Stefan Hajnoczi]

Do not validate input with g_return_val_if(). This API is intended for
checking programming errors and is compiled out with -DG_DISABLE_CHECKS.

Use an explicit if statement for input validation so it cannot
accidentally be compiled out.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/export/vhost-user-blk-server.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c
index 62672d1cb9..bccbc98d57 100644
--- a/block/export/vhost-user-blk-server.c
+++ b/block/export/vhost-user-blk-server.c
@@ -267,7 +267,11 @@ vu_blk_get_config(VuDev *vu_dev, uint8_t *config, uint32_t len)
     VuServer *server = container_of(vu_dev, VuServer, vu_dev);
     VuBlkExport *vexp = container_of(server, VuBlkExport, vu_server);
 
-    g_return_val_if_fail(len <= sizeof(struct virtio_blk_config), -1);
+    if (len > sizeof(struct virtio_blk_config)) {
+        error_report("Invalid get_config len %u, expected <= %zu",
+                     len, sizeof(struct virtio_blk_config));
+        return -1;
+    }
 
     memcpy(config, &vexp->blkcfg, len);
     return 0;
-- 
2.28.0

Re: [PATCH v2 1/4] contrib/vhost-user-blk: avoid g_return_val_if() input validation

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1464 bytes --]

On Wed, Dec 2, 2020 at 7:26 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:

> Do not validate input with g_return_val_if(). This API is intended for
> checking programming errors and is compiled out with -DG_DISABLE_CHECKS.
>
> Use an explicit if statement for input validation so it cannot
> accidentally be compiled out.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

---
>  contrib/vhost-user-blk/vhost-user-blk.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/vhost-user-blk/vhost-user-blk.c
> b/contrib/vhost-user-blk/vhost-user-blk.c
> index dc981bf945..60e3c9ed37 100644
> --- a/contrib/vhost-user-blk/vhost-user-blk.c
> +++ b/contrib/vhost-user-blk/vhost-user-blk.c
> @@ -404,7 +404,11 @@ vub_get_config(VuDev *vu_dev, uint8_t *config,
> uint32_t len)
>      VugDev *gdev;
>      VubDev *vdev_blk;
>
> -    g_return_val_if_fail(len <= sizeof(struct virtio_blk_config), -1);
> +    if (len > sizeof(struct virtio_blk_config)) {
> +        fprintf(stderr, "Invalid get_config len %u, expected <= %zu\n",
> +                len, sizeof(struct virtio_blk_config));
> +        return -1;
> +    }
>
>      gdev = container_of(vu_dev, VugDev, parent);
>      vdev_blk = container_of(gdev, VubDev, parent);
> --
> 2.28.0
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 2306 bytes --]

Re: [PATCH v2 2/4] contrib/vhost-user-gpu: avoid g_return_val_if() input validation

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1542 bytes --]

On Wed, Dec 2, 2020 at 7:27 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:

> Do not validate input with g_return_val_if(). This API is intended for
> checking programming errors and is compiled out with -DG_DISABLE_CHECKS.
>
> Use an explicit if statement for input validation so it cannot
> accidentally be compiled out.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  contrib/vhost-user-gpu/vhost-user-gpu.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c
> b/contrib/vhost-user-gpu/vhost-user-gpu.c
> index a019d0a9ac..534bad24d1 100644
> --- a/contrib/vhost-user-gpu/vhost-user-gpu.c
> +++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
> @@ -1044,7 +1044,11 @@ vg_get_config(VuDev *dev, uint8_t *config, uint32_t
> len)
>  {
>      VuGpu *g = container_of(dev, VuGpu, dev.parent);
>
> -    g_return_val_if_fail(len <= sizeof(struct virtio_gpu_config), -1);
> +    if (len > sizeof(struct virtio_gpu_config)) {
> +        g_critical("%s: len %u is larger than %zu",
> +                   __func__, len, sizeof(struct virtio_gpu_config));
>

g_critical() already has __FILE__ __LINE__ and G_STRFUNC.

otherwise looks good:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

+        return -1;
> +    }
>
>      if (opt_virgl) {
>          g->virtio_config.num_capsets = vg_virgl_get_num_capsets();
> --
> 2.28.0
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 2415 bytes --]

Re: [PATCH v2 3/4] contrib/vhost-user-input: avoid g_return_val_if() input validation

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1478 bytes --]

On Wed, Dec 2, 2020 at 7:27 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:

> Do not validate input with g_return_val_if(). This API is intended for
> checking programming errors and is compiled out with -DG_DISABLE_CHECKS.
>
> Use an explicit if statement for input validation so it cannot
> accidentally be compiled out.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  contrib/vhost-user-input/main.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/vhost-user-input/main.c
> b/contrib/vhost-user-input/main.c
> index 6020c6f33a..1d79c61200 100644
> --- a/contrib/vhost-user-input/main.c
> +++ b/contrib/vhost-user-input/main.c
> @@ -212,7 +212,11 @@ static int vi_get_config(VuDev *dev, uint8_t *config,
> uint32_t len)
>  {
>      VuInput *vi = container_of(dev, VuInput, dev.parent);
>
> -    g_return_val_if_fail(len <= sizeof(*vi->sel_config), -1);
> +    if (len > sizeof(*vi->sel_config)) {
> +        g_critical("%s: len %u is larger than %zu",
> +                   __func__, len, sizeof(*vi->sel_config));
> +        return -1;
>

g_critical() already has __FILE__ __LINE__ and G_STRFUNC.

otherwise looks good:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>



> +    }
>
>      if (vi->sel_config) {
>          memcpy(config, vi->sel_config, len);
> --
> 2.28.0
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 2370 bytes --]

Re: [PATCH v2 4/4] block/export: avoid g_return_val_if() input validation

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1499 bytes --]

On Wed, Dec 2, 2020 at 7:27 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:

> Do not validate input with g_return_val_if(). This API is intended for
> checking programming errors and is compiled out with -DG_DISABLE_CHECKS.
>
> Use an explicit if statement for input validation so it cannot
> accidentally be compiled out.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

---
>  block/export/vhost-user-blk-server.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/block/export/vhost-user-blk-server.c
> b/block/export/vhost-user-blk-server.c
> index 62672d1cb9..bccbc98d57 100644
> --- a/block/export/vhost-user-blk-server.c
> +++ b/block/export/vhost-user-blk-server.c
> @@ -267,7 +267,11 @@ vu_blk_get_config(VuDev *vu_dev, uint8_t *config,
> uint32_t len)
>      VuServer *server = container_of(vu_dev, VuServer, vu_dev);
>      VuBlkExport *vexp = container_of(server, VuBlkExport, vu_server);
>
> -    g_return_val_if_fail(len <= sizeof(struct virtio_blk_config), -1);
> +    if (len > sizeof(struct virtio_blk_config)) {
> +        error_report("Invalid get_config len %u, expected <= %zu",
> +                     len, sizeof(struct virtio_blk_config));
> +        return -1;
> +    }
>
>      memcpy(config, &vexp->blkcfg, len);
>      return 0;
> --
> 2.28.0
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 2351 bytes --]

Re: [PATCH v2 1/4] contrib/vhost-user-blk: avoid g_return_val_if() input validation

[Raphael Norwitz]

On Wed, Dec 2, 2020 at 10:27 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> Do not validate input with g_return_val_if(). This API is intended for
> checking programming errors and is compiled out with -DG_DISABLE_CHECKS.
>
> Use an explicit if statement for input validation so it cannot
> accidentally be compiled out.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>

> ---
>  contrib/vhost-user-blk/vhost-user-blk.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/vhost-user-blk/vhost-user-blk.c b/contrib/vhost-user-blk/vhost-user-blk.c
> index dc981bf945..60e3c9ed37 100644
> --- a/contrib/vhost-user-blk/vhost-user-blk.c
> +++ b/contrib/vhost-user-blk/vhost-user-blk.c
> @@ -404,7 +404,11 @@ vub_get_config(VuDev *vu_dev, uint8_t *config, uint32_t len)
>      VugDev *gdev;
>      VubDev *vdev_blk;
>
> -    g_return_val_if_fail(len <= sizeof(struct virtio_blk_config), -1);
> +    if (len > sizeof(struct virtio_blk_config)) {
> +        fprintf(stderr, "Invalid get_config len %u, expected <= %zu\n",
> +                len, sizeof(struct virtio_blk_config));
> +        return -1;
> +    }
>
>      gdev = container_of(vu_dev, VugDev, parent);
>      vdev_blk = container_of(gdev, VubDev, parent);
> --
> 2.28.0
>

Re: [PATCH v2 1/4] contrib/vhost-user-blk: avoid g_return_val_if() input validation

[Philippe Mathieu-Daudé]

On 12/2/20 4:26 PM, Stefan Hajnoczi wrote:
> Do not validate input with g_return_val_if(). This API is intended for
> checking programming errors and is compiled out with -DG_DISABLE_CHECKS.
> 
> Use an explicit if statement for input validation so it cannot
> accidentally be compiled out.
> 
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  contrib/vhost-user-blk/vhost-user-blk.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v2 4/4] block/export: avoid g_return_val_if() input validation

[Philippe Mathieu-Daudé]

On 12/2/20 4:26 PM, Stefan Hajnoczi wrote:
> Do not validate input with g_return_val_if(). This API is intended for
> checking programming errors and is compiled out with -DG_DISABLE_CHECKS.
> 
> Use an explicit if statement for input validation so it cannot
> accidentally be compiled out.
> 
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/export/vhost-user-blk-server.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v2 2/4] contrib/vhost-user-gpu: avoid g_return_val_if() input validation

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1544 bytes --]

On Wed, Dec 02, 2020 at 07:50:51PM +0400, Marc-André Lureau wrote:
> On Wed, Dec 2, 2020 at 7:27 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> > Do not validate input with g_return_val_if(). This API is intended for
> > checking programming errors and is compiled out with -DG_DISABLE_CHECKS.
> >
> > Use an explicit if statement for input validation so it cannot
> > accidentally be compiled out.
> >
> > Suggested-by: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  contrib/vhost-user-gpu/vhost-user-gpu.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c
> > b/contrib/vhost-user-gpu/vhost-user-gpu.c
> > index a019d0a9ac..534bad24d1 100644
> > --- a/contrib/vhost-user-gpu/vhost-user-gpu.c
> > +++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
> > @@ -1044,7 +1044,11 @@ vg_get_config(VuDev *dev, uint8_t *config, uint32_t
> > len)
> >  {
> >      VuGpu *g = container_of(dev, VuGpu, dev.parent);
> >
> > -    g_return_val_if_fail(len <= sizeof(struct virtio_gpu_config), -1);
> > +    if (len > sizeof(struct virtio_gpu_config)) {
> > +        g_critical("%s: len %u is larger than %zu",
> > +                   __func__, len, sizeof(struct virtio_gpu_config));
> >
> 
> g_critical() already has __FILE__ __LINE__ and G_STRFUNC.

I did this for consistency with the logging in this source file. The
other g_critical() calls in the file also print __func__.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 3/4] contrib/vhost-user-input: avoid g_return_val_if() input validation

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1398 bytes --]

On Wed, Dec 02, 2020 at 07:51:49PM +0400, Marc-André Lureau wrote:
> On Wed, Dec 2, 2020 at 7:27 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> > Do not validate input with g_return_val_if(). This API is intended for
> > checking programming errors and is compiled out with -DG_DISABLE_CHECKS.
> >
> > Use an explicit if statement for input validation so it cannot
> > accidentally be compiled out.
> >
> > Suggested-by: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  contrib/vhost-user-input/main.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/contrib/vhost-user-input/main.c
> > b/contrib/vhost-user-input/main.c
> > index 6020c6f33a..1d79c61200 100644
> > --- a/contrib/vhost-user-input/main.c
> > +++ b/contrib/vhost-user-input/main.c
> > @@ -212,7 +212,11 @@ static int vi_get_config(VuDev *dev, uint8_t *config,
> > uint32_t len)
> >  {
> >      VuInput *vi = container_of(dev, VuInput, dev.parent);
> >
> > -    g_return_val_if_fail(len <= sizeof(*vi->sel_config), -1);
> > +    if (len > sizeof(*vi->sel_config)) {
> > +        g_critical("%s: len %u is larger than %zu",
> > +                   __func__, len, sizeof(*vi->sel_config));
> > +        return -1;
> >
> 
> g_critical() already has __FILE__ __LINE__ and G_STRFUNC.

Will fix.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/4] contrib/vhost-user-gpu: avoid g_return_val_if() input validation

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1769 bytes --]

On Thu, Dec 3, 2020 at 1:52 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:

> On Wed, Dec 02, 2020 at 07:50:51PM +0400, Marc-André Lureau wrote:
> > On Wed, Dec 2, 2020 at 7:27 PM Stefan Hajnoczi <stefanha@redhat.com>
> wrote:
> >
> > > Do not validate input with g_return_val_if(). This API is intended for
> > > checking programming errors and is compiled out with
> -DG_DISABLE_CHECKS.
> > >
> > > Use an explicit if statement for input validation so it cannot
> > > accidentally be compiled out.
> > >
> > > Suggested-by: Markus Armbruster <armbru@redhat.com>
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> > >  contrib/vhost-user-gpu/vhost-user-gpu.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c
> > > b/contrib/vhost-user-gpu/vhost-user-gpu.c
> > > index a019d0a9ac..534bad24d1 100644
> > > --- a/contrib/vhost-user-gpu/vhost-user-gpu.c
> > > +++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
> > > @@ -1044,7 +1044,11 @@ vg_get_config(VuDev *dev, uint8_t *config,
> uint32_t
> > > len)
> > >  {
> > >      VuGpu *g = container_of(dev, VuGpu, dev.parent);
> > >
> > > -    g_return_val_if_fail(len <= sizeof(struct virtio_gpu_config), -1);
> > > +    if (len > sizeof(struct virtio_gpu_config)) {
> > > +        g_critical("%s: len %u is larger than %zu",
> > > +                   __func__, len, sizeof(struct virtio_gpu_config));
> > >
> >
> > g_critical() already has __FILE__ __LINE__ and G_STRFUNC.
>
> I did this for consistency with the logging in this source file. The
> other g_critical() calls in the file also print __func__.
>
>
>
I see, nevermind then. I gave rb anyway


-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 2629 bytes --]

Re: [PATCH v2 2/4] contrib/vhost-user-gpu: avoid g_return_val_if() input validation

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2224 bytes --]

On Thu, Dec 03, 2020 at 02:26:08PM +0400, Marc-André Lureau wrote:
> On Thu, Dec 3, 2020 at 1:52 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> > On Wed, Dec 02, 2020 at 07:50:51PM +0400, Marc-André Lureau wrote:
> > > On Wed, Dec 2, 2020 at 7:27 PM Stefan Hajnoczi <stefanha@redhat.com>
> > wrote:
> > >
> > > > Do not validate input with g_return_val_if(). This API is intended for
> > > > checking programming errors and is compiled out with
> > -DG_DISABLE_CHECKS.
> > > >
> > > > Use an explicit if statement for input validation so it cannot
> > > > accidentally be compiled out.
> > > >
> > > > Suggested-by: Markus Armbruster <armbru@redhat.com>
> > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > > ---
> > > >  contrib/vhost-user-gpu/vhost-user-gpu.c | 6 +++++-
> > > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c
> > > > b/contrib/vhost-user-gpu/vhost-user-gpu.c
> > > > index a019d0a9ac..534bad24d1 100644
> > > > --- a/contrib/vhost-user-gpu/vhost-user-gpu.c
> > > > +++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
> > > > @@ -1044,7 +1044,11 @@ vg_get_config(VuDev *dev, uint8_t *config,
> > uint32_t
> > > > len)
> > > >  {
> > > >      VuGpu *g = container_of(dev, VuGpu, dev.parent);
> > > >
> > > > -    g_return_val_if_fail(len <= sizeof(struct virtio_gpu_config), -1);
> > > > +    if (len > sizeof(struct virtio_gpu_config)) {
> > > > +        g_critical("%s: len %u is larger than %zu",
> > > > +                   __func__, len, sizeof(struct virtio_gpu_config));
> > > >
> > >
> > > g_critical() already has __FILE__ __LINE__ and G_STRFUNC.
> >
> > I did this for consistency with the logging in this source file. The
> > other g_critical() calls in the file also print __func__.
> >
> >
> >
> I see, nevermind then. I gave rb anyway

Thanks! I checked now and don't see the function name printed by
g_critical() even though G_STRFUNC is captured in the header file:

** (process:693258): CRITICAL **: 11:30:18.210: test

Maybe only custom log handlers can display the function name?

So it seems the explicit __func__ approach is okay-ish :).

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/4] vhost-user: avoid g_return_val_if() in get/set_config()

[Stefano Garzarella]

On Wed, Dec 02, 2020 at 03:26:07PM +0000, Stefan Hajnoczi wrote:
>v2:
> * Print errors [Marc-André]
>
>Markus Armbruster pointed out that g_return_val_if() is meant for programming
>errors. It must not be used for input validation since it can be compiled out.
>Use explicit if statements instead.
>
>This patch series converts vhost-user device backends that use
>g_return_val_if() in get/set_config().
>
>Stefan Hajnoczi (4):
>  contrib/vhost-user-blk: avoid g_return_val_if() input validation
>  contrib/vhost-user-gpu: avoid g_return_val_if() input validation
>  contrib/vhost-user-input: avoid g_return_val_if() input validation
>  block/export: avoid g_return_val_if() input validation
>
> block/export/vhost-user-blk-server.c    | 6 +++++-
> contrib/vhost-user-blk/vhost-user-blk.c | 6 +++++-
> contrib/vhost-user-gpu/vhost-user-gpu.c | 6 +++++-
> contrib/vhost-user-input/main.c         | 6 +++++-
> 4 files changed, 20 insertions(+), 4 deletions(-)
>
>-- 
>2.28.0
>

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Re: [PATCH v2 0/4] vhost-user: avoid g_return_val_if() in get/set_config()

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1382 bytes --]

Hi Michael,

On Thu, Dec 3, 2020 at 5:41 PM Stefano Garzarella <sgarzare@redhat.com>
wrote:

> On Wed, Dec 02, 2020 at 03:26:07PM +0000, Stefan Hajnoczi wrote:
> >v2:
> > * Print errors [Marc-André]
> >
> >Markus Armbruster pointed out that g_return_val_if() is meant for
> programming
> >errors. It must not be used for input validation since it can be compiled
> out.
> >Use explicit if statements instead.
> >
> >This patch series converts vhost-user device backends that use
> >g_return_val_if() in get/set_config().
> >
> >Stefan Hajnoczi (4):
> >  contrib/vhost-user-blk: avoid g_return_val_if() input validation
> >  contrib/vhost-user-gpu: avoid g_return_val_if() input validation
> >  contrib/vhost-user-input: avoid g_return_val_if() input validation
> >  block/export: avoid g_return_val_if() input validation
> >
> > block/export/vhost-user-blk-server.c    | 6 +++++-
> > contrib/vhost-user-blk/vhost-user-blk.c | 6 +++++-
> > contrib/vhost-user-gpu/vhost-user-gpu.c | 6 +++++-
> > contrib/vhost-user-input/main.c         | 6 +++++-
> > 4 files changed, 20 insertions(+), 4 deletions(-)
> >
> >--
> >2.28.0
> >
>
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>
>
You didn't collect the v2 patch series, with the received reviewed-by. Not
a big deal here, but please be more careful.

thanks

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 1997 bytes --]