[Qemu-devel] [PATCH for 2.10 v2 00/20] fix bugs reported by Clang Static Analyzer

[Qemu-devel] [PATCH for 2.10 v2 00/20] fix bugs reported by Clang Static Analyzer

[Philippe Mathieu-Daudé]

Hi,

This series is the result of [now NOT] having fun with Clang's Static Analyzer
(see https://clang-analyzer.llvm.org/).

v2:
- addressed review feedbacks,
- added various R-b,
- dropped noise (Peter sharp eye),
- dropped dup patches

Patches 1-13 are already reviewed,
Patches 14,15 address feedbacks from v1,
Patch 16 test if patch collecting tools can handle missing patches,
Patches 17-19 are new,
Patch 20 is new but not very important ;) bonus that can wait 2.11.

Regards,

Phil.

v1:

Patch 1 was in another series (delayed for 2.11), it would be nice to have it
in 2.10.

I ran Clang static analyzer "scan-build" via a docker image based on
debian/unstable to use bleeding code, if one is interested in reproduce or use
it the Dockerfile is available here:
http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg07487.html

I used the following commands:

$ ../configure --host-cc=clang-5.0 --cc=clang-5.0 --cxx=clang++-5.0 \
  --disable-docs --enable-debug
$ scan-build-5.0 -o testresults --keep-going -maxloop 2 -no-failure-reports \
  -analyzer-config stable-report-filename=true \
  -disable-checker alpha.clone.CloneChecker \
  -enable-checker alpha.core.CastSize \
  -enable-checker alpha.core.Conversion \
  -enable-checker alpha.core.IdenticalExpr \
  -enable-checker alpha.core.SizeofPtr \
  -disable-checker alpha.deadcode.UnreachableCode \
  -enable-checker alpha.security.ArrayBoundV2 \
  -enable-checker alpha.security.MallocOverflow \
  -enable-checker alpha.unix.cstring.BufferOverlap \
  -enable-checker alpha.unix.cstring.OutOfBounds \
  -disable-checker deadcode.DeadStores \
  -disable-checker optin.performance.Padding \
  -enable-checker optin.portability.UnixAPI \
  -disable-checker security.insecureAPI.getpw \
  -disable-checker security.insecureAPI.gets \
  -enable-checker security.insecureAPI.strcpy \
  -disable-checker unix.Vfork \
    make -k -j4

Philippe Mathieu-Daudé (20):
  tests: add missing dependency to build QTEST_QEMU_BINARY
  loader: check get_image_size() return value
  ivshmem: fix incorrect error handling in ivshmem_recv_msg()
  nbd: fix memory leak in nbd_opt_go()
  qcow2: fix null pointer dereference
  ui/vnc: fix leak of SocketAddress **
  net/eth: fix incorrect check of iov_to_buf() return value
  vfio/platform: fix use of freed memory
  vfio/pci: fix use of freed memory
  m68k/translate: fix incorrect copy/paste
  linux-user/sh4: fix incorrect memory write
  syscall: fix dereference of undefined pointer
  syscall: fix use of uninitialized values
  syscall: check inotify() and eventfd() return value
  thunk: assert nb_fields is valid
  bt-sdp: fix memory leak in sdp_service_record_build()
  9pfs: avoid sign conversion error simplifying the code
  spapr_vio: fix overflow of qdevs in spapr_dt_vdevice()
  i2c/exynos4210: fix write to I2CADD register, bit 0 is not mapped

 block/qcow2-refcount.c  |  4 +--
 hw/9pfs/9p.c            |  6 ++---
 hw/bt/sdp.c             |  6 +++--
 hw/core/loader.c        |  4 +--
 hw/i2c/exynos4210_i2c.c |  2 +-
 hw/misc/ivshmem.c       |  5 +++-
 hw/ppc/spapr_vio.c      |  4 +--
 hw/vfio/pci.c           | 11 ++++++---
 hw/vfio/platform.c      |  2 +-
 linux-user/elfload.c    |  2 +-
 linux-user/syscall.c    | 18 +++++++++++---
 nbd/client.c            |  8 +++---
 net/eth.c               |  4 +--
 target/m68k/translate.c |  2 +-
 tests/Makefile.include  |  2 +-
 thunk.c                 |  3 ++-
 ui/vnc.c                | 66 ++++++++++++++++++++++++-------------------------
 17 files changed, 83 insertions(+), 66 deletions(-)

-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 01/20] tests: add missing dependency to build QTEST_QEMU_BINARY

[Philippe Mathieu-Daudé]

This allow a one liner from fresh repository clone, i.e.:

  ./configure && make -j check-qtest-aarch64

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 tests/Makefile.include | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 7af278db55..b55fe39d94 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -830,7 +830,7 @@ GCOV_OPTIONS = -n $(if $(V),-f,)
 # gtester tests, possibly with verbose output
 
 .PHONY: $(patsubst %, check-qtest-%, $(QTEST_TARGETS))
