From: Bandan Das <bsd@redhat.com>
For every MTP_WRITE_BUF_SZ copied, this patch writes it to file before
getting the next block of data. The file is kept opened for the
duration of the operation but the sanity checks on the write operation
are performed only once when the write operation starts. Additionally,
we also update the file size in the object metadata once the file has
completely been written.
Suggested-by: Gerd Hoffman <kraxel@redhat.com>
Signed-off-by: Bandan Das <bsd@redhat.com>
Message-id: 20190129131908.27924-3-bsd@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
hw/usb/dev-mtp.c | 134 +++++++++++++++++++++++++++++++++++++------------------
1 file changed, 91 insertions(+), 43 deletions(-)
diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 2e342c7745..2ca25c9da5 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -36,6 +36,13 @@ enum mtp_container_type {
TYPE_EVENT = 4,
};
+/* MTP write stage, for internal use only */
+enum mtp_write_status {
+ WRITE_START = 1,
+ WRITE_CONTINUE = 2,
+ WRITE_END = 3,
+};
+
enum mtp_code {
/* command codes */
CMD_GET_DEVICE_INFO = 0x1001,
@@ -154,6 +161,9 @@ struct MTPData {
/* Used for >4G file sizes */
bool pending;
int fd;
+ uint8_t write_status;
+ /* Internal pointer per every MTP_WRITE_BUF_SZ */
+ uint64_t data_offset;
};
struct MTPObject {
@@ -1620,10 +1630,14 @@ static char *utf16_to_str(uint8_t len, uint16_t *arr)
}
/* Wrapper around write, returns 0 on failure */
-static uint64_t write_retry(int fd, void *buf, uint64_t size)
+static uint64_t write_retry(int fd, void *buf, uint64_t size, off_t offset)
{
uint64_t bytes_left = size, ret;
+ if (lseek(fd, offset, SEEK_SET) < 0) {
+ goto done;
+ }
+
while (bytes_left > 0) {
ret = write(fd, buf, bytes_left);
if ((ret == -1) && (errno != EINTR || errno != EAGAIN ||
@@ -1634,9 +1648,20 @@ static uint64_t write_retry(int fd, void *buf, uint64_t size)
buf += ret;
}
+done:
return size - bytes_left;
}
+static void usb_mtp_update_object(MTPObject *parent, char *name)
+{
+ MTPObject *o =
+ usb_mtp_object_lookup_name(parent, name, strlen(name));
+
+ if (o) {
+ lstat(o->path, &o->stat);
+ }
+}
+
static void usb_mtp_write_data(MTPState *s)
{
MTPData *d = s->data_out;
@@ -1648,48 +1673,56 @@ static void usb_mtp_write_data(MTPState *s)
assert(d != NULL);
- if (parent == NULL || !s->write_pending) {
- usb_mtp_queue_result(s, RES_INVALID_OBJECTINFO, d->trans,
- 0, 0, 0, 0);
+ switch (d->write_status) {
+ case WRITE_START:
+ if (!parent || !s->write_pending) {
+ usb_mtp_queue_result(s, RES_INVALID_OBJECTINFO, d->trans,
+ 0, 0, 0, 0);
return;
- }
-
- 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);
- goto free;
- }
- if ((s->dataset.size != 0xFFFFFFFF) && (s->dataset.size != d->offset)) {
- usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
- 0, 0, 0, 0);
- goto done;
- }
- d->fd = open(path, O_CREAT | O_WRONLY | O_CLOEXEC | O_NOFOLLOW, mask);
- if (d->fd == -1) {
- usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
- 0, 0, 0, 0);
- goto done;
}
- /*
- * Return success if initiator sent 0 sized data
- */
- if (!s->dataset.size) {
- goto success;
- }
+ 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);
+ goto free;
+ }
+ d->fd = open(path, O_CREAT | O_WRONLY |
+ O_CLOEXEC | O_NOFOLLOW, mask);
+ if (d->fd == -1) {
+ usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
+ 0, 0, 0, 0);
+ goto done;
+ }
- rc = write_retry(d->fd, d->data, d->offset);
- if (rc != d->offset) {
+ /* Return success if initiator sent 0 sized data */
+ if (!s->dataset.size) {
+ goto success;
+ }
+ if (d->length != MTP_WRITE_BUF_SZ && !d->pending) {
+ d->write_status = WRITE_END;
+ }
+ }
+ /* fall through */
+ case WRITE_CONTINUE:
+ case WRITE_END:
+ rc = write_retry(d->fd, d->data, d->data_offset,
+ d->offset - d->data_offset);
+ if (rc != d->data_offset) {
usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
0, 0, 0, 0);
goto done;
+ }
+ if (d->write_status != WRITE_END) {
+ return;
+ } else {
+ /* Only for < 4G file sizes */
+ if (s->dataset.size != 0xFFFFFFFF && d->offset != s->dataset.size) {
+ usb_mtp_queue_result(s, RES_INCOMPLETE_TRANSFER, d->trans,
+ 0, 0, 0, 0);
+ goto done;
}
- /* Only for < 4G file sizes */
- if (s->dataset.size != 0xFFFFFFFF && rc != s->dataset.size) {
- usb_mtp_queue_result(s, RES_INCOMPLETE_TRANSFER, d->trans,
- 0, 0, 0, 0);
- goto done;
+ usb_mtp_update_object(parent, s->dataset.filename);
}
}
@@ -1788,25 +1821,33 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container,
d->offset = 0;
d->first = false;
d->pending = false;
+ d->data_offset = 0;
+ d->write_status = WRITE_START;
}
if (d->pending) {
- usb_mtp_realloc(d, MTP_WRITE_BUF_SZ);
- d->length += MTP_WRITE_BUF_SZ;
+ memset(d->data, 0, d->length);
+ if (d->length != MTP_WRITE_BUF_SZ) {
+ usb_mtp_realloc(d, MTP_WRITE_BUF_SZ - d->length);
+ d->length += (MTP_WRITE_BUF_SZ - d->length);
+ }
d->pending = false;
+ d->write_status = WRITE_CONTINUE;
+ d->data_offset = 0;
}
- if (d->length - d->offset > data_len) {
+ if (d->length - d->data_offset > data_len) {
dlen = data_len;
} else {
- dlen = d->length - d->offset;
+ dlen = d->length - d->data_offset;
}
switch (d->code) {
case CMD_SEND_OBJECT_INFO:
- usb_packet_copy(p, d->data + d->offset, dlen);
+ usb_packet_copy(p, d->data + d->data_offset, dlen);
d->offset += dlen;
- if (d->offset == d->length) {
+ d->data_offset += dlen;
+ if (d->data_offset == d->length) {
/* The operation might have already failed */
if (!s->result) {
usb_mtp_write_metadata(s, dlen);
@@ -1817,19 +1858,26 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container,
}
break;
case CMD_SEND_OBJECT:
- usb_packet_copy(p, d->data + d->offset, dlen);
+ usb_packet_copy(p, d->data + d->data_offset, dlen);
d->offset += dlen;
+ d->data_offset += dlen;
if ((p->iov.size % 64) || !p->iov.size) {
assert((s->dataset.size == 0xFFFFFFFF) ||
(s->dataset.size == d->offset));
+ if (d->length == MTP_WRITE_BUF_SZ) {
+ d->write_status = WRITE_END;
+ } else {
+ d->write_status = WRITE_START;
+ }
usb_mtp_write_data(s);
usb_mtp_data_free(s->data_out);
s->data_out = NULL;
return;
}
- if (d->offset == d->length) {
+ if (d->data_offset == d->length) {
d->pending = true;
+ usb_mtp_write_data(s);
}
break;
default:
--
2.9.3
On Wed, 30 Jan 2019 at 07:41, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> From: Bandan Das <bsd@redhat.com>
>
> For every MTP_WRITE_BUF_SZ copied, this patch writes it to file before
> getting the next block of data. The file is kept opened for the
> duration of the operation but the sanity checks on the write operation
> are performed only once when the write operation starts. Additionally,
> we also update the file size in the object metadata once the file has
> completely been written.
>
> Suggested-by: Gerd Hoffman <kraxel@redhat.com>
> Signed-off-by: Bandan Das <bsd@redhat.com>
> Message-id: 20190129131908.27924-3-bsd@redhat.com
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Hi; Coverity has spotted a couple of issues with this patch:
> +static void usb_mtp_update_object(MTPObject *parent, char *name)
> +{
> + MTPObject *o =
> + usb_mtp_object_lookup_name(parent, name, strlen(name));
> +
> + if (o) {
> + lstat(o->path, &o->stat);
CID 1398651: We don't check the return value of this lstat() for failure.
> + }
> +}
> +
> static void usb_mtp_write_data(MTPState *s)
> {
> MTPData *d = s->data_out;
[...]
> + case WRITE_CONTINUE:
> + case WRITE_END:
> + rc = write_retry(d->fd, d->data, d->data_offset,
> + d->offset - d->data_offset);
> + if (rc != d->data_offset) {
> usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
> 0, 0, 0, 0);
> goto done;
> + }
> + if (d->write_status != WRITE_END) {
> + return;
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".
thanks
-- PMM
Peter Maydell <peter.maydell@linaro.org> writes:
> On Wed, 30 Jan 2019 at 07:41, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>
>> From: Bandan Das <bsd@redhat.com>
>>
>> For every MTP_WRITE_BUF_SZ copied, this patch writes it to file before
>> getting the next block of data. The file is kept opened for the
>> duration of the operation but the sanity checks on the write operation
>> are performed only once when the write operation starts. Additionally,
>> we also update the file size in the object metadata once the file has
>> completely been written.
>>
>> Suggested-by: Gerd Hoffman <kraxel@redhat.com>
>> Signed-off-by: Bandan Das <bsd@redhat.com>
>> Message-id: 20190129131908.27924-3-bsd@redhat.com
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>
> Hi; Coverity has spotted a couple of issues with this patch:
>
>
>> +static void usb_mtp_update_object(MTPObject *parent, char *name)
>> +{
>> + MTPObject *o =
>> + usb_mtp_object_lookup_name(parent, name, strlen(name));
>> +
>> + if (o) {
>> + lstat(o->path, &o->stat);
>
> CID 1398651: We don't check the return value of this lstat() for failure.
>
Thanks, will post a patch for this.
>> + }
>> +}
>> +
>> static void usb_mtp_write_data(MTPState *s)
>> {
>> MTPData *d = s->data_out;
>
> [...]
>
>> + case WRITE_CONTINUE:
>> + case WRITE_END:
>> + rc = write_retry(d->fd, d->data, d->data_offset,
>> + d->offset - d->data_offset);
>> + if (rc != d->data_offset) {
>> usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
>> 0, 0, 0, 0);
>> goto done;
>> + }
>> + if (d->write_status != WRITE_END) {
>> + return;
>
> 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.
Bandan
> thanks
> -- PMM
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
Peter Maydell <peter.maydell@linaro.org> writes:
> On Fri, 15 Feb 2019 at 18:45, Bandan Das <bsd@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
© 2016 - 2026 Red Hat, Inc.