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

[Bandan Das]

Peter Maydell <address@hidden> writes:

> On Fri, 15 Feb 2019 at 18:45, Bandan Das <address@hidden> wrote:
>>
...
>> 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.
>
> Looking at the code I think Coverity is correct about the 'path'
> string. We allocate this with
>             path = g_strdup_printf("%s/%s", parent->path, 
> s->dataset.filename);
> and then use it in a mkdir() and an open() call, and never
> save the pointer anywhere. But we only g_free() it in the
> exit paths that go through "free:".
>
Thanks for the catch! Indeed, it's not being saved anywhere. I assumed
without looking it's d->path but we don't need path anywhere else, so I guess
your solution below is fair.

> One simple fix to this would be to narrow the scope of 'path',
> so we deal with it only inside the if():
>
>         if (s->dataset.filename) {
>             char *path = g_strdup_printf("%s/%s", parent->path,
> s->dataset.filename);
>             if (s->dataset.format == FMT_ASSOCIATION) {
>                 d->fd = mkdir(path, mask);
>                 g_free(path);
>                 goto free;
>             }
>             d->fd = open(path, O_CREAT | O_WRONLY |
>                          O_CLOEXEC | O_NOFOLLOW, mask);
>             g_free(path);
>             if (d->fd == -1) {
>                 usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
>                                      0, 0, 0, 0);
>                 goto done;
>             }
>             [...]
>
> and then drop the g_free(path) from the end of the function.
>
>
> While I'm looking at this, that call to mkdir() looks bogus:
>                 d->fd = mkdir(path, mask);
> mkdir() does not return a file descriptor, it returns a
> 0-or-negative status code (which we are not checking),
> so storing its result into d->fd looks very weird. If
> we ever do try to treat d->fd as an fd later on then we
> will end up operating on stdin, which is going to have
> very confusing effects.
>
Agreed, this is incorrect and confusing. I will add a test
for mkdir. I will post a patch for this as well as set
d->fd to -1. Thanks for taking a look.

Bandan

>
> If the MTPState* is kept around and reused, should the
> cleanup code that does close(d->fd) also set d->fd to -1,
> so that we know that the fd has been closed and don't
> later try to close it twice or otherwise use it ?
>
> thanks
> -- PMM