-$(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: $(check-qtest-y)
+$(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: subdir-%-softmmu $(check-qtest-y)
 	$(if $(CONFIG_GCOV),@rm -f *.gcda */*.gcda */*/*.gcda */*/*/*.gcda,)
 	$(call quiet-command,QTEST_QEMU_BINARY=$*-softmmu/qemu-system-$* \
 		QTEST_QEMU_IMG=qemu-img$(EXESUF) \
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 02/20] loader: check get_image_size() return value

[Philippe Mathieu-Daudé]

since a negative value means it errored.

hw/core/loader.c:149:9: warning: Loss of sign in implicit conversion
    if (size > max_sz) {
        ^~~~
hw/core/loader.c:171:9: warning: Loss of sign in implicit conversion
    if (size > memory_region_size(mr)) {
        ^~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/core/loader.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index c17ace0a2e..4bb176f284 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -146,7 +146,7 @@ int load_image_targphys_as(const char *filename,
     int size;
 
     size = get_image_size(filename);
-    if (size > max_sz) {
+    if (size < 0 || size > max_sz) {
         return -1;
     }
     if (size > 0) {
@@ -168,7 +168,7 @@ int load_image_mr(const char *filename, MemoryRegion *mr)
 
     size = get_image_size(filename);
 
-    if (size > memory_region_size(mr)) {
+    if (size < 0 || size > memory_region_size(mr)) {
         return -1;
     }
     if (size > 0) {
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 03/20] ivshmem: fix incorrect error handling in ivshmem_recv_msg()

[Philippe Mathieu-Daudé]

Screwed up in commit 3a55fc0f, v2.6.0.

If qemu_chr_fe_read_all() returns -EINTR the do {} statement continues and the
n accumulator used to complete reads upto sizeof(msg) is decremented by 4 (the
value of EINTR on Linux).
To avoid that, use simpler if() statements and continue if EINTR occured.

hw/misc/ivshmem.c:650:14: warning: Loss of sign in implicit conversion
    } while (n < sizeof(msg));
             ^

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 hw/misc/ivshmem.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index a58f9ee579..47a015f072 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -642,7 +642,10 @@ static int64_t ivshmem_recv_msg(IVShmemState *s, int *pfd, Error **errp)
     do {
         ret = qemu_chr_fe_read_all(&s->server_chr, (uint8_t *)&msg + n,
                                    sizeof(msg) - n);
-        if (ret < 0 && ret != -EINTR) {
+        if (ret < 0) {
+            if (ret == -EINTR) {
+                continue;
+            }
             error_setg_errno(errp, -ret, "read from server failed");
             return INT64_MIN;
         }
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 04/20] nbd: fix memory leak in nbd_opt_go()

[Philippe Mathieu-Daudé]

nbd/client.c:385:12: warning: Potential leak of memory pointed to by 'buf'

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 nbd/client.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c
index 509ed5e4ba..0a17de80b5 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -376,9 +376,11 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
     if (info->request_sizes) {
         stw_be_p(buf + 4 + len + 2, NBD_INFO_BLOCK_SIZE);
     }
-    if (nbd_send_option_request(ioc, NBD_OPT_GO,
-                                4 + len + 2 + 2 * info->request_sizes, buf,
-                                errp) < 0) {
+    error = nbd_send_option_request(ioc, NBD_OPT_GO,
+                                    4 + len + 2 + 2 * info->request_sizes,
+                                    buf, errp);
+    g_free(buf);
+    if (error < 0) {
         return -1;
     }
 
-- 
2.13.3

Re: [Qemu-devel] [PATCH for 2.10 v2 04/20] nbd: fix memory leak in nbd_opt_go()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 585 bytes --]

On 07/26/2017 09:42 PM, Philippe Mathieu-Daudé wrote:
> nbd/client.c:385:12: warning: Potential leak of memory pointed to by 'buf'
> 
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  nbd/client.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

Thanks; queued on my NBD branch for rc1:
git://repo.or.cz/qemu/ericb.git nbd

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

[Qemu-devel] [PATCH for 2.10 v2 05/20] qcow2: fix null pointer dereference

[Philippe Mathieu-Daudé]

It seems this assert() was somehow misplaced.

block/qcow2-refcount.c:2193:42: warning: Array access (from variable 'on_disk_reftable') results in a null pointer dereference
        on_disk_reftable[refblock_index] = refblock_offset;
        ~~~~~~~~~~~~~~~~                 ^

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/qcow2-refcount.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index c9b0dcb4f3..168fc32e7b 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2189,6 +2189,8 @@ write_refblocks:
              * this will leak that range, but we can easily fix that by running
              * a leak-fixing check after this rebuild operation */
             reftable_offset = -1;
+        } else {
+            assert(on_disk_reftable);
         }
         on_disk_reftable[refblock_index] = refblock_offset;
 
@@ -2258,8 +2260,6 @@ write_refblocks:
         goto write_refblocks;
     }
 
-    assert(on_disk_reftable);
-
     for (refblock_index = 0; refblock_index < reftable_size; refblock_index++) {
         cpu_to_be64s(&on_disk_reftable[refblock_index]);
     }
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 06/20] ui/vnc: fix leak of SocketAddress **

[Philippe Mathieu-Daudé]

Extract the (correct) cleaning code as a new function vnc_free_addresses() then
use it to remove the memory leaks.

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
---
 ui/vnc.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index eb91559b6b..651cbb8606 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3521,6 +3521,20 @@ static int vnc_display_get_address(const char *addrstr,
     return ret;
 }
 
+static void vnc_free_addresses(SocketAddress ***retsaddr,
+                               size_t *retnsaddr)
+{
+    size_t i;
+
+    for (i = 0; i < *retnsaddr; i++) {
+        qapi_free_SocketAddress((*retsaddr)[i]);
+    }
+    g_free(*retsaddr);
+
+    *retsaddr = NULL;
+    *retnsaddr = 0;
+}
+
 static int vnc_display_get_addresses(QemuOpts *opts,
                                      bool reverse,
                                      SocketAddress ***retsaddr,
@@ -3538,7 +3552,6 @@ static int vnc_display_get_addresses(QemuOpts *opts,
     bool has_ipv6 = qemu_opt_get(opts, "ipv6");
     bool ipv4 = qemu_opt_get_bool(opts, "ipv4", false);
     bool ipv6 = qemu_opt_get_bool(opts, "ipv6", false);
-    size_t i;
     int displaynum = -1;
     int ret = -1;
 
@@ -3614,16 +3627,8 @@ static int vnc_display_get_addresses(QemuOpts *opts,
     ret = 0;
  cleanup:
     if (ret < 0) {
-        for (i = 0; i < *retnsaddr; i++) {
-            qapi_free_SocketAddress((*retsaddr)[i]);
-        }
-        g_free(*retsaddr);
-        for (i = 0; i < *retnwsaddr; i++) {
-            qapi_free_SocketAddress((*retwsaddr)[i]);
-        }
-        g_free(*retwsaddr);
-        *retsaddr = *retwsaddr = NULL;
-        *retnsaddr = *retnwsaddr = 0;
+        vnc_free_addresses(retsaddr, retnsaddr);
+        vnc_free_addresses(retwsaddr, retnwsaddr);
     }
     return ret;
 }
@@ -3772,7 +3777,6 @@ void vnc_display_open(const char *id, Error **errp)
     int acl = 0;
     int lock_key_sync = 1;
     int key_delay_ms;
-    size_t i;
 
     if (!vd) {
         error_setg(errp, "VNC display not active");
@@ -3993,12 +3997,8 @@ void vnc_display_open(const char *id, Error **errp)
     }
 
  cleanup:
-    for (i = 0; i < nsaddr; i++) {
-        qapi_free_SocketAddress(saddr[i]);
-    }
-    for (i = 0; i < nwsaddr; i++) {
-        qapi_free_SocketAddress(wsaddr[i]);
-    }
+    vnc_free_addresses(&saddr, &nsaddr);
+    vnc_free_addresses(&wsaddr, &nwsaddr);
     return;
 
 fail:
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 07/20] net/eth: fix incorrect check of iov_to_buf() return value

[Philippe Mathieu-Daudé]

So we have sizeof(struct in6_address) != sizeof(uintptr_t)
and Clang > Coverity on this, see 4555ca6816c :)

net/eth.c:426:30: warning: The code calls sizeof() on a pointer type. This can produce an unexpected result
        return bytes_read == sizeof(dst_addr);
                             ^     ~~~~~~~~~~
net/eth.c:475:34: warning: The code calls sizeof() on a pointer type. This can produce an unexpected result
            return bytes_read == sizeof(src_addr);
                                 ^     ~~~~~~~~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Dmitry Fleytman <dmitry@daynix.com>
---
 net/eth.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/eth.c b/net/eth.c
index 5b9ba26a56..ae5d881aae 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -423,7 +423,7 @@ _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int pkt_frags,
                                 rthdr_offset + sizeof(*ext_hdr),
                                 dst_addr, sizeof(*dst_addr));
 
-        return bytes_read == sizeof(dst_addr);
+        return bytes_read == sizeof(*dst_addr);
     }
 
     return false;
@@ -472,7 +472,7 @@ _eth_get_rss_ex_src_addr(const struct iovec *pkt, int pkt_frags,
                                     opt_offset + sizeof(opthdr),
                                     src_addr, sizeof(*src_addr));
 
-            return bytes_read == sizeof(src_addr);
+            return bytes_read == sizeof(*src_addr);
         }
 
         opt_offset += optlen;
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 08/20] vfio/platform: fix use of freed memory

[Philippe Mathieu-Daudé]

free the data _after_ using it.

hw/vfio/platform.c:126:29: warning: Use of memory after it is freed
        qemu_set_fd_handler(*pfd, NULL, NULL, NULL);
                            ^~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/vfio/platform.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index 7c09deda61..da84abf4fc 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -120,11 +120,11 @@ static int vfio_set_trigger_eventfd(VFIOINTp *intp,
     *pfd = event_notifier_get_fd(intp->interrupt);
     qemu_set_fd_handler(*pfd, (IOHandler *)handler, NULL, intp);
     ret = ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set);
-    g_free(irq_set);
     if (ret < 0) {
         error_report("vfio: Failed to set trigger eventfd: %m");
         qemu_set_fd_handler(*pfd, NULL, NULL, NULL);
     }
+    g_free(irq_set);
     return ret;
 }
 
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 09/20] vfio/pci: fix use of freed memory

[Philippe Mathieu-Daudé]

hw/vfio/pci.c:308:29: warning: Use of memory after it is freed
        qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
                            ^~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/vfio/pci.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index d4051cb951..31e1edf447 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -257,7 +257,7 @@ static void vfio_intx_update(PCIDevice *pdev)
 static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
 {
     uint8_t pin = vfio_pci_read_config(&vdev->pdev, PCI_INTERRUPT_PIN, 1);
-    int ret, argsz;
+    int ret, argsz, retval = 0;
     struct vfio_irq_set *irq_set;
     int32_t *pfd;
     Error *err = NULL;
@@ -302,12 +302,12 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
     qemu_set_fd_handler(*pfd, vfio_intx_interrupt, NULL, vdev);
 
     ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
-    g_free(irq_set);
     if (ret) {
         error_setg_errno(errp, -ret, "failed to setup INTx fd");
         qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
         event_notifier_cleanup(&vdev->intx.interrupt);
-        return -errno;
+        retval = -errno;
+        goto cleanup;
     }
 
     vfio_intx_enable_kvm(vdev, &err);
@@ -319,7 +319,10 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
 
     trace_vfio_intx_enable(vdev->vbasedev.name);
 
-    return 0;
+cleanup:
+    g_free(irq_set);
+
+    return retval;
 }
 
 static void vfio_intx_disable(VFIOPCIDevice *vdev)
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 10/20] m68k/translate: fix incorrect copy/paste

[Philippe Mathieu-Daudé]

db3d7945ae extended gen_cc_cond() for cond [6, 7, 9, 10] but misswrote [4, 5]

