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