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

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(a)redhat.com&gt; >> --- >> 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.