[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PULL 7/8] usb-mtp: breakup MTP write into smaller chun
From: |
Bandan Das |
Subject: |
Re: [Qemu-devel] [PULL 7/8] usb-mtp: breakup MTP write into smaller chunks |
Date: |
Fri, 15 Feb 2019 13:45:51 -0500 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
Peter Maydell <address@hidden> writes:
> On Wed, 30 Jan 2019 at 07:41, Gerd Hoffmann <address@hidden> wrote:
>>
>> From: Bandan Das <address@hidden>
>>
>> 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 <address@hidden>
>> Signed-off-by: Bandan Das <address@hidden>
>> Message-id: address@hidden
>> Signed-off-by: Gerd Hoffmann <address@hidden>
>
> 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