target/m68k/translate.c:1323:70: warning: identical expressions on both sides of logical operator
        if (op == CC_OP_ADDB || op == CC_OP_ADDW || op == CC_OP_ADDL ||
            op == CC_OP_ADDB || op == CC_OP_ADDW || op == CC_OP_ADDL) {
            ~~~~~~~~~~~~~~~~    ~~~~~~~~~~~~~~~~    ~~~~~~~~~~~~~~~~ ^

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
---
 target/m68k/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index ada2a91b64..be24355080 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -1321,7 +1321,7 @@ static void gen_cc_cond(DisasCompare *c, DisasContext *s, int cond)
     case 5: /* CS (C) */
         /* Some cases fold C into X.  */
         if (op == CC_OP_ADDB || op == CC_OP_ADDW || op == CC_OP_ADDL ||
-            op == CC_OP_ADDB || op == CC_OP_ADDW || op == CC_OP_ADDL) {
+            op == CC_OP_SUBB || op == CC_OP_SUBW || op == CC_OP_SUBL) {
             tcond = TCG_COND_NE;
             c->v1 = QREG_CC_X;
             goto done;
-- 
2.13.3

Re: [Qemu-devel] [PATCH for 2.10 v2 10/20] m68k/translate: fix incorrect copy/paste

[Richard Henderson]

On 07/26/2017 07:42 PM, Philippe Mathieu-Daudé wrote:
> db3d7945ae extended gen_cc_cond() for cond [6, 7, 9, 10] but misswrote [4, 5]
> 
> target/m68k/translate.c:1323:70: warning: identical expressions on both sides of logical operator
>          if (op == CC_OP_ADDB || op == CC_OP_ADDW || op == CC_OP_ADDL ||
>              op == CC_OP_ADDB || op == CC_OP_ADDW || op == CC_OP_ADDL) {
>              ~~~~~~~~~~~~~~~~    ~~~~~~~~~~~~~~~~    ~~~~~~~~~~~~~~~~ ^
> 
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org>
> Reviewed-by: Laurent Vivier<laurent@vivier.eu>
> ---
>   target/m68k/translate.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

[Qemu-devel] [PATCH for 2.10 v2 11/20] linux-user/sh4: fix incorrect memory write

[Philippe Mathieu-Daudé]

not hit since 2009! :)

linux-user/elfload.c:1102:20: warning: Out of bound memory access (access exceeds upper limit of memory block)
        (*regs[i]) = tswap32(env->gregs[i]);
        ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
---
 linux-user/elfload.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 2a902f7806..79062882ba 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -1099,7 +1099,7 @@ static inline void elf_core_copy_regs(target_elf_gregset_t *regs,
     int i;
 
     for (i = 0; i < 16; i++) {
-        (*regs[i]) = tswapreg(env->gregs[i]);
+        (*regs)[i] = tswapreg(env->gregs[i]);
     }
 
     (*regs)[TARGET_REG_PC] = tswapreg(env->pc);
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 12/20] syscall: fix dereference of undefined pointer

[Philippe Mathieu-Daudé]

linux-user/syscall.c:5581:9: warning: Dereference of undefined pointer value
    if (*host_rt_dev_ptr != 0) {
        ^~~~~~~~~~~~~~~~

Reported-by: Clang Static Analyzer
Suggested-by: Laurent Vivier <lvivier@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 linux-user/syscall.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 003943b736..71d45a9963 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -5573,6 +5573,7 @@ static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
                                     field_types, THUNK_HOST);
     }
     unlock_user(argptr, arg, 0);
+    assert(host_rt_dev_ptr);
 
     ret = get_errno(safe_ioctl(fd, ie->host_cmd, buf_temp));
     if (*host_rt_dev_ptr != 0) {
-- 
2.13.3

Re: [Qemu-devel] [PATCH for 2.10 v2 12/20] syscall: fix dereference of undefined pointer

[Laurent Vivier]

Le 27/07/2017 à 04:42, Philippe Mathieu-Daudé a écrit :
> linux-user/syscall.c:5581:9: warning: Dereference of undefined pointer value
>     if (*host_rt_dev_ptr != 0) {
>         ^~~~~~~~~~~~~~~~
> 
> Reported-by: Clang Static Analyzer
> Suggested-by: Laurent Vivier <lvivier@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

> ---
>  linux-user/syscall.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 003943b736..71d45a9963 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -5573,6 +5573,7 @@ static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
>                                      field_types, THUNK_HOST);
>      }
>      unlock_user(argptr, arg, 0);
> +    assert(host_rt_dev_ptr);
>  
>      ret = get_errno(safe_ioctl(fd, ie->host_cmd, buf_temp));
>      if (*host_rt_dev_ptr != 0) {
> 

[Qemu-devel] [PATCH for 2.10 v2 13/20] syscall: fix use of uninitialized values

[Philippe Mathieu-Daudé]

linux-user/syscall.c:1627:35: warning: 1st function call argument is an uninitialized value
        target_saddr->sa_family = tswap16(addr->sa_family);
                                  ^~~~~~~~~~~~~~~~~~~~~~~~
linux-user/syscall.c:1629:25: warning: The left operand of '==' is a garbage value
    if (addr->sa_family == AF_NETLINK && len >= sizeof(struct sockaddr_nl)) {
        ~~~~~~~~~~~~~~~ ^

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
---
 linux-user/syscall.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 71d45a9963..81f52f7483 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1622,6 +1622,7 @@ static inline abi_long host_to_target_sockaddr(abi_ulong target_addr,
     if (len == 0) {
         return 0;
     }
+    assert(addr);
 
     target_saddr = lock_user(VERIFY_WRITE, target_addr, len, 0);
     if (!target_saddr)
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 14/20] syscall: check inotify() and eventfd() return value

[Philippe Mathieu-Daudé]

linux-user/syscall.c:555:25: warning: Out of bound memory access (accessed memory precedes memory block)
    target_fd_trans[fd] = trans;
    ~~~~~~~~~~~~~~~~~~~~^~~~~~~

Reported-by: Clang Static Analyzer
Suggested-by: Laurent Vivier <lvivier@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 linux-user/syscall.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 81f52f7483..dfc1301e63 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -11742,7 +11742,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
 #if defined(TARGET_NR_inotify_init) && defined(__NR_inotify_init)
     case TARGET_NR_inotify_init:
         ret = get_errno(sys_inotify_init());
-        fd_trans_register(ret, &target_inotify_trans);
+        if (ret >= 0) {
+            fd_trans_register(ret, &target_inotify_trans);
+        }
         break;
 #endif
 #ifdef CONFIG_INOTIFY1
@@ -11750,7 +11752,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
     case TARGET_NR_inotify_init1:
         ret = get_errno(sys_inotify_init1(target_to_host_bitmask(arg1,
                                           fcntl_flags_tbl)));
-        fd_trans_register(ret, &target_inotify_trans);
+        if (ret >= 0) {
+            fd_trans_register(ret, &target_inotify_trans);
+        }
         break;
 #endif
 #endif
@@ -11916,7 +11920,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
 #if defined(TARGET_NR_eventfd)
     case TARGET_NR_eventfd:
         ret = get_errno(eventfd(arg1, 0));
-        fd_trans_register(ret, &target_eventfd_trans);
+        if (ret >= 0) {
+            fd_trans_register(ret, &target_eventfd_trans);
+        }
         break;
 #endif
 #if defined(TARGET_NR_eventfd2)
@@ -11930,7 +11936,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
             host_flags |= O_CLOEXEC;
         }
         ret = get_errno(eventfd(arg1, host_flags));
-        fd_trans_register(ret, &target_eventfd_trans);
+        if (ret >= 0) {
+            fd_trans_register(ret, &target_eventfd_trans);
+        }
         break;
     }
 #endif
-- 
2.13.3

Re: [Qemu-devel] [PATCH for 2.10 v2 14/20] syscall: check inotify() and eventfd() return value

[Laurent Vivier]

Le 27/07/2017 à 04:42, Philippe Mathieu-Daudé a écrit :
> linux-user/syscall.c:555:25: warning: Out of bound memory access (accessed memory precedes memory block)
>     target_fd_trans[fd] = trans;
>     ~~~~~~~~~~~~~~~~~~~~^~~~~~~
> 
> Reported-by: Clang Static Analyzer
> Suggested-by: Laurent Vivier <lvivier@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

> ---
>  linux-user/syscall.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 81f52f7483..dfc1301e63 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -11742,7 +11742,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>  #if defined(TARGET_NR_inotify_init) && defined(__NR_inotify_init)
>      case TARGET_NR_inotify_init:
>          ret = get_errno(sys_inotify_init());
> -        fd_trans_register(ret, &target_inotify_trans);
> +        if (ret >= 0) {
> +            fd_trans_register(ret, &target_inotify_trans);
> +        }
>          break;
>  #endif
>  #ifdef CONFIG_INOTIFY1
> @@ -11750,7 +11752,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>      case TARGET_NR_inotify_init1:
>          ret = get_errno(sys_inotify_init1(target_to_host_bitmask(arg1,
>                                            fcntl_flags_tbl)));
> -        fd_trans_register(ret, &target_inotify_trans);
> +        if (ret >= 0) {
> +            fd_trans_register(ret, &target_inotify_trans);
> +        }
>          break;
>  #endif
>  #endif
> @@ -11916,7 +11920,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>  #if defined(TARGET_NR_eventfd)
>      case TARGET_NR_eventfd:
>          ret = get_errno(eventfd(arg1, 0));
> -        fd_trans_register(ret, &target_eventfd_trans);
> +        if (ret >= 0) {
> +            fd_trans_register(ret, &target_eventfd_trans);
> +        }
>          break;
>  #endif
>  #if defined(TARGET_NR_eventfd2)
> @@ -11930,7 +11936,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>              host_flags |= O_CLOEXEC;
>          }
>          ret = get_errno(eventfd(arg1, host_flags));
> -        fd_trans_register(ret, &target_eventfd_trans);
> +        if (ret >= 0) {
> +            fd_trans_register(ret, &target_eventfd_trans);
> +        }
>          break;
>      }
>  #endif
> 

