Re: [Qemu-devel] [PATCH] checkpatch: Disallow glib asserts in main code

[Markus Armbruster <armbru@redhat.com>]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Glib commit a6a875068779 (from 2013) made many of the glib assert
> macros non-fatal if a flag is set.
> This causes two problems:
>   a) Compilers moan that your code is unsafe even though you've
>      put an assert in before the point of use.
>   b) Someone evil could, in a library, call
>      g_test_set_nonfatal_assertions() and cause our assertions in
>      important places not to fail and potentially allow memory overruns.
>
> Ban most of the glib assertion functions (basically everything except
> g_assert and g_assert_not_reached) except in tests/
>
> This makes checkpatch gives an error such as:
>
>   ERROR: Use g_assert or g_assert_not_reached
>   #77: FILE: vl.c:4725:
>   +    g_assert_cmpstr("Chocolate", >, "Cheese");
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  scripts/checkpatch.pl | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index f084542934..73cee81b79 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2571,6 +2571,27 @@ sub process {
>  		if ($line =~ /\bbzero\(/) {
>  			ERROR("use memset() instead of bzero()\n" . $herecurr);
>  		}
> +		my $non_exit_glib_asserts = qr{g_assert_cmpstr|
> +						g_assert_cmpint|
> +						g_assert_cmpuint|
> +						g_assert_cmphex|
> +						g_assert_cmpfloat|
> +						g_assert_true|
> +						g_assert_false|
> +						g_assert_nonnull|
> +						g_assert_null|
> +						g_assert_no_error|
> +						g_assert_error|
> +						g_test_assert_expected_messages|
> +						g_test_trap_assert_passed|
> +						g_test_trap_assert_stdout|
> +						g_test_trap_assert_stdout_unmatched|
> +						g_test_trap_assert_stderr|
> +						g_test_trap_assert_stderr_unmatched}x;
> +		if ($realfile !~ /^tests\// &&
> +			$line =~ /\b(?:$non_exit_glib_asserts)\(/) {
> +			ERROR("Use g_assert or g_assert_not_reached\n". $herecurr);
> +		}
>  	}
>  
>  	# If we have no input at all, then there is nothing to report on

If these are screwy enough to warrant rejecting them in new code,
they're probably screwy enough to purge them from existing code:

    $ git-grep -E 'g_assert_cmpstr|g_assert_cmpint|g_assert_cmpuint|g_assert_cmphex|g_assert_cmpfloat|g_assert_true|g_assert_false|g_assert_nonnull|g_assert_null|g_assert_no_error|g_assert_error|g_test_assert_expected_messages|g_test_trap_assert_passed|g_test_trap_assert_stdout|g_test_trap_assert_stdout_unmatched|g_test_trap_assert_stderr|g_test_trap_assert_stderr_unmatched' | grep -v ^tests/
    hw/ide/ahci.c:        g_assert_cmpint(size, >, 1);
    hw/ppc/spapr_ovec.c:    g_assert_cmpint(bitnr, <, OV_MAXBITS);
    hw/ppc/spapr_ovec.c:    g_assert_cmpint(bitnr, <, OV_MAXBITS);
    hw/ppc/spapr_ovec.c:    g_assert_cmpint(bitnr, <, OV_MAXBITS);
    hw/ppc/spapr_ovec.c:    g_assert_cmpint(vector, >=, 1); /* vector numbering starts at 1 */
    hw/ppc/spapr_ovec.c:    g_assert_cmpint(vector_len, <=, OV_MAXBYTES);
    hw/ppc/spapr_ovec.c:    g_assert_cmpint(vec_len, <=, OV_MAXBYTES);
    include/glib-compat.h:#ifndef g_assert_true
    include/glib-compat.h:#define g_assert_true(expr)                                                    \
    include/glib-compat.h:#ifndef g_assert_false
    include/glib-compat.h:#define g_assert_false(expr)                                                   \
    include/glib-compat.h:#ifndef g_assert_null
    include/glib-compat.h:#define g_assert_null(expr)                                                    \
    include/glib-compat.h:#ifndef g_assert_nonnull
    include/glib-compat.h:#define g_assert_nonnull(expr)                                                 \
    qom/object.c:        g_assert_cmpint(parent->class_size, <=, ti->class_size);
    qom/object.c:    g_assert_cmpint(type->instance_size, >=, sizeof(Object));
    qom/object.c:    g_assert_cmpint(size, >=, type->instance_size);
    qom/object.c:    g_assert_cmpint(obj->ref, ==, 0);
    qom/object.c:    g_assert_cmpint(obj->ref, >, 0);
    scripts/cocci-macro-file.h:#define g_assert_cmpint(a, op, b)   g_assert(a op b)
    scripts/cocci-macro-file.h:#define g_assert_cmpuint(a, op, b)   g_assert(a op b)
    scripts/cocci-macro-file.h:#define g_assert_cmphex(a, op, b)   g_assert(a op b)
    scripts/cocci-macro-file.h:#define g_assert_cmpstr(a, op, b)   g_assert(strcmp(a, b) op 0)
    util/qht.c:    g_assert_cmpuint(new->n_buckets, !=, old->n_buckets);

Volunteers?