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