[Qemu-devel] [PATCH v2 0/3] usb-mtp coverity fixes

[Qemu-devel] [PATCH v2 0/3] usb-mtp coverity fixes

[Bandan Das]

v2:
  Rebase
  Prevent a null dereference when deleting objects
Fix some coverity issues and a few others pointed out by Peter

Bandan Das (3):
  usb-mtp: return incomplete transfer on a lstat failure
  usb-mtp: fix some usb_mtp_write_data return paths
  usb-mtp: prevent null dereference while deleting objects

 hw/usb/dev-mtp.c | 41 +++++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 16 deletions(-)

-- 
2.19.2

[Qemu-devel] [PATCH v2 1/3] usb-mtp: return incomplete transfer on a lstat failure

[Bandan Das]

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

[Qemu-devel] [PATCH v2 2/3] usb-mtp: fix some usb_mtp_write_data return paths

[Bandan Das]

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

[Qemu-devel] [PATCH v2 3/3] usb-mtp: prevent null dereference while deleting objects

[Bandan Das]

Spotted by Coverity: CID 1399144

Signed-off-by: Bandan Das <bsd@redhat.com>
---
 hw/usb/dev-mtp.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 1f22284949..06e376bcd2 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1177,9 +1177,7 @@ static int usb_mtp_deletefn(MTPState *s, MTPObject *o, uint32_t trans)
             usb_mtp_object_free_one(s, o);
             success = true;
         }
-    }
-
-    if (o->format == FMT_ASSOCIATION) {
+    } else if (o->format == FMT_ASSOCIATION) {
         if (rmdir(o->path)) {
             partial_delete = true;
         } else {
-- 
2.19.2