1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH v2 00/11] Add admin protocol support for virtlogd/virtlockd
  4. View patch

[libvirt] [PATCH v2 07/11] rpc: annotate various parameters as being required to be non-NULL

Daniel P. Berrange posted 11 patches 7 years, 3 months ago
[libvirt] [PATCH v2 07/11] rpc: annotate various parameters as being required to be non-NULL
Posted by Daniel P. Berrange 7 years, 3 months ago 
The server name and client data callbacks need to be non-NULL or the
system will crash at various times. This is particularly bad when some
of the crashes only occur post-exec restart.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 src/rpc/virnetserver.h       | 7 +++++--
 src/rpc/virnetserverclient.h | 7 +++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/src/rpc/virnetserver.h b/src/rpc/virnetserver.h
index 7728a67f5f..a79c39fdb2 100644
--- a/src/rpc/virnetserver.h
+++ b/src/rpc/virnetserver.h
@@ -47,7 +47,8 @@ virNetServerPtr virNetServerNew(const char *name,
                                 virNetServerClientPrivNew clientPrivNew,
                                 virNetServerClientPrivPreExecRestart clientPrivPreExecRestart,
                                 virFreeCallback clientPrivFree,
-                                void *clientPrivOpaque);
+                                void *clientPrivOpaque)
+    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(11) ATTRIBUTE_NONNULL(13);
 
 virNetServerPtr virNetServerNewPostExecRestart(virJSONValuePtr object,
                                                const char *name,
@@ -55,7 +56,9 @@ virNetServerPtr virNetServerNewPostExecRestart(virJSONValuePtr object,
                                                virNetServerClientPrivNewPostExecRestart clientPrivNewPostExecRestart,
                                                virNetServerClientPrivPreExecRestart clientPrivPreExecRestart,
                                                virFreeCallback clientPrivFree,
-                                               void *clientPrivOpaque);
+                                               void *clientPrivOpaque)
+    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+    ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(6);
 
 void virNetServerClose(virNetServerPtr srv);
 
diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h
index 3c48759abc..4a0d3cc25e 100644
--- a/src/rpc/virnetserverclient.h
+++ b/src/rpc/virnetserverclient.h
@@ -72,14 +72,17 @@ virNetServerClientPtr virNetServerClientNew(unsigned long long id,
                                             virNetServerClientPrivNew privNew,
                                             virNetServerClientPrivPreExecRestart privPreExecRestart,
                                             virFreeCallback privFree,
-                                            void *privOpaque);
+                                            void *privOpaque)
+    ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(7) ATTRIBUTE_NONNULL(9);
 
 virNetServerClientPtr virNetServerClientNewPostExecRestart(virNetServerPtr srv,
                                                            virJSONValuePtr object,
                                                            virNetServerClientPrivNewPostExecRestart privNew,
                                                            virNetServerClientPrivPreExecRestart privPreExecRestart,
                                                            virFreeCallback privFree,
-                                                           void *privOpaque);
+                                                           void *privOpaque)
+    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+    ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5);
 
 virJSONValuePtr virNetServerClientPreExecRestart(virNetServerClientPtr client);
 
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 07/11] rpc: annotate various parameters as being required to be non-NULL
Posted by John Ferlan 7 years, 3 months ago 

On 01/23/2018 08:23 AM, Daniel P. Berrange wrote:
> The server name and client data callbacks need to be non-NULL or the
> system will crash at various times. This is particularly bad when some
> of the crashes only occur post-exec restart.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  src/rpc/virnetserver.h       | 7 +++++--
>  src/rpc/virnetserverclient.h | 7 +++++--
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 

My Coverity build was not happy this morning...

> diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h
> index 3c48759abc..4a0d3cc25e 100644
> --- a/src/rpc/virnetserverclient.h
> +++ b/src/rpc/virnetserverclient.h
> @@ -72,14 +72,17 @@ virNetServerClientPtr virNetServerClientNew(unsigned long long id,
>                                              virNetServerClientPrivNew privNew,
>                                              virNetServerClientPrivPreExecRestart privPreExecRestart,
>                                              virFreeCallback privFree,
> -                                            void *privOpaque);
> +                                            void *privOpaque)
> +    ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(7) ATTRIBUTE_NONNULL(9);

