[libvirt] [PATCH] remote_internal.c: don't dereference a NULL "conn"

[Daniel P. Berrange]

On Wed, Sep 02, 2009 at 12:26:43PM +0200, Jim Meyering wrote:
> Daniel Veillard wrote:
> > On Wed, Sep 02, 2009 at 11:58:07AM +0200, Jim Meyering wrote:
> >> Here's the test just before the else-if in the patch below:
> >>
> >>     if (conn &&
> >>         conn->driver &&
> >>         STREQ (conn->driver->name, "remote")) {
> >>
> >> So, in the else-branch, "conn" is guaranteed to be NULL.
> >> And dereferenced.
> >>
> >> This may be only a theoretical risk, but if so,
> >> the test of "conn" above should be changed to an assertion,
> >> and/or the parameter should get the nonnull attribute.
> >
> >   I'm fine with your patch, so that even if code around changes
> > having a gard is IMHO a good thing
> 
> Thanks for the quick review.
> Daniel Berrange said he'd prefer the nonnull approach I mentioned above,
> so I've just adjusted (caveat, still not tested or even compiled)

Yeah, for functions where it is expected that the passed in param
be non-NULL, then annotations are definitely the way togo. This 
lets the compiler/checkers validate the callers instead, avoiding
the need to clutter the methods with irrelevant NULL checks.

> 
> >From 375bbb2d85fdf708858d726b6c7682e40dc07c2c Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering at redhat.com>
> Date: Wed, 2 Sep 2009 12:20:32 +0200
> Subject: [PATCH 1/2] infra: define ATTRIBUTE_NONNULL to mark non-NULL parameters
> 
> * src/internal.h (ATTRIBUTE_NONNULL): Define.
> ---
>  src/internal.h |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/src/internal.h b/src/internal.h
> index 936cd03..8fa579c 100644
> --- a/src/internal.h
> +++ b/src/internal.h
> @@ -116,6 +116,14 @@
>  #endif
>  #endif
> 
> +#ifndef ATTRIBUTE_NONNULL
> +# if __GNUC_PREREQ (3, 3)
> +#  define ATTRIBUTE_NONNULL(m) __attribute__((__nonnull__(m)))
> +# else
> +#  define ATTRIBUTE_NONNULL(m)
> +# endif
> +#endif
> +
>  #else
>  #ifndef ATTRIBUTE_UNUSED
>  #define ATTRIBUTE_UNUSED
> --
> 1.6.4.2.395.ge3d52
> 
> 
> >From 61dc45fcae8b17d9f619563d427f1bc74f44d553 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering at redhat.com>
> Date: Wed, 2 Sep 2009 12:22:14 +0200
> Subject: [PATCH 2/2] remote_internal.c: appease clang
> 
> * src/remote_internal.c (remoteNetworkOpen): Mark "conn" parameter
> as non-NULL.  Remove now-unnecessary "conn == NULL" test.
> (remoteDevMonOpen): Likewise.
> ---
>  src/remote_internal.c |   10 ++++------
>  1 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/src/remote_internal.c b/src/remote_internal.c
> index ea50c11..fe36ddd 100644
> --- a/src/remote_internal.c
> +++ b/src/remote_internal.c
> @@ -3281,13 +3281,12 @@ done:
>  static virDrvOpenStatus
>  remoteNetworkOpen (virConnectPtr conn,
>                     virConnectAuthPtr auth,
> -                   int flags)
> +                   int flags) ATTRIBUTE_NONNULL (1)
>  {
>      if (inside_daemon)
>          return VIR_DRV_OPEN_DECLINED;
> 
> -    if (conn &&
> -        conn->driver &&
> +    if (conn->driver &&
>          STREQ (conn->driver->name, "remote")) {
>          struct private_data *priv;
> 
> @@ -5130,13 +5129,12 @@ done:
>  static virDrvOpenStatus
>  remoteDevMonOpen(virConnectPtr conn,
>                   virConnectAuthPtr auth ATTRIBUTE_UNUSED,
> -                 int flags ATTRIBUTE_UNUSED)
> +                 int flags ATTRIBUTE_UNUSED) ATTRIBUTE_NONNULL (1)
>  {
>      if (inside_daemon)
>          return VIR_DRV_OPEN_DECLINED;
> 
> -    if (conn &&
> -        conn->driver &&
> +    if (conn->driver &&
>          STREQ (conn->driver->name, "remote")) {
>          struct private_data *priv = conn->privateData;
>          /* If we're here, the remote driver is already

ACK


Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|