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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

scan-admin@xxxxxxxxxxxx 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(&egc->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(&egc->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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel