qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PULL 4/5] usb-mtp: Introduce write support for MTP obj


From: Bandan Das
Subject: Re: [Qemu-devel] [PULL 4/5] usb-mtp: Introduce write support for MTP objects
Date: Mon, 30 Apr 2018 13:56:10 -0400
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux)

Peter Maydell <address@hidden> writes:

> On 27 February 2018 at 08:39, Gerd Hoffmann <address@hidden> wrote:
>> From: Bandan Das <address@hidden>
>>
>> Allow write operations on behalf of the initiator. The
>> precursor to write is the sending of the write metadata
>> that consists of the ObjectInfo dataset. This patch introduces
>> a flag that is set when the responder is ready to receive
>> write data based on a previous SendObjectInfo operation by
>> the initiator (The SendObjectInfo implementation is in a
>> later patch)
>>
>> Signed-off-by: Bandan Das <address@hidden>
>> Message-id: address@hidden
>> Signed-off-by: Gerd Hoffmann <address@hidden>
>
> Hi. Coverity (CID 1390604) complains here about a possible
> use-after-NULL-check:
>
>> @@ -1567,8 +1706,13 @@ static void usb_mtp_handle_data(USBDevice *dev, 
>> USBPacket *p)
>>              p->status = USB_RET_STALL;
>>              return;
>>          }
>> -        usb_packet_copy(p, &container, sizeof(container));
>> -        switch (le16_to_cpu(container.type)) {
>> +        if (s->data_out && !s->data_out->first) {
>> +            container_type = TYPE_DATA;
>
> Here we check s->data_out, so we might set container_type
> to TYPE_DATA with s->data_out == NULL...

Just to be sure, is it enough to replace the if with:
if (s->data !=NULL && !s->data->first) ?

Bandan

>> +        } else {
>> +            usb_packet_copy(p, &container, sizeof(container));
>> +            container_type = le16_to_cpu(container.type);
>> +        }
>> +        switch (container_type) {
>>          case TYPE_COMMAND:
>>              if (s->data_in || s->data_out || s->result) {
>>                  trace_usb_mtp_stall(s->dev.addr, "transaction inflight");
>> @@ -1599,6 +1743,15 @@ static void usb_mtp_handle_data(USBDevice *dev, 
>> USBPacket *p)
>>                                    (cmd.argc > 4) ? cmd.argv[4] : 0);
>>              usb_mtp_command(s, &cmd);
>>              break;
>> +        case TYPE_DATA:
>> +            /* One of the previous transfers has already errored but the
>> +             * responder is still sending data associated with it
>> +             */
>> +            if (s->result != NULL) {
>> +                return;
>> +            }
>> +            usb_mtp_get_data(s, &container, p);
>
> ...but here we call usb_mtp_get_data(), which will always
> dereference s->data_out, and so will crash if it is NULL.
>
>> +            break;
>>          default:
>>              /* not needed as long as the mtp device is read-only */
>>              p->status = USB_RET_STALL;
>> --
>> 2.9.3
>
> thanks
> -- PMM



reply via email to

[Prev in Thread] Current Thread [Next in Thread]