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

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

[Dr. David Alan Gilbert (git)]

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
-- 
2.12.2

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

[Paolo Bonzini]

----- Original Message -----
> From: "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com>
> To: qemu-devel@nongnu.org, pbonzini@redhat.com, armbru@redhat.com
> Sent: Thursday, April 27, 2017 6:55:26 PM
> Subject: [Qemu-devel] [PATCH] checkpatch: Disallow glib asserts in main code
> 
> 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
> --
> 2.12.2
> 
> 
> 

Queued, thanks.

Paolo

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

[Markus Armbruster]

"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?

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

[Peter Maydell]

On 28 April 2017 at 14:34, Markus Armbruster <armbru@redhat.com> wrote:
> If these are screwy enough to warrant rejecting them in new code,
> they're probably screwy enough to purge them from existing code:
>

>     include/glib-compat.h:#ifndef g_assert_true
>     include/glib-compat.h:#define g_assert_true(expr)

These ones prompted me to wonder if we could/should use
#pragma GCC poison g_assert_true

rather than just leaving it up to checkpatch.

thanks
-- PMM

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

[Eric Blake]

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

On 04/28/2017 08:34 AM, Markus Armbruster wrote:

>>
>> Ban most of the glib assertion functions (basically everything except
>> g_assert and g_assert_not_reached) except in tests/

You'll also want to exclude scripts/ and possible include/glib-compat.h...


> 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);

Those should go.

>     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)                                                 \

We still need these until we can require glib 2.38 or even 2.40.

>     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);

These should go.

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

These must stay permanently.

>     util/qht.c:    g_assert_cmpuint(new->n_buckets, !=, old->n_buckets);
> 
> Volunteers?
> 
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Daniel P. Berrange]

On Thu, Apr 27, 2017 at 05:55:26PM +0100, Dr. David Alan Gilbert (git) wrote:
> 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");

Or could we perhaps instead undo the damage via a hack like

 #define g_assert_cmpint g_assert_cmpint_orig
 #define g_assert_cmpint(x, y, z) \
     g_assert_cmpint_orig(x, y,x); \
     abort()
    

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Dr. David Alan Gilbert]

* Daniel P. Berrange (berrange@redhat.com) wrote:
> On Thu, Apr 27, 2017 at 05:55:26PM +0100, Dr. David Alan Gilbert (git) wrote:
> > 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");
> 
> Or could we perhaps instead undo the damage via a hack like
> 
>  #define g_assert_cmpint g_assert_cmpint_orig
>  #define g_assert_cmpint(x, y, z) \
>      g_assert_cmpint_orig(x, y,x); \
>      abort()

I'd be kind of OK adding a q_assert_cmpint if you wanted,
but I think we shouldn't change the semantics of a public
name.

Dave

> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Daniel P. Berrange]

On Fri, Apr 28, 2017 at 02:45:32PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrange (berrange@redhat.com) wrote:
> > On Thu, Apr 27, 2017 at 05:55:26PM +0100, Dr. David Alan Gilbert (git) wrote:
> > > 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");
> > 
> > Or could we perhaps instead undo the damage via a hack like
> > 
> >  #define g_assert_cmpint g_assert_cmpint_orig
> >  #define g_assert_cmpint(x, y, z) \
> >      g_assert_cmpint_orig(x, y,x); \
> >      abort()
> 
> I'd be kind of OK adding a q_assert_cmpint if you wanted,
> but I think we shouldn't change the semantics of a public
> name.

Personally I think it would be worth having them - the whole point of
these more specific g_assert_* macros is that they provide clearer
error messages when they're triggered, so I prefer their use generally


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Eric Blake]

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

On 04/28/2017 10:10 AM, Daniel P. Berrange wrote:

>>> Or could we perhaps instead undo the damage via a hack like
>>>
>>>  #define g_assert_cmpint g_assert_cmpint_orig
>>>  #define g_assert_cmpint(x, y, z) \
>>>      g_assert_cmpint_orig(x, y,x); \
>>>      abort()

Not quite the right hack (we don't want to unconditionally abort, but
only when the condition fails).

>>
>> I'd be kind of OK adding a q_assert_cmpint if you wanted,
>> but I think we shouldn't change the semantics of a public
>> name.

I tend to agree there; having our own distinct name means that we can
see at a glance that our version will quit, no matter what the glib
version does.

> 
> Personally I think it would be worth having them - the whole point of
> these more specific g_assert_* macros is that they provide clearer
> error messages when they're triggered, so I prefer their use generally

I agree that the improved error messages part is worthwhile.  So maybe
we want:

#define q_assert_cmpint(x, y, z) \
  do { \
    g_assert_cmpint(x, y, z); \
    assert(x y z); \
  } while (0)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]