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

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

[Stefan Hajnoczi]

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    | 4 +++-
 contrib/vhost-user-blk/vhost-user-blk.c | 4 +++-
 contrib/vhost-user-gpu/vhost-user-gpu.c | 4 +++-
 contrib/vhost-user-input/main.c         | 4 +++-
 4 files changed, 12 insertions(+), 4 deletions(-)

-- 
2.28.0

[PATCH 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 | 4 +++-
 1 file changed, 3 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..1540d5e87d 100644
--- a/contrib/vhost-user-blk/vhost-user-blk.c
+++ b/contrib/vhost-user-blk/vhost-user-blk.c
@@ -404,7 +404,9 @@ 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)) {
+        return -1;
+    }
 
     gdev = container_of(vu_dev, VugDev, parent);
     vdev_blk = container_of(gdev, VubDev, parent);
-- 
2.28.0

[PATCH 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 | 4 +++-
 1 file changed, 3 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..f445ef28ec 100644
--- a/contrib/vhost-user-gpu/vhost-user-gpu.c
+++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
@@ -1044,7 +1044,9 @@ 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)) {
+        return -1;
+    }
 
     if (opt_virgl) {
         g->virtio_config.num_capsets = vg_virgl_get_num_capsets();
-- 
2.28.0

[PATCH 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 | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/contrib/vhost-user-input/main.c b/contrib/vhost-user-input/main.c
index 6020c6f33a..50a8a03d9b 100644
--- a/contrib/vhost-user-input/main.c
+++ b/contrib/vhost-user-input/main.c
@@ -212,7 +212,9 @@ 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)) {
+        return -1;
+    }
 
     if (vi->sel_config) {
         memcpy(config, vi->sel_config, len);
-- 
2.28.0

[PATCH 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 | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c
index 62672d1cb9..d143ce8993 100644
--- a/block/export/vhost-user-blk-server.c
+++ b/block/export/vhost-user-blk-server.c
@@ -267,7 +267,9 @@ 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)) {
+        return -1;
+    }
 
     memcpy(config, &vexp->blkcfg, len);
     return 0;
-- 
2.28.0

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

[Marc-André Lureau]

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

Hi

On Wed, Nov 18, 2020 at 1:17 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:

> 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
>
>
The condition is the same for all the patches, checking the message config
payload is large enough. Afaict, the value is set by the client, so it
could be a runtime error, and thus explicit checking is required.

Nevertheless, one nice thing about g_return* macros, is that it provides an
error message when the condition fails, which helps. Could you replace it?

(fwiw, I think g_return* macros are so convenient, I would simply make
G_DISABLE_CHECKS forbidden and call it a day)


-- 
Marc-André Lureau

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

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

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Wed, Nov 18, 2020 at 1:17 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
>> 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
>>
>>
> The condition is the same for all the patches, checking the message config
> payload is large enough. Afaict, the value is set by the client, so it
> could be a runtime error, and thus explicit checking is required.
>
> Nevertheless, one nice thing about g_return* macros, is that it provides an
> error message when the condition fails, which helps. Could you replace it?
>
> (fwiw, I think g_return* macros are so convenient, I would simply make
> G_DISABLE_CHECKS forbidden and call it a day)

Nice or not, they are as inappropriate for input validation as assert()
is:

    If expr evaluates to FALSE, the current function should be
    considered to have undefined behaviour (a programmer error). The
    only correct solution to such an error is to change the module that
    is calling the current function, so that it avoids this incorrect
    call.

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

[Stefan Hajnoczi]

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

On Wed, Nov 18, 2020 at 07:21:15PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Nov 18, 2020 at 1:17 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> > 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
> >
> >
> The condition is the same for all the patches, checking the message config
> payload is large enough. Afaict, the value is set by the client, so it
> could be a runtime error, and thus explicit checking is required.
> 
> Nevertheless, one nice thing about g_return* macros, is that it provides an
> error message when the condition fails, which helps. Could you replace it?
> 
> (fwiw, I think g_return* macros are so convenient, I would simply make
> G_DISABLE_CHECKS forbidden and call it a day)

I'll add an error message in v2.

Stefan

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

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

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> 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().

Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Stefano Garzarella]

On Wed, Nov 18, 2020 at 09:16:40AM +0000, Stefan Hajnoczi wrote:
>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    | 4 +++-
> contrib/vhost-user-blk/vhost-user-blk.c | 4 +++-
> contrib/vhost-user-gpu/vhost-user-gpu.c | 4 +++-
> contrib/vhost-user-input/main.c         | 4 +++-
> 4 files changed, 12 insertions(+), 4 deletions(-)
>

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