Use of g_return_if_fail(), g_return_val_if_fail()

Use of g_return_if_fail(), g_return_val_if_fail()

[Markus Armbruster]

g_return_if_fail(), g_return_val_if_fail() are for programming errors:

    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.

Unlike assert(), they continue regardless, undefined behavior be damned:

    To make this undefined behaviour visible, if expr evaluates to
    FALSE, the result is usually that a critical message is logged and
    the current function returns.

Except when you ask for abort():

    To debug failure of a g_return_if_fail() check, run the code under a
    debugger with G_DEBUG=fatal-criticals or G_DEBUG=fatal-warnings
    defined in the environment.

Like assert(), they can be compiled out:

    If G_DISABLE_CHECKS is defined then the check is not performed. You
    should therefore not depend on any side effects of expr .

There are just three uses outside contrib/:

* backends/dbus-vmstate.c:232:        g_return_val_if_fail(bytes_read == len, -1);

  Marc-André, why is bytes_read != len a programming error?

  Why is returning safe?

* block/export/vhost-user-blk-server.c:270:    g_return_val_if_fail(len <= sizeof(struct virtio_blk_config), -1);

  Stefan, why is len > sizeof(struct virtio_blk_config) a programming
  error?

  Why is returning safe?

* hw/display/vhost-user-gpu.c:335:    g_return_if_fail(msg != NULL);

  This one is obviously dead code: g_malloc() cannot fail.

  If it could, I do doubt returning after reading a partial message
  would be safe.

Re: Use of g_return_if_fail(), g_return_val_if_fail()

[Marc-André Lureau]

Hi

On Tue, Nov 17, 2020 at 7:14 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> g_return_if_fail(), g_return_val_if_fail() are for programming errors:
>
>     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.
>
> Unlike assert(), they continue regardless, undefined behavior be damned:
>
>     To make this undefined behaviour visible, if expr evaluates to
>     FALSE, the result is usually that a critical message is logged and
>     the current function returns.
>
> Except when you ask for abort():
>
>     To debug failure of a g_return_if_fail() check, run the code under a
>     debugger with G_DEBUG=fatal-criticals or G_DEBUG=fatal-warnings
>     defined in the environment.
>
> Like assert(), they can be compiled out:
>
>     If G_DISABLE_CHECKS is defined then the check is not performed. You
>     should therefore not depend on any side effects of expr .
>
> There are just three uses outside contrib/:
>
> * backends/dbus-vmstate.c:232:        g_return_val_if_fail(bytes_read == len, -1);
>
>   Marc-André, why is bytes_read != len a programming error?
>
>   Why is returning safe?

It's "safe" as it returns -1 to indicate an error to post_load callback.

Hmm, it may not be just a programming error. read_all() may return
success with less bytes than requested.

Here, replacing it with full error_report() may be more appropriate,
since possibly the condition could happen if the input stream is
malformed. I can send a patch.

g_return* would be fine if it was just a programming error (checking
read_all contract for example).

Re: Use of g_return_if_fail(), g_return_val_if_fail()

[Stefan Hajnoczi]

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

On Tue, Nov 17, 2020 at 04:14:38PM +0100, Markus Armbruster wrote:
> * block/export/vhost-user-blk-server.c:270:    g_return_val_if_fail(len <= sizeof(struct virtio_blk_config), -1);
> 
>   Stefan, why is len > sizeof(struct virtio_blk_config) a programming
>   error?
> 
>   Why is returning safe?

Thanks for pointing this out. The vhost-user frontend passed an invalid
len and we're validating input. This and the other instances in
vhost-user config function in contrib/ should be replaced with explicit
input validation.

I'll send a patch.

Stefan

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