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

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

On Fri, 15 Feb 2019 at 18:45, Bandan Das <bsd@redhat.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
> > 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.

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:".

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.


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