From: Bandan Das <bsd@redhat.com>
During a write, free up the "path" before getting more data.
Also, while we at it, remove the confusing usage of d->fd for
storing mkdir status
Spotted by Coverity: CID 1398642
Signed-off-by: Bandan Das <bsd@redhat.com>
Message-id: 20190306210409.14842-3-bsd@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
hw/usb/dev-mtp.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 4dde14fc7887..1f22284949df 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1605,7 +1605,7 @@ static int usb_mtp_update_object(MTPObject *parent, char *name)
return ret;
}
-static void usb_mtp_write_data(MTPState *s)
+static int usb_mtp_write_data(MTPState *s)
{
MTPData *d = s->data_out;
MTPObject *parent =
@@ -1613,6 +1613,7 @@ static void usb_mtp_write_data(MTPState *s)
char *path = NULL;
uint64_t rc;
mode_t mask = 0644;
+ int ret = 0;
assert(d != NULL);
@@ -1621,13 +1622,13 @@ static void usb_mtp_write_data(MTPState *s)
if (!parent || !s->write_pending) {
usb_mtp_queue_result(s, RES_INVALID_OBJECTINFO, d->trans,
0, 0, 0, 0);
- return;
+ return 1;
}
if (s->dataset.filename) {
path = g_strdup_printf("%s/%s", parent->path, s->dataset.filename);
if (s->dataset.format == FMT_ASSOCIATION) {
- d->fd = mkdir(path, mask);
+ ret = mkdir(path, mask);
goto free;
}
d->fd = open(path, O_CREAT | O_WRONLY |
@@ -1657,7 +1658,8 @@ static void usb_mtp_write_data(MTPState *s)
goto done;
}
if (d->write_status != WRITE_END) {
- return;
+ g_free(path);
+ return ret;
} else {
/*
* Return an incomplete transfer if file size doesn't match
@@ -1685,12 +1687,14 @@ done:
*/
if (d->fd != -1) {
close(d->fd);
+ d->fd = -1;
}
free:
g_free(s->dataset.filename);
s->dataset.size = 0;
g_free(path);
s->write_pending = false;
+ return ret;
}
static void usb_mtp_write_metadata(MTPState *s, uint64_t dlen)
@@ -1727,14 +1731,12 @@ static void usb_mtp_write_metadata(MTPState *s, uint64_t dlen)
s->write_pending = true;
if (s->dataset.format == FMT_ASSOCIATION) {
- usb_mtp_write_data(s);
- /* next_handle will be allocated to the newly created dir */
- if (d->fd == -1) {
+ if (usb_mtp_write_data(s)) {
+ /* next_handle will be allocated to the newly created dir */
usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
0, 0, 0, 0);
return;
}
- d->fd = -1;
}
usb_mtp_queue_result(s, RES_OK, d->trans, 3, QEMU_STORAGE_ID,
--
2.18.1
On Thu, 7 Mar 2019 at 09:58, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> From: Bandan Das <bsd@redhat.com>
>
> During a write, free up the "path" before getting more data.
> Also, while we at it, remove the confusing usage of d->fd for
> storing mkdir status
>
> Spotted by Coverity: CID 1398642
>
> Signed-off-by: Bandan Das <bsd@redhat.com>
> Message-id: 20190306210409.14842-3-bsd@redhat.com
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Hi; Coverity found an issue with the code change here
(CID 1399415):
> index 4dde14fc7887..1f22284949df 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -1605,7 +1605,7 @@ static int usb_mtp_update_object(MTPObject *parent, char *name)
> return ret;
> }
>
> -static void usb_mtp_write_data(MTPState *s)
> +static int usb_mtp_write_data(MTPState *s)
Here we change usb_mtp_write_data() to return an error code...
> {
> MTPData *d = s->data_out;
> MTPObject *parent =
> @@ -1727,14 +1731,12 @@ static void usb_mtp_write_metadata(MTPState *s, uint64_t dlen)
> s->write_pending = true;
>
> if (s->dataset.format == FMT_ASSOCIATION) {
> - usb_mtp_write_data(s);
> - /* next_handle will be allocated to the newly created dir */
> - if (d->fd == -1) {
> + if (usb_mtp_write_data(s)) {
> + /* next_handle will be allocated to the newly created dir */
> usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
> 0, 0, 0, 0);
> return;
...and we updated this callsite to check the error return value.
But the two places in usb_mtp_get_data() that call
usb_mtp_write_metadata() still don't check its return
value: don't they need to handle failure too?
thanks
-- PMM
Peter Maydell <peter.maydell@linaro.org> writes:
> On Thu, 7 Mar 2019 at 09:58, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>
>> From: Bandan Das <bsd@redhat.com>
>>
>> During a write, free up the "path" before getting more data.
>> Also, while we at it, remove the confusing usage of d->fd for
>> storing mkdir status
>>
>> Spotted by Coverity: CID 1398642
>>
>> Signed-off-by: Bandan Das <bsd@redhat.com>
>> Message-id: 20190306210409.14842-3-bsd@redhat.com
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>
> Hi; Coverity found an issue with the code change here
> (CID 1399415):
>
>> index 4dde14fc7887..1f22284949df 100644
>> --- a/hw/usb/dev-mtp.c
>> +++ b/hw/usb/dev-mtp.c
>> @@ -1605,7 +1605,7 @@ static int usb_mtp_update_object(MTPObject *parent, char *name)
>> return ret;
>> }
>>
>> -static void usb_mtp_write_data(MTPState *s)
>> +static int usb_mtp_write_data(MTPState *s)
>
> Here we change usb_mtp_write_data() to return an error code...
>
>> {
>> MTPData *d = s->data_out;
>> MTPObject *parent =
>
>> @@ -1727,14 +1731,12 @@ static void usb_mtp_write_metadata(MTPState *s, uint64_t dlen)
>> s->write_pending = true;
>>
>> if (s->dataset.format == FMT_ASSOCIATION) {
>> - usb_mtp_write_data(s);
>> - /* next_handle will be allocated to the newly created dir */
>> - if (d->fd == -1) {
>> + if (usb_mtp_write_data(s)) {
>> + /* next_handle will be allocated to the newly created dir */
>> usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
>> 0, 0, 0, 0);
>> return;
>
> ...and we updated this callsite to check the error return value.
>
> But the two places in usb_mtp_get_data() that call
> usb_mtp_write_metadata() still don't check its return
> value: don't they need to handle failure too?
>
I believe this is ok because:
The return value of usb_mtp_write_data is only used to check if mkdir
failed and update s->result in usb_mtp_write_metadata().
The next time usb_mtp_handle_data is called, it will process s->result.
Bandan
> thanks
> -- PMM
On Fri, 8 Mar 2019 at 19:39, Bandan Das <bsd@redhat.com> wrote: > > Peter Maydell <peter.maydell@linaro.org> writes: > > But the two places in usb_mtp_get_data() that call > > usb_mtp_write_metadata() still don't check its return > > value: don't they need to handle failure too? > > > I believe this is ok because: > The return value of usb_mtp_write_data is only used to check if mkdir > failed and update s->result in usb_mtp_write_metadata(). > The next time usb_mtp_handle_data is called, it will process s->result. I think I still don't really understand the error handling in this function. Why do we deal with mkdir() failing by having the function return -1 and then doing usb_mtp_queue_result(s, RES_STORE_FULL, ...) (but only at one callsite), whereas for open() or write() failing we do the usb_mtp_queue_result(s, RES_STORE_FULL, ...) inside the function itself? thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes:
> On Fri, 8 Mar 2019 at 19:39, Bandan Das <bsd@redhat.com> wrote:
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>> > But the two places in usb_mtp_get_data() that call
>> > usb_mtp_write_metadata() still don't check its return
>> > value: don't they need to handle failure too?
>> >
>> I believe this is ok because:
>> The return value of usb_mtp_write_data is only used to check if mkdir
>> failed and update s->result in usb_mtp_write_metadata().
>> The next time usb_mtp_handle_data is called, it will process s->result.
>
> I think I still don't really understand the error handling
> in this function. Why do we deal with mkdir() failing by
> having the function return -1 and then doing
> usb_mtp_queue_result(s, RES_STORE_FULL, ...)
> (but only at one callsite), whereas for open() or write()
> failing we do the usb_mtp_queue_result(s, RES_STORE_FULL, ...)
> inside the function itself?
>
usb_mtp_write_metadata() handles the sendobjectinfo phase where the
initiator sends the metadata associated with the incoming object.
For a file, the name and the size is sent and once the responder sends
back OK, the initiator starts the sendobject phase. For a folder,
the name of the folder is sent with size being 0, and
no sendobject phase follows.
So, the reason I am sending back the return
value is because for a folder, I want to send a success or a failure
based on whether mkdir succeeded but for a file object, I want to return
success so that the next phase can continue. Is this rewrite better ?
static void usb_mtp_object_delete(MTPState *s, uint32_t handle,
@@ -1674,7 +1666,13 @@ static void usb_mtp_write_data(MTPState *s)
if (s->dataset.filename) {
path = g_strdup_printf("%s/%s", parent->path, s->dataset.filename);
if (s->dataset.format == FMT_ASSOCIATION) {
- d->fd = mkdir(path, mask);
+ if (mkdir(path, mask)) {
+ usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
+ 0, 0, 0, 0);
+ } else {
+ usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
+ 0, 0, 0, 0);
+ }
goto free;
}
d->fd = open(path, O_CREAT | O_WRONLY | O_CLOEXEC | O_NOFOLLOW, mask);
@@ -1769,17 +1767,10 @@ static void usb_mtp_write_metadata(MTPState *s, uint64_t dlen)
if (s->dataset.format == FMT_ASSOCIATION) {
usb_mtp_write_data(s);
- /* next_handle will be allocated to the newly created dir */
- if (d->fd == -1) {
- usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
- 0, 0, 0, 0);
- return;
- }
- d->fd = -1;
+ } else {
+ usb_mtp_queue_result(s, RES_OK, d->trans, 3, QEMU_STORAGE_ID,
+ s->dataset.parent_handle, next_handle);
}
-
- usb_mtp_queue_result(s, RES_OK, d->trans, 3, QEMU_STORAGE_ID,
- s->dataset.parent_handle, next_handle);
}
static void usb_mtp_get_data(MTPState *s, mtp_container *container,
> thanks
> -- PMM
On Fri, 15 Mar 2019 at 15:49, Bandan Das <bsd@redhat.com> wrote:
> usb_mtp_write_metadata() handles the sendobjectinfo phase where the
> initiator sends the metadata associated with the incoming object.
> For a file, the name and the size is sent and once the responder sends
> back OK, the initiator starts the sendobject phase. For a folder,
> the name of the folder is sent with size being 0, and
> no sendobject phase follows.
Thanks for the explanation.
> So, the reason I am sending back the return
> value is because for a folder, I want to send a success or a failure
> based on whether mkdir succeeded but for a file object, I want to return
> success so that the next phase can continue. Is this rewrite better ?
>
> static void usb_mtp_object_delete(MTPState *s, uint32_t handle,
> @@ -1674,7 +1666,13 @@ static void usb_mtp_write_data(MTPState *s)
> if (s->dataset.filename) {
> path = g_strdup_printf("%s/%s", parent->path, s->dataset.filename);
> if (s->dataset.format == FMT_ASSOCIATION) {
> - d->fd = mkdir(path, mask);
> + if (mkdir(path, mask)) {
> + usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
> + 0, 0, 0, 0);
> + } else {
> + usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
> + 0, 0, 0, 0);
> + }
Presumably one of these should be RES_OK of some kind ?
> goto free;
> }
It does seem like a more consistent approach if we can say
"usb_mtp_write_data() will always queue an appropriate result"
rather than having it do that for some use cases and not others,
so I like this suggestion.
thanks
-- PMM
Peter Maydell <peter.maydell@linaro.org> writes:
...
>> + } else {
>> + usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
>> + 0, 0, 0, 0);
>> + }
>
> Presumably one of these should be RES_OK of some kind ?
>
Ah, yes, that's a typo.
>> goto free;
>> }
>
> It does seem like a more consistent approach if we can say
> "usb_mtp_write_data() will always queue an appropriate result"
> rather than having it do that for some use cases and not others,
> so I like this suggestion.
>
Sounds good, I will send a patch.
> thanks
> -- PMM
© 2016 - 2025 Red Hat, Inc.