1. Patchew
  2. QEMU
  3. View series

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

Dr. David Alan Gilbert (git) posted 1 patch 6 years, 12 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20170427165526.19836-1-dgilbert@redhat.com
Test checkpatch passed
Test docker passed
Test s390x passed
scripts/checkpatch.pl | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
[Qemu-devel] [PATCH] checkpatch: Disallow glib asserts in main code
Posted by Dr. David Alan Gilbert (git) 6 years, 12 months ago 
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
Posted by Paolo Bonzini 6 years, 12 months ago 

----- 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
Posted by Daniel P. Berrange 6 years, 12 months ago 
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
Posted by Dr. David Alan Gilbert 6 years, 12 months ago 
* 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
Posted by Daniel P. Berrange 6 years, 12 months ago 
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
Posted by Eric Blake 6 years, 12 months ago 
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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.