* [Qemu-devel] [PATCH v2 1/3] usb-mtp: return incomplete transfer on a lstat failure
2019-03-06 21:04 [Qemu-devel] [PATCH v2 0/3] usb-mtp coverity fixes Bandan Das
@ 2019-03-06 21:04 ` Bandan Das
2019-03-06 21:04 ` [Qemu-devel] [PATCH v2 2/3] usb-mtp: fix some usb_mtp_write_data return paths Bandan Das
2019-03-06 21:04 ` [Qemu-devel] [PATCH v2 3/3] usb-mtp: prevent null dereference while deleting objects Bandan Das
2 siblings, 0 replies; 4+ messages in thread
From: Bandan Das @ 2019-03-06 21:04 UTC (permalink / raw)
To: qemu-devel; +Cc: kraxel, peter.maydell
MTP writes objects in small chunks and at the end gets the
real file size to update the object metadata. If this fails for
any reason, return an INCOMPLETE_TRANSFER to the initiator
Spotted by Coverity: CID 1398651
Signed-off-by: Bandan Das <bsd@redhat.com>
---
hw/usb/dev-mtp.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 4ee4fc5a89..4dde14fc78 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1591,14 +1591,18 @@ done:
return ret;
}
-static void usb_mtp_update_object(MTPObject *parent, char *name)
+static int usb_mtp_update_object(MTPObject *parent, char *name)
{
+ int ret = -1;
+
MTPObject *o =
usb_mtp_object_lookup_name(parent, name, strlen(name));
if (o) {
- lstat(o->path, &o->stat);
+ ret = lstat(o->path, &o->stat);
}
+
+ return ret;
}
static void usb_mtp_write_data(MTPState *s)
@@ -1655,13 +1659,18 @@ static void usb_mtp_write_data(MTPState *s)
if (d->write_status != WRITE_END) {
return;
} else {
- /* Only for < 4G file sizes */
- if (s->dataset.size != 0xFFFFFFFF && d->offset != s->dataset.size) {
+ /*
+ * Return an incomplete transfer if file size doesn't match
+ * for < 4G file or if lstat fails which will result in an incorrect
+ * file size
+ */
+ if ((s->dataset.size != 0xFFFFFFFF &&
+ d->offset != s->dataset.size) ||
+ usb_mtp_update_object(parent, s->dataset.filename)) {
usb_mtp_queue_result(s, RES_INCOMPLETE_TRANSFER, d->trans,
0, 0, 0, 0);
goto done;
}
- usb_mtp_update_object(parent, s->dataset.filename);
}
}
--
2.19.2
^ permalink raw reply related [flat|nested] 4+ messages in thread* [Qemu-devel] [PATCH v2 2/3] usb-mtp: fix some usb_mtp_write_data return paths
2019-03-06 21:04 [Qemu-devel] [PATCH v2 0/3] usb-mtp coverity fixes Bandan Das
2019-03-06 21:04 ` [Qemu-devel] [PATCH v2 1/3] usb-mtp: return incomplete transfer on a lstat failure Bandan Das
@ 2019-03-06 21:04 ` Bandan Das
2019-03-06 21:04 ` [Qemu-devel] [PATCH v2 3/3] usb-mtp: prevent null dereference while deleting objects Bandan Das
2 siblings, 0 replies; 4+ messages in thread
From: Bandan Das @ 2019-03-06 21:04 UTC (permalink / raw)
To: qemu-devel; +Cc: kraxel, peter.maydell
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>
---
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 4dde14fc78..1f22284949 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.19.2
^ permalink raw reply related [flat|nested] 4+ messages in thread