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@xxxxxxxxxx>
> >> ---
> >> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list