Re: [Xen-devel] New Defects reported by Coverity Scan for XenProject
scan-ad...@coverity.com writes ("New Defects reported by Coverity Scan for
XenProject"):
> Please find the latest report on new defect(s) introduced to XenProject found
> with Coverity Scan.
>
> 35 new defect(s) introduced to XenProject found with Coverity Scan.
> 2 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 20 of 35 defect(s)
I have been through the tools reports here. None of them are security
problems in released branches.
I would like some advice from Coverity experts on the two below:
> ** CID 1358088: Concurrent data access violations (MISSING_LOCK)
Applies to 1358086..1358105 inclusive. Here is a sample:
> *** CID 1358088: Concurrent data access violations (MISSING_LOCK)
> /tools/libxl/libxl_event.c: 2189 in libxl__ao_progress_report()
> 2183 } else if (how->callback) {
> 2184 libxl__aop_occurred *aop = libxl__zalloc(>gc,
> sizeof(*aop));
> 2185 ao->progress_reports_outstanding++;
> 2186 aop->ao = ao;
> 2187 aop->ev = ev;
> 2188 aop->how = how;
> >>> CID 1358088: Concurrent data access violations (MISSING_LOCK)
> >>> Accessing "egc->aops_for_callback.tqh_last" without holding lock
> >>> "libxl__ctx.lock". Elsewhere, "libxl__egc.aops_for_callback.tqh_last" is
> >>> accessed with "libxl__ctx.lock" held 34 out of 44 times.
> 2189 LIBXL_TAILQ_INSERT_TAIL(>aops_for_callback, aop, entry);
> 2190 LOG(DEBUG,"ao %p: progress report: callback queued
> aop=%p",ao,aop);
> 2191 } else {
> 2192 LOG(DEBUG,"ao %p: progress report: event queued ev=%p
> type=%s",
> 2193 ao, ev, libxl_event_type_to_string(ev->type));
> 2194 libxl__event_occurred(egc, ev);
This is a false positive. An egc is always allocated on the stack of
the thread that uses it. It is only ever used by that thread. So
does not need to be protected by a lock.
Is there a way we can teach Coverity about this ?
> ** CID 1358085: Incorrect expression (IDENTICAL_BRANCHES)
> /tools/libxl/_libxl_types.c: 5611 in libxl_device_usbdev_gen_json()
Applies to 1358081..1358084 inclusive.
Here is a sample:
> *** CID 1358085: Incorrect expression (IDENTICAL_BRANCHES)
> /tools/libxl/_libxl_types.c: 5611 in libxl_device_usbdev_gen_json()
> 5605 if (s != yajl_gen_status_ok)
> 5606 goto out;
> 5607 break;
> 5608 }
> 5609 }
> 5610 s = yajl_gen_map_close(hand);
> >>> CID 1358085: Incorrect expression (IDENTICAL_BRANCHES)
> >>> The same code is executed when the condition "s !=
> >>> yajl_gen_status_ok" is true or false, because the code in the if-then
> >>> branch and after the if statement is identical. Should the if statement
> >>> be removed?
> 5611 if (s != yajl_gen_status_ok)
> 5612 goto out;
> 5613 out:
> 5614 return s;
This is a false positive arising from autogenerated code. The
autogenerated code naturally has a completely systematic error
handling pattern, which leads to it sometimes doing this:
if (error)
goto out;
out:
/* exit path */
Is there a way to arrange to always persuade Coverity that this is
intentional ?
Thanks,
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] New Defects reported by Coverity Scan for XenProject
On 25/02/16 10:00, Ian Campbell wrote:
>
> On Wed, 2016-02-24 at 21:02 -0800, scan-ad...@coverity.com 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.
Yes, they're certainly redundant, and I definitely prefer the latter
check rather than the former. I'll send a patch.
-George
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] New Defects reported by Coverity Scan for XenProject
On Wed, 2016-02-24 at 21:02 -0800, scan-ad...@coverity.com 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.campb...@citrix.com",
> click
> https://scan.coverity.com/subscriptions/edit?email=ian.campbell%40citrix.com=86eecf9a70a32d8ef6ac41e4c7cdaf58
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] New Defects reported by Coverity Scan for XenProject
On 4/2/2015 at 08:32 AM, Ian Campbell ian.campb...@citrix.com wrote: Hi Charles, I'm not sure if this is a real issue in the qdisk support for xenstat, but I think it may relate to the first return 0 in qmp_read which may not free the accumulated array. Yes. That is a leak on the failure case. Could you take a look please? Patch sent, Thanks - Charles Ian. On Wed, 2015-04-01 at 05:51 -0700, scan-ad...@coverity.com wrote: Hi, Please find the latest report on new defect(s) introduced to XenProject found with Coverity Scan. 1 new defect(s) introduced to XenProject found with Coverity Scan. 2 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 1 of 1 defect(s) ** CID 1292691: Resource leaks (RESOURCE_LEAK) /tools/xenstat/libxenstat/src/xenstat_qmp.c: 327 in qmp_query() _ ___ *** CID 1292691: Resource leaks (RESOURCE_LEAK) /tools/xenstat/libxenstat/src/xenstat_qmp.c: 327 in qmp_query() 321 int n; 322 323 n = strlen(cmd); 324 if (qmp_write(qfd, cmd, n) != n) 325 return NULL; 326 if (!qmp_read(qfd, qstats)) CID 1292691: Resource leaks (RESOURCE_LEAK) Variable qstats going out of scope leaks the storage it points to. 327 return NULL; 328 return qstats; 329 } 330 331 /* Returns a socket connected to the QMP socket. Returns -1 on failure. */ 332 static int qmp_connect(char *path) _ ___ To view the defects in Coverity Scan visit, https://scan.coverity.com/projects/606?tab=overview To manage Coverity Scan email notifications for ian.campb...@citrix.com, click https://scan.coverity.com/subscriptions/edit?email=ian.campbell%40citrix.comt oken=1ce0fc428b9f94f66fd8d1ecf6cbb76a . ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] New Defects reported by Coverity Scan for XenProject
Hi Charles, I'm not sure if this is a real issue in the qdisk support for xenstat, but I think it may relate to the first return 0 in qmp_read which may not free the accumulated array. Could you take a look please? Ian. On Wed, 2015-04-01 at 05:51 -0700, scan-ad...@coverity.com wrote: Hi, Please find the latest report on new defect(s) introduced to XenProject found with Coverity Scan. 1 new defect(s) introduced to XenProject found with Coverity Scan. 2 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 1 of 1 defect(s) ** CID 1292691: Resource leaks (RESOURCE_LEAK) /tools/xenstat/libxenstat/src/xenstat_qmp.c: 327 in qmp_query() *** CID 1292691: Resource leaks (RESOURCE_LEAK) /tools/xenstat/libxenstat/src/xenstat_qmp.c: 327 in qmp_query() 321 int n; 322 323 n = strlen(cmd); 324 if (qmp_write(qfd, cmd, n) != n) 325 return NULL; 326 if (!qmp_read(qfd, qstats)) CID 1292691: Resource leaks (RESOURCE_LEAK) Variable qstats going out of scope leaks the storage it points to. 327 return NULL; 328 return qstats; 329 } 330 331 /* Returns a socket connected to the QMP socket. Returns -1 on failure. */ 332 static int qmp_connect(char *path) To view the defects in Coverity Scan visit, https://scan.coverity.com/projects/606?tab=overview To manage Coverity Scan email notifications for ian.campb...@citrix.com, click https://scan.coverity.com/subscriptions/edit?email=ian.campbell%40citrix.comtoken=1ce0fc428b9f94f66fd8d1ecf6cbb76a . ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
