Re: [Qemu-devel] [PULL 7/8] usb-mtp: breakup MTP write into smaller chunks

[Bandan Das <bsd@redhat.com>]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 30 Jan 2019 at 07:41, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>
>> From: Bandan Das <bsd@redhat.com>
>>
>> For every MTP_WRITE_BUF_SZ copied, this patch writes it to file before
>> getting the next block of data. The file is kept opened for the
>> duration of the operation but the sanity checks on the write operation
>> are performed only once when the write operation starts. Additionally,
>> we also update the file size in the object metadata once the file has
>> completely been written.
>>
>> Suggested-by: Gerd Hoffman <kraxel@redhat.com>
>> Signed-off-by: Bandan Das <bsd@redhat.com>
>> Message-id: 20190129131908.27924-3-bsd@redhat.com
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>
> Hi; Coverity has spotted a couple of issues with this patch:
>
>
>> +static void usb_mtp_update_object(MTPObject *parent, char *name)
>> +{
>> +    MTPObject *o =
>> +        usb_mtp_object_lookup_name(parent, name, strlen(name));
>> +
>> +    if (o) {
>> +        lstat(o->path, &o->stat);
>
> CID 1398651: We don't check the return value of this lstat() for failure.
>

Thanks, will post a patch for this.

>> +    }
>> +}
>> +
>>  static void usb_mtp_write_data(MTPState *s)
>>  {
>>      MTPData *d = s->data_out;
>
> [...]
>
>> +    case WRITE_CONTINUE:
>> +    case WRITE_END:
>> +        rc = write_retry(d->fd, d->data, d->data_offset,
>> +                         d->offset - d->data_offset);
>> +        if (rc != d->data_offset) {
>>              usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
>>                                   0, 0, 0, 0);
>>              goto done;
>> +        }
>> +        if (d->write_status != WRITE_END) {
>> +            return;
>
> CID 1398642: This early-return case in usb_mtp_write_data() returns
> from the function without doing any of the cleanup (closing file,
> freeing data, etc). Possibly it should be "goto done;" instead ?
> The specific thing Coverity complains about is the memory pointed
> to by "path".
>

I believe this is a false positive, there's still more data incoming
and we have successfully written the data we got this time, so we return
without freeing up any of the structures. I will add a comment here.

Bandan

> thanks
> -- PMM