On 7 May 2018 at 10:44, Gerd Hoffmann <kra...@redhat.com> wrote: > From: Bandan Das <b...@redhat.com> > > CID 1390578: In usb_mtp_write_metadata, parent can never be NULL but > just in case, add an assert > CID 1390592: Check for o->format only if o !=NULL > CID 1390604: Check s->data_out != NULL in usb_mtp_handle_data
> @@ -1838,7 +1838,7 @@ static void usb_mtp_handle_data(USBDevice *dev, > USBPacket *p) > p->status = USB_RET_STALL; > return; > } > - if (s->data_out && !s->data_out->first) { > + if ((s->data_out != NULL) && !s->data_out->first) { > container_type = TYPE_DATA; > } else { > usb_packet_copy(p, &container, sizeof(container)); > -- I just noticed this, but this isn't actually changing the logic in this function: "s->data_out" and "s->data_out != NULL" are exactly the same test. So this won't change CID 1390604. Looking back at my previous email on this, it looks like I managed to completely misinterpret the code and/or what coverity is talking about, which is probably the source of the confusion. Let me try again... In the function usb_mtp_handle_data() EP_DATA_OUT case, we have: (1) a check for whether s->data_out is NULL, which implies that it might be NULL sometimes (2) some time later, a call to usb_mtp_get_data() which is not protected by a test for whether s->data_out is NULL and if s->data_out is NULL at point (2) then usb_mtp_get_data() will crash. Obviously the code path that goes through "containe_type = TYPE_DATA" must have s->data_out being non-NULL. But in the else branch of that, can the container_type we get via usb_packet_copy() ever be TYPE_DATA ? (It's not clear to me whether that comes from a trusted or untrusted source.) If this is a "can't happen" situation we can mark it as a false positive in coverity. thanks -- PMM