This one caused the Coverity build to fail because virNetServerClientNew
checks "if (privNew)" before assigning client->privateData

>  
>  virNetServerClientPtr virNetServerClientNewPostExecRestart(virNetServerPtr srv,
>                                                             virJSONValuePtr object,
>                                                             virNetServerClientPrivNewPostExecRestart privNew,
>                                                             virNetServerClientPrivPreExecRestart privPreExecRestart,
>                                                             virFreeCallback privFree,
> -                                                           void *privOpaque);
> +                                                           void *privOpaque)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
> +    ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5);

Likewise here too for virNetServerClientNewPostExecRestart w/ privNew

Finally, the "tests/virnetserverclienttest.c fails to build because of
the NULL argument check.

Reproducible if you enable static analysis...

John
>  
>  virJSONValuePtr virNetServerClientPreExecRestart(virNetServerClientPtr client);
>  
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 07/11] rpc: annotate various parameters as being required to be non-NULL
Posted by John Ferlan 7 years, 3 months ago 

On 02/01/2018 06:57 AM, John Ferlan wrote:
> 
> 
> On 01/23/2018 08:23 AM, Daniel P. Berrange wrote:
>> The server name and client data callbacks need to be non-NULL or the
>> system will crash at various times. This is particularly bad when some
>> of the crashes only occur post-exec restart.
>>
>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>> ---
>>  src/rpc/virnetserver.h       | 7 +++++--
>>  src/rpc/virnetserverclient.h | 7 +++++--
>>  2 files changed, 10 insertions(+), 4 deletions(-)
>>
> 
> My Coverity build was not happy this morning...
> 
>> diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h
>> index 3c48759abc..4a0d3cc25e 100644
>> --- a/src/rpc/virnetserverclient.h
>> +++ b/src/rpc/virnetserverclient.h
>> @@ -72,14 +72,17 @@ virNetServerClientPtr virNetServerClientNew(unsigned long long id,
>>                                              virNetServerClientPrivNew privNew,
>>                                              virNetServerClientPrivPreExecRestart privPreExecRestart,
>>                                              virFreeCallback privFree,
>> -                                            void *privOpaque);
>> +                                            void *privOpaque)
>> +    ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(7) ATTRIBUTE_NONNULL(9);
> 
> This one caused the Coverity build to fail because virNetServerClientNew
> checks "if (privNew)" before assigning client->privateData
> 

Also wouldn't the NONNULL's need to change based on "#ifdef WITH_GNUTLS"
for arguments after #5?

>>  
>>  virNetServerClientPtr virNetServerClientNewPostExecRestart(virNetServerPtr srv,
>>                                                             virJSONValuePtr object,
>>                                                             virNetServerClientPrivNewPostExecRestart privNew,
>>                                                             virNetServerClientPrivPreExecRestart privPreExecRestart,
>>                                                             virFreeCallback privFree,
>> -                                                           void *privOpaque);
>> +                                                           void *privOpaque)
>> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
>> +    ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5);
> 
> Likewise here too for virNetServerClientNewPostExecRestart w/ privNew
> 
> Finally, the "tests/virnetserverclienttest.c fails to build because of
> the NULL argument check.
> 

And virnetdaemontest.c...