[Qemu-devel] [PATCH for 2.10 v2 15/20] thunk: assert nb_fields is valid

[Philippe Mathieu-Daudé]

thunk.c:91:32: warning: Call to 'malloc' has an allocation size of 0 bytes
        se->field_offsets[i] = malloc(nb_fields * sizeof(int));
                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 thunk.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/thunk.c b/thunk.c
index 2dac36666d..d5d8645cd4 100644
--- a/thunk.c
+++ b/thunk.c
@@ -67,7 +67,6 @@ void thunk_register_struct(int id, const char *name, const argtype *types)
     int nb_fields, offset, max_align, align, size, i, j;
 
     assert(id < max_struct_entries);
-    se = struct_entries + id;
 
     /* first we count the number of fields */
     type_ptr = types;
@@ -76,6 +75,8 @@ void thunk_register_struct(int id, const char *name, const argtype *types)
         type_ptr = thunk_type_next(type_ptr);
         nb_fields++;
     }
+    assert(nb_fields > 0);
+    se = struct_entries + id;
     se->field_types = types;
     se->nb_fields = nb_fields;
     se->name = name;
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 v2 17/20] bt-sdp: fix memory leak in sdp_service_record_build()

[Philippe Mathieu-Daudé]

hw/bt/sdp.c:753:5: warning: Potential leak of memory pointed to by 'data'
    qsort(record->attribute_list, record->attributes,
    ^~~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/bt/*:
get_maintainer.pl: No maintainers found

 hw/bt/sdp.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/bt/sdp.c b/hw/bt/sdp.c
index f67b3b89c0..7b2186e1f4 100644
--- a/hw/bt/sdp.c
+++ b/hw/bt/sdp.c
@@ -711,7 +711,7 @@ static void sdp_service_record_build(struct sdp_service_record_s *record,
                 struct sdp_def_service_s *def, int handle)
 {
     int len = 0;
-    uint8_t *data;
+    uint8_t *buf, *data;
     int *uuid;
 
     record->uuids = 0;
@@ -725,7 +725,8 @@ static void sdp_service_record_build(struct sdp_service_record_s *record,
             g_malloc0(record->attributes * sizeof(*record->attribute_list));
     record->uuid =
             g_malloc0(record->uuids * sizeof(*record->uuid));
-    data = g_malloc(len);
+    buf = g_malloc(len);
+    data = buf;
 
     record->attributes = 0;
     uuid = record->uuid;
@@ -748,6 +749,7 @@ static void sdp_service_record_build(struct sdp_service_record_s *record,
         record->attribute_list[record->attributes ++].len = len;
         data += len;
     }
+    g_free(buf);
 
     /* Sort the attribute list by the AttributeID */
     qsort(record->attribute_list, record->attributes,
-- 
2.13.3

Re: [Qemu-devel] [PATCH for 2.10 v2 17/20] bt-sdp: fix memory leak in sdp_service_record_build()

[Paolo Bonzini]

On 27/07/2017 04:42, Philippe Mathieu-Daudé wrote:
> hw/bt/sdp.c:753:5: warning: Potential leak of memory pointed to by 'data'
>     qsort(record->attribute_list, record->attributes,
>     ^~~~~
> 
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/bt/*:
> get_maintainer.pl: No maintainers found
> 
>  hw/bt/sdp.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/bt/sdp.c b/hw/bt/sdp.c
> index f67b3b89c0..7b2186e1f4 100644
> --- a/hw/bt/sdp.c
> +++ b/hw/bt/sdp.c
> @@ -711,7 +711,7 @@ static void sdp_service_record_build(struct sdp_service_record_s *record,
>                  struct sdp_def_service_s *def, int handle)
>  {
>      int len = 0;
> -    uint8_t *data;
> +    uint8_t *buf, *data;
>      int *uuid;
>  
>      record->uuids = 0;
> @@ -725,7 +725,8 @@ static void sdp_service_record_build(struct sdp_service_record_s *record,
>              g_malloc0(record->attributes * sizeof(*record->attribute_list));
>      record->uuid =
>              g_malloc0(record->uuids * sizeof(*record->uuid));
> -    data = g_malloc(len);
> +    buf = g_malloc(len);
> +    data = buf;
>  
>      record->attributes = 0;
>      uuid = record->uuid;
> @@ -748,6 +749,7 @@ static void sdp_service_record_build(struct sdp_service_record_s *record,
>          record->attribute_list[record->attributes ++].len = len;
>          data += len;
>      }
> +    g_free(buf);
>  
>      /* Sort the attribute list by the AttributeID */
>      qsort(record->attribute_list, record->attributes,
> 

This is wrong, but the code is insane and wrong too so it's not your
fault. :)  The allocated memory escapes here:
    
            record->attribute_list[record->attributes].pair = data;
    
So clang is correct that the memory might leak if len is zero.  We
know it isn't, so there is no leak.  An assertion could shut up clang.

But the craziness doesn't end there.  The memory is freed by
bt_l2cap_sdp_close_ch:
    
           g_free(sdp->service_list[i].attribute_list->pair);
    
which actually should have been written:
    
           g_free(sdp->service_list[i].attribute_list[0].pair);

because the first element of the array points to the malloc-ed buffer.

The attribute_list is sorted with qsort, which would be very fishy,
but indeed the first entry of attribute_list should point to data
even after the qsort, because the first record has id
SDP_ATTR_RECORD_HANDLE, whose numeric value is zero.  Another
assertion will help here but... hang on...

the qsort function is
    
        static int sdp_attributeid_compare(
                    const struct sdp_service_attribute_s *a,
                    const struct sdp_service_attribute_s *b)
        {
            return (int) b->attribute_id - a->attribute_id;
        }
    
and _no one ever_ writes attribute_id.  So it only works if qsort is
stable, and who knows what else is broken, but we can fix it by
setting attribute_id in the while loop.

The patch after the signature should do it.  Please review carefully
because I've no idea how to test this stuff.

Paolo

diff --git a/hw/bt/sdp.c b/hw/bt/sdp.c
index f67b3b89c0..f16a0f0b09 100644
--- a/hw/bt/sdp.c
+++ b/hw/bt/sdp.c
@@ -580,7 +580,7 @@ static void bt_l2cap_sdp_close_ch(void *opaque)
     int i;
 
     for (i = 0; i < sdp->services; i ++) {
-        g_free(sdp->service_list[i].attribute_list->pair);
+        g_free(sdp->service_list[i].attribute_list[0].pair);
         g_free(sdp->service_list[i].attribute_list);
         g_free(sdp->service_list[i].uuid);
     }
@@ -720,6 +720,8 @@ static void sdp_service_record_build(struct sdp_service_record_s *record,
         len += sdp_attr_max_size(&def->attributes[record->attributes ++].data,
                         &record->uuids);
     }
+
+    assert(len > 0);
     record->uuids = pow2ceil(record->uuids);
     record->attribute_list =
             g_malloc0(record->attributes * sizeof(*record->attribute_list));
@@ -730,12 +730,14 @@ static void sdp_service_record_build(struct sdp_service_record_s *record,
     record->attributes = 0;
     uuid = record->uuid;
     while (def->attributes[record->attributes].data.type) {
+        int attribute_id = def->attributes[record->attributes].id;
         record->attribute_list[record->attributes].pair = data;
+        record->attribute_list[record->attributes].attribute_id = attribute_id;
 
         len = 0;
         data[len ++] = SDP_DTYPE_UINT | SDP_DSIZE_2;
-        data[len ++] = def->attributes[record->attributes].id >> 8;
-        data[len ++] = def->attributes[record->attributes].id & 0xff;
+        data[len ++] = attribute_id >> 8;
+        data[len ++] = attribute_id & 0xff;
         len += sdp_attr_write(data + len,
                         &def->attributes[record->attributes].data, &uuid);
 
@@ -749,10 +751,15 @@ static void sdp_service_record_build(struct sdp_service_record_s *record,
         data += len;
     }
 
-    /* Sort the attribute list by the AttributeID */
+    /* Sort the attribute list by the AttributeID.  The first must be
+     * SDP_ATTR_RECORD_HANDLE so that bt_l2cap_sdp_close_ch can free
+     * the buffer.
+     */
     qsort(record->attribute_list, record->attributes,
                     sizeof(*record->attribute_list),
                     (void *) sdp_attributeid_compare);
+    assert(record->attribute_list[0].pair == data);
+
     /* Sort the searchable UUIDs list for bisection */
     qsort(record->uuid, record->uuids,
                     sizeof(*record->uuid),

[Qemu-devel] [PATCH for 2.10 v2 18/20] 9pfs: avoid sign conversion error simplifying the code

[Philippe Mathieu-Daudé]

(note this is how other functions also handle the errors).

hw/9pfs/9p.c:948:18: warning: Loss of sign in implicit conversion
        offset = err;
                 ^~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/9pfs/9p.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 333dbb6f8e..0a37c8bd13 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -945,7 +945,6 @@ static void coroutine_fn v9fs_version(void *opaque)
     v9fs_string_init(&version);
     err = pdu_unmarshal(pdu, offset, "ds", &s->msize, &version);
     if (err < 0) {
-        offset = err;
         goto out;
     }
     trace_v9fs_version(pdu->tag, pdu->id, s->msize, version.data);
@@ -962,13 +961,12 @@ static void coroutine_fn v9fs_version(void *opaque)
 
     err = pdu_marshal(pdu, offset, "ds", s->msize, &version);
     if (err < 0) {
-        offset = err;
         goto out;
     }
-    offset += err;
+    err += offset;
     trace_v9fs_version_return(pdu->tag, pdu->id, s->msize, version.data);
 out:
-    pdu_complete(pdu, offset);
+    pdu_complete(pdu, err);
     v9fs_string_free(&version);
 }
 
-- 
2.13.3

Re: [Qemu-devel] [PATCH for 2.10 v2 18/20] 9pfs: avoid sign conversion error simplifying the code

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 1563 bytes --]

On Wed, 26 Jul 2017 23:42:22 -0300
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> (note this is how other functions also handle the errors).
> 
> hw/9pfs/9p.c:948:18: warning: Loss of sign in implicit conversion
>         offset = err;
>                  ^~~
> 
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

Now, I'm not sure this can be merged during hard freeze since it is
more code cleanup than actual bug fixing...

>  hw/9pfs/9p.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 333dbb6f8e..0a37c8bd13 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -945,7 +945,6 @@ static void coroutine_fn v9fs_version(void *opaque)
>      v9fs_string_init(&version);
>      err = pdu_unmarshal(pdu, offset, "ds", &s->msize, &version);
>      if (err < 0) {
> -        offset = err;
>          goto out;
>      }
>      trace_v9fs_version(pdu->tag, pdu->id, s->msize, version.data);
> @@ -962,13 +961,12 @@ static void coroutine_fn v9fs_version(void *opaque)
>  
>      err = pdu_marshal(pdu, offset, "ds", s->msize, &version);
>      if (err < 0) {
> -        offset = err;
>          goto out;
>      }
> -    offset += err;
> +    err += offset;
>      trace_v9fs_version_return(pdu->tag, pdu->id, s->msize, version.data);
>  out:
> -    pdu_complete(pdu, offset);
> +    pdu_complete(pdu, err);
>      v9fs_string_free(&version);
>  }
>  


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [Qemu-trivial] [PATCH for 2.10 v2 18/20] 9pfs: avoid sign conversion error simplifying the code

[Philippe Mathieu-Daudé]

On 07/27/2017 08:40 AM, Greg Kurz wrote:
> On Wed, 26 Jul 2017 23:42:22 -0300
> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> Now, I'm not sure this can be merged during hard freeze since it is
> more code cleanup than actual bug fixing...

Hmm the commit message is probably not enough.
The problem is this code can send broken packets, see inlined:

> 
>>   hw/9pfs/9p.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
>> index 333dbb6f8e..0a37c8bd13 100644
>> --- a/hw/9pfs/9p.c
>> +++ b/hw/9pfs/9p.c
>> @@ -945,7 +945,6 @@ static void coroutine_fn v9fs_version(void *opaque)
>>       v9fs_string_init(&version);
>>       err = pdu_unmarshal(pdu, offset, "ds", &s->msize, &version);
>>       if (err < 0) {

if err == -1

>> -        offset = err;

here this sets offset = (size_t)(-1) = SIZE_MAX

>>           goto out;
>>       }
>>       trace_v9fs_version(pdu->tag, pdu->id, s->msize, version.data);
>> @@ -962,13 +961,12 @@ static void coroutine_fn v9fs_version(void *opaque)
>>   
>>       err = pdu_marshal(pdu, offset, "ds", s->msize, &version);
>>       if (err < 0) {
>> -        offset = err;
>>           goto out;
>>       }
>> -    offset += err;

here offset += SIZE_MAX which wraps, so this equivs to offset -= 1

>> +    err += offset;
>>       trace_v9fs_version_return(pdu->tag, pdu->id, s->msize, version.data);
>>   out:
>> -    pdu_complete(pdu, offset);

now we have offset = 7 - 1 = 6, since 6 > 0 pdu_complete() does not 
marshal the error code but 6 bytes of crap from pdu?

Maybe I missed something.

>> +    pdu_complete(pdu, err);
>>       v9fs_string_free(&version);
>>   }
>>   

Regards,

Phil.

Re: [Qemu-devel] [Qemu-trivial] [PATCH for 2.10 v2 18/20] 9pfs: avoid sign conversion error simplifying the code

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 2201 bytes --]

On Thu, 27 Jul 2017 16:18:07 -0300
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> On 07/27/2017 08:40 AM, Greg Kurz wrote:
> > On Wed, 26 Jul 2017 23:42:22 -0300
> > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> > 
> > Now, I'm not sure this can be merged during hard freeze since it is
> > more code cleanup than actual bug fixing...  
> 
> Hmm the commit message is probably not enough.
> The problem is this code can send broken packets, see inlined:
> 
> >   
> >>   hw/9pfs/9p.c | 6 ++----
> >>   1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> >> index 333dbb6f8e..0a37c8bd13 100644
> >> --- a/hw/9pfs/9p.c
> >> +++ b/hw/9pfs/9p.c
> >> @@ -945,7 +945,6 @@ static void coroutine_fn v9fs_version(void *opaque)
> >>       v9fs_string_init(&version);
> >>       err = pdu_unmarshal(pdu, offset, "ds", &s->msize, &version);
> >>       if (err < 0) {  
> 
> if err == -1
> 
> >> -        offset = err;  
> 
> here this sets offset = (size_t)(-1) = SIZE_MAX
> 
> >>           goto out;

and here we jump directly to the out label, so...

> >>       }
> >>       trace_v9fs_version(pdu->tag, pdu->id, s->msize, version.data);
> >> @@ -962,13 +961,12 @@ static void coroutine_fn v9fs_version(void *opaque)
> >>   
> >>       err = pdu_marshal(pdu, offset, "ds", s->msize, &version);
> >>       if (err < 0) {
> >> -        offset = err;
> >>           goto out;
> >>       }
> >> -    offset += err;  
> 
> here offset += SIZE_MAX which wraps, so this equivs to offset -= 1
> 

... this cannot happen.

> >> +    err += offset;
> >>       trace_v9fs_version_return(pdu->tag, pdu->id, s->msize, version.data);
> >>   out:
> >> -    pdu_complete(pdu, offset);  
> 
> now we have offset = 7 - 1 = 6, since 6 > 0 pdu_complete() does not 
> marshal the error code but 6 bytes of crap from pdu?
> 
> Maybe I missed something.
> 
> >> +    pdu_complete(pdu, err);
> >>       v9fs_string_free(&version);
> >>   }
> >>     
> 
> Regards,
> 
> Phil.

