[libvirt] [PATCH v2 3/4] virDomainChrGetDomainPtrsInternal: Return an integer

[Peter Krempa]

On Thu, Jun 02, 2016 at 13:58:52 +0200, Michal Privoznik wrote:
> On 02.06.2016 13:36, Peter Krempa wrote:
> > On Thu, Jun 02, 2016 at 12:42:52 +0200, Michal Privoznik wrote:
> >> There's this problem on the recent gcc-6.1:
> >>
> >> In file included from conf/domain_conf.c:37:0:
> >> conf/domain_conf.c: In function 'virDomainChrPreAlloc':
> >> conf/domain_conf.c:14109:35: error: potential null pointer dereference [-Werror=null-dereference]
> >>      return VIR_REALLOC_N(*arrPtr, *cntPtr + 1);
> >>                                    ^~
> >> ./util/viralloc.h:158:73: note: in definition of macro 'VIR_REALLOC_N'
> >>  # define VIR_REALLOC_N(ptr, count) virReallocN(&(ptr), sizeof(*(ptr)), (count), \
> >>                                                                          ^~~~~
> >> conf/domain_conf.c: In function 'virDomainChrRemove':
> >> conf/domain_conf.c:14133:21: error: potential null pointer dereference [-Werror=null-dereference]
> >>      for (i = 0; i < *cntPtr; i++) {
> >>                      ^~~~~~~
> >>
> >> GCC basically fails to see, that the
> >> virDomainChrGetDomainPtrsInternal will never actually return NULL
> >> because it's never called over a domain char device with _LAST
> >> type. But to make it shut up, lets turn this function into
> >> returning an integer and check in the callers if a zero value
> >> value was returned.
> >>
> >> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> >> ---
> >>  src/conf/domain_conf.c | 27 +++++++++++++++++----------
> >>  1 file changed, 17 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> >> index 568c699..2efe0a3 100644
> >> --- a/src/conf/domain_conf.c
> >> +++ b/src/conf/domain_conf.c
> >> @@ -14038,7 +14038,7 @@ virDomainChrFind(virDomainDefPtr def,
> >>  
> >>  /* Return the address within vmdef to be modified when working with a
> >>   * chrdefptr of the given type.  */
> >> -static void
> >> +static int ATTRIBUTE_RETURN_CHECK
> >>  virDomainChrGetDomainPtrsInternal(virDomainDefPtr vmdef,
> >>                                    virDomainChrDeviceType type,
> >>                                    virDomainChrDefPtr ***arrPtr,
> >> @@ -14070,6 +14070,8 @@ virDomainChrGetDomainPtrsInternal(virDomainDefPtr vmdef,
> >>          *cntPtr = NULL;
> >>          break;
> >>      }
> >> +
> >> +    return (*arrPtr && *cntPtr) ? 0 : -1;
> > 
> > This doesn't set any error. The VIR_DOMAIN_CHR_DEVICE_TYPE_LAST case
> > should do so and possibly return -1 right away to avoid the ternary.
> 
> Well, I'm sort of puzzled here. The reason why gcc thinks we might deref
> NULL is that at the input of this function a chardev with a type other
> than enumerated in the switch occurred. In which case we don't set any
> of the passed arguments and thus effectively defer NULL later in the
> code. This is obviously incorrect as our parser guarantees chardev type
> within range as expected here.
> 
> Anyway, if I went by your approach, compiler would think that for
> CHR_DEVICE_TYPE_YET_UNKNOWN we leave the pointers untouched and return 0
> and deref NULL. The same compiler which checks all enum members are
> enumerated in the switch and therefore there can't be any
> CHR_DEVICE_TYPE_YET_UNKNOWN. Le sigh.

I think you could do something along:

    *arrPtr = NULL;
    *cntPtr = NULL;

    switch (type) {
    case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL:
        *arrPtr = &vmdef->parallels;
        *cntPtr = &vmdef->nparallels;
        return 0;

    case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL:
        *arrPtr = &vmdef->serials;
        *cntPtr = &vmdef->nserials;
        return 0;

    case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE:
        *arrPtr = &vmdef->consoles;
        *cntPtr = &vmdef->nconsoles;
        return 0;

    case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL:
        *arrPtr = &vmdef->channels;
        *cntPtr = &vmdef->nchannels;
        return 0;

    case VIR_DOMAIN_CHR_DEVICE_TYPE_LAST:
        break;
    }

    virReportError(VIR_..., "it's broken");
    return -1;

... to basically create a 'default' case outside of the switch.

Peter