John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 07/11] rpc: annotate various parameters as being required to be non-NULL
Posted by Daniel P. Berrangé 7 years, 3 months ago 
On Thu, Feb 01, 2018 at 07:14:26AM -0500, John Ferlan wrote:
> 
> 
> On 02/01/2018 06:57 AM, John Ferlan wrote:
> > 
> > 
> > On 01/23/2018 08:23 AM, Daniel P. Berrange wrote:
> >> The server name and client data callbacks need to be non-NULL or the
> >> system will crash at various times. This is particularly bad when some
> >> of the crashes only occur post-exec restart.
> >>
> >> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> >> ---
> >>  src/rpc/virnetserver.h       | 7 +++++--
> >>  src/rpc/virnetserverclient.h | 7 +++++--
> >>  2 files changed, 10 insertions(+), 4 deletions(-)
> >>
> > 
> > My Coverity build was not happy this morning...
> > 
> >> diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h
> >> index 3c48759abc..4a0d3cc25e 100644
> >> --- a/src/rpc/virnetserverclient.h
> >> +++ b/src/rpc/virnetserverclient.h
> >> @@ -72,14 +72,17 @@ virNetServerClientPtr virNetServerClientNew(unsigned long long id,
> >>                                              virNetServerClientPrivNew privNew,
> >>                                              virNetServerClientPrivPreExecRestart privPreExecRestart,
> >>                                              virFreeCallback privFree,
> >> -                                            void *privOpaque);
> >> +                                            void *privOpaque)
> >> +    ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(7) ATTRIBUTE_NONNULL(9);
> > 
> > This one caused the Coverity build to fail because virNetServerClientNew
> > checks "if (privNew)" before assigning client->privateData
> > 
> 
> Also wouldn't the NONNULL's need to change based on "#ifdef WITH_GNUTLS"
> for arguments after #5?

Oh fun yes.

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 07/11] rpc: annotate various parameters as being required to be non-NULL
Posted by Daniel P. Berrangé 7 years, 3 months ago 
On Thu, Feb 01, 2018 at 06:57:49AM -0500, John Ferlan wrote:
> 
> 
> On 01/23/2018 08:23 AM, Daniel P. Berrange wrote:
> > The server name and client data callbacks need to be non-NULL or the
> > system will crash at various times. This is particularly bad when some
> > of the crashes only occur post-exec restart.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  src/rpc/virnetserver.h       | 7 +++++--
> >  src/rpc/virnetserverclient.h | 7 +++++--
> >  2 files changed, 10 insertions(+), 4 deletions(-)
> > 
> 
> My Coverity build was not happy this morning...
> 
> > diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h
> > index 3c48759abc..4a0d3cc25e 100644
> > --- a/src/rpc/virnetserverclient.h
> > +++ b/src/rpc/virnetserverclient.h
> > @@ -72,14 +72,17 @@ virNetServerClientPtr virNetServerClientNew(unsigned long long id,
> >                                              virNetServerClientPrivNew privNew,
> >                                              virNetServerClientPrivPreExecRestart privPreExecRestart,
> >                                              virFreeCallback privFree,
> > -                                            void *privOpaque);
> > +                                            void *privOpaque)
> > +    ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(7) ATTRIBUTE_NONNULL(9);
> 
> This one caused the Coverity build to fail because virNetServerClientNew
> checks "if (privNew)" before assigning client->privateData
> 
> >  
> >  virNetServerClientPtr virNetServerClientNewPostExecRestart(virNetServerPtr srv,
> >                                                             virJSONValuePtr object,
> >                                                             virNetServerClientPrivNewPostExecRestart privNew,
> >                                                             virNetServerClientPrivPreExecRestart privPreExecRestart,
> >                                                             virFreeCallback privFree,
> > -                                                           void *privOpaque);
> > +                                                           void *privOpaque)
> > +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
> > +    ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5);
> 
> Likewise here too for virNetServerClientNewPostExecRestart w/ privNew

Fun, I'm trying to understand again how I caused the crash when these
were NULL. I think it was because in the virNetServerClientNew() call
they were non-NULL, but the virNetServerClientNewPostExecRestart()
I had passed NULL, or vica-verca.

Anyway since all production code callers pass non-NULL, I think these
should be marked NONNULL regardless of crash possibility....

> Finally, the "tests/virnetserverclienttest.c fails to build because of
> the NULL argument check.

...so I'll fix this test and remove the if (...) checks :-)


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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.