I've pushed this to my 9p-next branch to be merged in 2.11.

Cheers,

--
Greg

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[Qemu-devel] [PATCH for 2.10 v2 19/20] spapr_vio: fix overflow of qdevs in spapr_dt_vdevice()

[Philippe Mathieu-Daudé]

sizeof(ptr) was used instead of sizeof(struct)...

also use g_malloc_n() which take care of possible type overflow.

hw/ppc/spapr_vio.c:641:22: warning: The code calls sizeof() on a pointer type. This can produce an unexpected result
    qdevs = g_malloc(sizeof(qdev) * num);
                     ^     ~~~~~~
hw/ppc/spapr_vio.c:648:23: warning: The code calls sizeof() on a pointer type. This can produce an unexpected result
    qsort(qdevs, num, sizeof(qdev), compare_reg);
                      ^     ~~~~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/ppc/spapr_vio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index ea3bc8bd9e..9991b44c9f 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -638,14 +638,14 @@ void spapr_dt_vdevice(VIOsPAPRBus *bus, void *fdt)
     }
 
     /* Copy out into an array of pointers */
-    qdevs = g_malloc(sizeof(qdev) * num);
+    qdevs = g_malloc_n(num, sizeof(*qdev));
     num = 0;
     QTAILQ_FOREACH(kid, &bus->bus.children, sibling) {
         qdevs[num++] = kid->child;
     }
 
     /* Sort the array */
