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

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

[Bandan Das]

Fix some coverity issues and a few others pointed out by Peter

Bandan Das (2):
  usb-mtp: return incomplete transfer on a lstat failure
  usb-mtp: fix some usb_mtp_write_data return paths

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

-- 
2.19.2

[Qemu-devel] [PATCH 1/2] 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 | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index f1d20fa1b9..79f2cb8743 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1644,14 +1644,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)
@@ -1708,14 +1712,19 @@ 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 2/2] 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 79f2cb8743..4c1c53d330 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1658,7 +1658,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 =
@@ -1666,6 +1666,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);
 
@@ -1674,13 +1675,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 |
@@ -1710,7 +1711,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
@@ -1739,12 +1741,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)
@@ -1781,14 +1785,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

Re: [Qemu-devel] [PATCH 0/2] usb-mtp coverity fixes

[Gerd Hoffmann]

On Fri, Feb 22, 2019 at 05:49:32PM -0500, Bandan Das wrote:
> Fix some coverity issues and a few others pointed out by Peter
> 
> Bandan Das (2):
>   usb-mtp: return incomplete transfer on a lstat failure
>   usb-mtp: fix some usb_mtp_write_data return paths

Series needs a rebase (conflicts with Daniel's inotify patches).

cheers,
  Gerd