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

[Peter Maydell <peter.maydell@linaro.org>]

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.

> +    }
> +}
> +
>  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".

thanks
-- PMM