-    qsort(qdevs, num, sizeof(qdev), compare_reg);
+    qsort(qdevs, num, sizeof(*qdev), compare_reg);
 
     /* Hack alert. Give the devices to libfdt in reverse order, we happen
      * to know that will mean they are in forward order in the tree. */
-- 
2.13.3

Re: [Qemu-devel] [PATCH for 2.10 v2 19/20] spapr_vio: fix overflow of qdevs in spapr_dt_vdevice()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2002 bytes --]

On Wed, Jul 26, 2017 at 11:42:23PM -0300, Philippe Mathieu-Daudé wrote:
> sizeof(ptr) was used instead of sizeof(struct)...
> 
> also use g_malloc_n() which take care of possible type overflow.
> 
> hw/ppc/spapr_vio.c:641:22: warning: The code calls sizeof() on a pointer type. This can produce an unexpected result
>     qdevs = g_malloc(sizeof(qdev) * num);
>                      ^     ~~~~~~
> hw/ppc/spapr_vio.c:648:23: warning: The code calls sizeof() on a pointer type. This can produce an unexpected result
>     qsort(qdevs, num, sizeof(qdev), compare_reg);
>                       ^     ~~~~~~
> 
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Nack.

Have a closer look, what's going in the array really is pointers, not
structures.  This is a false warning from clang, we need to find a
different way to suppress it.

