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

[Michal Privoznik]

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.

This is actually the reason I want to avoid having 'default' label in
the switch. If we ever add new device type it's nice to have compiler
identify all^Wa lot of^W^W^Wsome places that need fixing.

> 
>>  }
>>  
> 
> [...]
> 
>> @@ -14104,7 +14105,9 @@ virDomainChrPreAlloc(virDomainDefPtr vmdef,
>>      virDomainChrDefPtr **arrPtr = NULL;
>>      size_t *cntPtr = NULL;
>>  
>> -    virDomainChrGetDomainPtrsInternal(vmdef, chr->deviceType, &arrPtr, &cntPtr);
>> +    if (virDomainChrGetDomainPtrsInternal(vmdef, chr->deviceType,
>> +                                          &arrPtr, &cntPtr) < 0)
>> +        return -1;
> 
> So this will report the "unknown error".

Well, in theory yes, but in reality I don't see a way how the error
could happen in the first place.

> 
>>  
>>      return VIR_REALLOC_N(*arrPtr, *cntPtr + 1);
> 
> ACK with the error added.
>