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

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

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.

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

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>

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>