> ---
>  hw/ppc/spapr_vio.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> index ea3bc8bd9e..9991b44c9f 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -638,14 +638,14 @@ void spapr_dt_vdevice(VIOsPAPRBus *bus, void *fdt)
>      }
>  
>      /* Copy out into an array of pointers */
> -    qdevs = g_malloc(sizeof(qdev) * num);
> +    qdevs = g_malloc_n(num, sizeof(*qdev));
>      num = 0;
>      QTAILQ_FOREACH(kid, &bus->bus.children, sibling) {
>          qdevs[num++] = kid->child;
>      }
>  
>      /* Sort the array */
> -    qsort(qdevs, num, sizeof(qdev), compare_reg);
> +    qsort(qdevs, num, sizeof(*qdev), compare_reg);
>  
>      /* Hack alert. Give the devices to libfdt in reverse order, we happen
>       * to know that will mean they are in forward order in the tree. */

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH for 2.10 v2 19/20] spapr_vio: fix overflow of qdevs in spapr_dt_vdevice()

[Philippe Mathieu-Daudé]

On 07/27/2017 12:43 AM, David Gibson wrote:
> On Wed, Jul 26, 2017 at 11:42:23PM -0300, Philippe Mathieu-Daudé wrote:
>> sizeof(ptr) was used instead of sizeof(struct)...
>>
>> also use g_malloc_n() which take care of possible type overflow.
>>
>> hw/ppc/spapr_vio.c:641:22: warning: The code calls sizeof() on a pointer type. This can produce an unexpected result
>>      qdevs = g_malloc(sizeof(qdev) * num);
>>                       ^     ~~~~~~
>> hw/ppc/spapr_vio.c:648:23: warning: The code calls sizeof() on a pointer type. This can produce an unexpected result
>>      qsort(qdevs, num, sizeof(qdev), compare_reg);
>>                        ^     ~~~~~~
>>
>> Reported-by: Clang Static Analyzer
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> Nack.
> 
> Have a closer look, what's going in the array really is pointers, not
> structures.  This is a false warning from clang, we need to find a
> different way to suppress it.

Something was bothering me with that patch, wondering why it never 
explode previously, now I see/understand.

>> ---
>>   hw/ppc/spapr_vio.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
>> index ea3bc8bd9e..9991b44c9f 100644
>> --- a/hw/ppc/spapr_vio.c
>> +++ b/hw/ppc/spapr_vio.c
>> @@ -638,14 +638,14 @@ void spapr_dt_vdevice(VIOsPAPRBus *bus, void *fdt)
>>       }
>>   
>>       /* Copy out into an array of pointers */

/ashamed the comment was in front of me...

Thank you David for the review!

Phil.

>> -    qdevs = g_malloc(sizeof(qdev) * num);
>> +    qdevs = g_malloc_n(num, sizeof(*qdev));
>>       num = 0;
>>       QTAILQ_FOREACH(kid, &bus->bus.children, sibling) {
>>           qdevs[num++] = kid->child;
>>       }
>>   
>>       /* Sort the array */
>> -    qsort(qdevs, num, sizeof(qdev), compare_reg);
>> +    qsort(qdevs, num, sizeof(*qdev), compare_reg);
>>   
>>       /* Hack alert. Give the devices to libfdt in reverse order, we happen
>>        * to know that will mean they are in forward order in the tree. */
> 

[Qemu-devel] [PATCH for 2.10 v2 20/20] i2c/exynos4210: fix write to I2CADD register, bit 0 is not mapped

[Philippe Mathieu-Daudé]

>From the Exynos4210 User Manual [1]:

14.4.1.3 I2CADDn (MULTI-MASTER I2C-Bus Address Register)
  [7-1] slave address, latched from the I2C-bus.
  bit [0] is not mapped.

[1]: Exynos_4_Dual_45nm_User_Manaul_Public_REV1.00-0.pdf

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/i2c/exynos4210_i2c.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i2c/exynos4210_i2c.c b/hw/i2c/exynos4210_i2c.c
index c96fa7d7be..e6a9ca8f03 100644
--- a/hw/i2c/exynos4210_i2c.c
+++ b/hw/i2c/exynos4210_i2c.c
@@ -243,7 +243,7 @@ static void exynos4210_i2c_write(void *opaque, hwaddr offset,
         break;
     case I2CADD_ADDR:
         if ((s->i2cstat & I2CSTAT_OUTPUT_EN) == 0) {
-            s->i2cadd = v;
+            s->i2cadd = v & ~1;
         }
         break;
     case I2CDS_ADDR:
-- 
2.13.3

Re: [Qemu-devel] [PATCH for 2.10 v2 20/20] i2c/exynos4210: fix write to I2CADD register, bit 0 is not mapped

[Michael Tokarev]

27.07.2017 05:42, Philippe Mathieu-Daudé wrote:
> From the Exynos4210 User Manual [1]:
> 
> 14.4.1.3 I2CADDn (MULTI-MASTER I2C-Bus Address Register)
>   [7-1] slave address, latched from the I2C-bus.
>   bit [0] is not mapped.
> 
> [1]: Exynos_4_Dual_45nm_User_Manaul_Public_REV1.00-0.pdf

Okay, so what happens when one writes to bit0?

> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/i2c/exynos4210_i2c.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/i2c/exynos4210_i2c.c b/hw/i2c/exynos4210_i2c.c
> index c96fa7d7be..e6a9ca8f03 100644
> --- a/hw/i2c/exynos4210_i2c.c
> +++ b/hw/i2c/exynos4210_i2c.c
> @@ -243,7 +243,7 @@ static void exynos4210_i2c_write(void *opaque, hwaddr offset,
>          break;
>      case I2CADD_ADDR:
>          if ((s->i2cstat & I2CSTAT_OUTPUT_EN) == 0) {
> -            s->i2cadd = v;
> +            s->i2cadd = v & ~1;

Maybe a comment is a good idea here, saying that bit0 is unmapped.

Thanks,

/mjt

Re: [Qemu-devel] [PATCH for 2.10 v2 00/20] fix bugs reported by Clang Static Analyzer

[Michael Tokarev]

27.07.2017 05:42, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> This series is the result of [now NOT] having fun with Clang's Static Analyzer
> (see https://clang-analyzer.llvm.org/).

Applied almost all to -trivial, except of the ones wich had NACKs and questions.

Thanks,

/mjt