Re: [Xen-devel] New Defects reported by Coverity Scan for XenProject

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Wed, 2016-02-24 at 21:02 -0800, scan-admin@xxxxxxxxxxxx wrote:
> Hi,
> 
> Please find the latest report on new defect(s) introduced to XenProject found 
> with Coverity Scan.
> 
> 2 new defect(s) introduced to XenProject found with Coverity Scan.
> 12 defect(s), reported by Coverity Scan earlier, were marked fixed in the 
> recent build analyzed by Coverity Scan.
> 
> New defect(s) Reported-by: Coverity Scan
> Showing 2 of 2 defect(s)
> 
> 
> ** CID 1354244:  Null pointer dereferences  (FORWARD_NULL)
> /tools/libxc/xc_tbuf.c: 72 in xc_tbuf_get_size()
> 
> 
> ________________________________________________________________________________________________________
> *** CID 1354244:  Null pointer dereferences  (FORWARD_NULL)
> /tools/libxc/xc_tbuf.c: 72 in xc_tbuf_get_size()
> 66             return rc;
> 67     
> 68         t_info = xc_map_foreign_range(xch, DOMID_XEN,
> 69                         sysctl.u.tbuf_op.size, PROT_READ | PROT_WRITE,
> 70                         sysctl.u.tbuf_op.buffer_mfn);
> 71     
> > > >     CID 1354244:  Null pointer dereferences  (FORWARD_NULL)
> > > >     Comparing "t_info" to null implies that "t_info" might be null.
> 72         if ( t_info == NULL || t_info->tbuf_size == 0 )
> 73             rc = -1;
> 74         else
> 75            *size = t_info->tbuf_size;
> 76     
> 77         xenforeignmemory_unmap(xch->fmem, t_info, sysctl.u.tbuf_op.size);

This is complaining about the eventual munmap(t_info) => munmap(NULL) which
is behind xenforeignmemory_unmap().

Looks like it was newly added by the fix to CID 1351228 in 7c479883b04a
("libxc: fix leak of t_info in xc_tbuf_get_size()").
xenforeignmemory_unmap() should behave like munmap WRT tollerance of NULL,
I'm not 100% sure what that behaviour is since 0 is a valid address.
xenforeignmemory.h no doubt wants updating with the desired semantics and
either this code of the implementation adjusting to match.

While here I notice that using xc_map_*() to create the mapping and
xenforeignmemory_unmap() to destroy it is a bit odd since they are strictly
two separate APIs, even if one happens to be implemented in terms of the
other. Being libxc internal this code is at liberty to use xc_map_* but
should then use plain munmap to undo it, or it would also be reasonable to
port this code fully to the xenforeignmemory interface.

> 
> ** CID 1354243:  Control flow issues  (DEADCODE)
> /tools/xentrace/xenalyze.c: 4148 in cr3_dump_list()
> 
> 
> ________________________________________________________________________________________________________
> *** CID 1354243:  Control flow issues  (DEADCODE)
> /tools/xentrace/xenalyze.c: 4148 in cr3_dump_list()
> 4142     
> 4143         /* Count the number of elements */
> 4144         for(p=head; p; p=p->next)
> 4145             N++;
> 4146     
> 4147         if(!N)
> > > >     CID 1354243:  Control flow issues  (DEADCODE)
> > > >     Execution cannot reach this statement: "return;".

Here it has observed that due to the (above, just out of the context given
here) "if (!head) return" that the for loop must run at least once, so N
cannot be 0.

My guess is that this is a prexisting issue which was exposed to coverities
beady eye somehow by 28ab9f3d0e7c ("tools/xenalyze: Fix build with clang").
Or maybe this was previous marked deliberate but the change has caused
coverity to think this is a different instance of the same thing, eitherway
I don't think the issue itself is new.

FWIW having both if (!head) return and if (!N) return looks redundant to
me, the other two similar looking instances (from grepping for N++) have
only the latter check.

Ian.

> 4148             return;
> 4149     
> 4150         /* Alloc a struct of the right size */
> 4151         qsort_array = malloc(N * sizeof(struct eip_list_struct *));
> 4152     
> 4153         /* Point the array into it */
> 
> 
> ________________________________________________________________________________________________________
> To view the defects in Coverity Scan visit, 
> https://scan.coverity.com/projects/xenproject?tab=overview
> 
> To manage Coverity Scan email notifications for "ian.campbell@xxxxxxxxxx", 
> click 
> https://scan.coverity.com/subscriptions/edit?email=ian.campbell%40citrix.com&token=86eecf9a70a32d8ef6ac41e4c7cdaf58
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel