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

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

[Philippe Mathieu-Daudé]

Hi,

This series is the result of having fun with Clang's Static Analyzer during
the week-end (see https://clang-analyzer.llvm.org/).

No need to delay the first batch for -rc0.

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

I built a docker image based on debian/unstable to use bleeding code, the
Dockerfile is provided at the end of this series as RFC for people wanted
to give a try (also include 'dockershell' script and a fix in ./configure).
These RFC patches (33-35) are not expected to enter 2.10.

I used the following commands:

$ make docker-image-debian-bleeding-dev
$ ./scripts/dockershell debian-bleeding-dev
$ mkdir -p build
$ cd build
$ ../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

Regards,

Phil.

Philippe Mathieu-Daudé (35):
  tests: add missing dependency to build QTEST_QEMU_BINARY
  loader: check get_image_size() return value
  thunk: check nb_fields is valid before continuing
  ivshmem: fix incorrect error handling in ivshmem_recv_msg()
  nbd: fix memory leak in nbd_opt_go()
  qcow2: remove inconsistent check
  qcow2: fix null pointer dereference
  qcow2: fix null pointer dereference
  ui/vnc: fix leak of SocketAddress **
  net/eth: fix incorrect check of iov_to_buf() return value
  i2c/exynos4210: correctly check i2c_recv() return value
  vfio/platform: fix use of freed memory
  vfio/pci: fix use of freed memory
  vfio/ccw: fix incorrect malloc() size
  usb: correctly handle Zero Length Packets
  usb/dev-mtp: fix use of uninitialized values
  usb/dev-mtp: fix use of uninitialized values
  usb/dev-mtp: fix use of uninitialized values
  usb/dev-mtp: fix use of uninitialized variable
  arm/boot: fix undefined instruction on secondary smp cpu bootloader
  arm/sysbus-fdt: fix null pointer dereference
  arm/vexpress: fix potential memory leak
  m68k/translate: fix incorrect copy/paste
  linux-user/sh4: fix incorrect memory write
  linux-user: extract is_error() out of syscall.c
  linux-user: use is_error() to avoid warnings and make the code clearer
  syscall: fix dereference of undefined pointer
  syscall: check dup/dup2/dup3() errors, return EBADFD/EINVAL if
    required
  syscall: fix out-of-bound memory access
  syscall: fix use of uninitialized values
  syscall: replace strcpy() by g_strlcpy()
  timer/pxa2xx: silent warning about out-of-bound memory access
  configure: clang does not support -Wexpansion-to-defined
  docker: add debian 'bleeding' image with gcc7 and clang5
  script to run docker image

 block/qcow2-bitmap.c                               |  4 +-
 block/qcow2-refcount.c                             |  4 +-
 configure                                          |  6 +-
 hw/arm/boot.c                                      |  2 +-
 hw/arm/sysbus-fdt.c                                | 11 ++-
 hw/arm/vexpress.c                                  |  5 +-
 hw/core/loader.c                                   |  4 +-
 hw/i2c/exynos4210_i2c.c                            |  8 +-
 hw/misc/ivshmem.c                                  |  5 +-
 hw/timer/pxa2xx_timer.c                            |  2 +-
 hw/usb/dev-mtp.c                                   | 60 +++++++++-----
 hw/usb/redirect.c                                  |  2 +-
 hw/vfio/ccw.c                                      |  2 +-
 hw/vfio/pci.c                                      | 11 ++-
 hw/vfio/platform.c                                 |  2 +-
 linux-user/elfload.c                               |  2 +-
 linux-user/flatload.c                              | 15 ++--
 linux-user/qemu.h                                  |  5 ++
 linux-user/syscall.c                               | 45 +++++++----
 nbd/client.c                                       |  8 +-
 net/eth.c                                          |  4 +-
 scripts/dockershell                                | 30 +++++++
 target/m68k/translate.c                            |  3 +-
 tests/Makefile.include                             |  2 +-
 .../docker/dockerfiles/debian-bleeding-dev.docker  | 94 ++++++++++++++++++++++
 thunk.c                                            |  5 +-
 ui/vnc.c                                           | 36 ++++-----
 27 files changed, 279 insertions(+), 98 deletions(-)
 create mode 100755 scripts/dockershell
 create mode 100644 tests/docker/dockerfiles/debian-bleeding-dev.docker

-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 01/35] 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 02/35] 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>
---
 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

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

[Eric Blake]

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

On 07/24/2017 01:27 PM, Philippe Mathieu-Daudé wrote:
> 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>
> ---
>  hw/core/loader.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 --]

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

[Alistair Francis]

On Mon, Jul 24, 2017 at 11:38 AM, Eric Blake <eblake@redhat.com> wrote:
> On 07/24/2017 01:27 PM, Philippe Mathieu-Daudé wrote:
>> 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>
>> ---
>>  hw/core/loader.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,
Alistair

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

[Qemu-devel] [PATCH for 2.10 03/35] thunk: check nb_fields is valid before continuing

[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 | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/thunk.c b/thunk.c
index 2dac36666d..d1c5e221f5 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,10 @@ void thunk_register_struct(int id, const char *name, const argtype *types)
         type_ptr = thunk_type_next(type_ptr);
         nb_fields++;
     }
+    if (!nb_fields) {
+        return;
+    }
+    se = struct_entries + id;
     se->field_types = types;
     se->nb_fields = nb_fields;
     se->name = name;
-- 
2.13.3

Re: [Qemu-devel] [PATCH for 2.10 03/35] thunk: check nb_fields is valid before continuing

[Eric Blake]

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

On 07/24/2017 01:27 PM, Philippe Mathieu-Daudé wrote:
> 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 | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

Better would be fixing the code to use g_new0, and the corresponding free.

-- 
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 --]

Re: [Qemu-devel] [PATCH for 2.10 03/35] thunk: check nb_fields is valid before continuing

[Philippe Mathieu-Daudé]

On 07/24/2017 03:37 PM, Eric Blake wrote:
> On 07/24/2017 01:27 PM, Philippe Mathieu-Daudé wrote:
>> 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 | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> Better would be fixing the code to use g_new0, and the corresponding free.

Ok, for 2.11 although (not a fix).

Also thunk* alloc'd are never free'd during process lifetime, so will 
keep like that (no g_free).

Re: [Qemu-devel] [PATCH for 2.10 03/35] thunk: check nb_fields is valid before continuing

[Peter Maydell]

On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> 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 | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/thunk.c b/thunk.c
> index 2dac36666d..d1c5e221f5 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,10 @@ void thunk_register_struct(int id, const char *name, const argtype *types)
>          type_ptr = thunk_type_next(type_ptr);
>          nb_fields++;
>      }
> +    if (!nb_fields) {
> +        return;
> +    }

Can this ever actually happen? We only call this function
for a fixed set of known-at-compile-time data (it's invoked
by all the STRUCT() macro uses). It seems likely that it
would be better to make this an assert() and check that none
of our uses of STRUCT() cause it to fire.

> +    se = struct_entries + id;
>      se->field_types = types;
>      se->nb_fields = nb_fields;
>      se->name = name;
> --
> 2.13.3
>

thanks
-- PMM

[Qemu-devel] [PATCH for 2.10 04/35] ivshmem: fix incorrect error handling in ivshmem_recv_msg()

[Philippe Mathieu-Daudé]

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>
---
get_maintainer.pl: No maintainers found!

 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

Re: [Qemu-devel] [PATCH for 2.10 04/35] ivshmem: fix incorrect error handling in ivshmem_recv_msg()

[Markus Armbruster]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> 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));
>              ^
>

Let's add "Screwed up in commit 3a55fc0f, v2.6.0."

> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> get_maintainer.pl: No maintainers found!
>
>  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;
>          }

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Paolo, you taking this through your miscellaneous queue would save me
(and possibly Peter) a bit of work.  Only if you have something queued
already.  Let me know.

Re: [Qemu-devel] [PATCH for 2.10 04/35] ivshmem: fix incorrect error handling in ivshmem_recv_msg()

[Paolo Bonzini]

On 25/07/2017 10:18, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> 
>> 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));
>>              ^
>>
> 
> Let's add "Screwed up in commit 3a55fc0f, v2.6.0."
> 
>> Reported-by: Clang Static Analyzer
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> get_maintainer.pl: No maintainers found!
>>
>>  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;
>>          }
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 
> Paolo, you taking this through your miscellaneous queue would save me
> (and possibly Peter) a bit of work.  Only if you have something queued
> already.  Let me know.

Fair enough, I'll pick this up.

Paolo

[Qemu-devel] [PATCH for 2.10 05/35] 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>
---
 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 05/35] nbd: fix memory leak in nbd_opt_go()

[Eric Blake]

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

On 07/24/2017 01:27 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>
> ---
>  nbd/client.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

I plan to take this through the NBD tree for 2.10, if no one else picks
up the entire series first.

-- 
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 06/35] qcow2: remove inconsistent check

[Philippe Mathieu-Daudé]

This is equivalent to assert(russian roulette) so better remove it.

block/qcow2-bitmap.c:259:29: warning: The left operand of '==' is a garbage value
        assert(bitmap_table == NULL);
               ~~~~~~~~~~~~ ^

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 block/qcow2-bitmap.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 3e8735a20d..fe72df5057 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -254,7 +254,6 @@ static int free_bitmap_clusters(BlockDriverState *bs, Qcow2BitmapTable *tb)
 
     ret = bitmap_table_load(bs, tb, &bitmap_table);
     if (ret < 0) {
-        assert(bitmap_table == NULL);
         return ret;
     }
 
-- 
2.13.3

Re: [Qemu-devel] [PATCH for 2.10 06/35] qcow2: remove inconsistent check

[Eric Blake]

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

On 07/24/2017 01:27 PM, Philippe Mathieu-Daudé wrote:
> This is equivalent to assert(russian roulette) so better remove it.
> 
> block/qcow2-bitmap.c:259:29: warning: The left operand of '==' is a garbage value
>         assert(bitmap_table == NULL);
>                ~~~~~~~~~~~~ ^
> 
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  block/qcow2-bitmap.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 3e8735a20d..fe72df5057 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -254,7 +254,6 @@ static int free_bitmap_clusters(BlockDriverState *bs, Qcow2BitmapTable *tb)
>  
>      ret = bitmap_table_load(bs, tb, &bitmap_table);
>      if (ret < 0) {
> -        assert(bitmap_table == NULL);

Rather, we should fix bitmap_table_load() to ensure that bitmap_table is
always assigned, even on error.

-- 
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 --]

Re: [Qemu-devel] [PATCH for 2.10 06/35] qcow2: remove inconsistent check

[Paolo Bonzini]

On 24/07/2017 20:42, Eric Blake wrote:
> On 07/24/2017 01:27 PM, Philippe Mathieu-Daudé wrote:
>> This is equivalent to assert(russian roulette) so better remove it.
>>
>> block/qcow2-bitmap.c:259:29: warning: The left operand of '==' is a garbage value
>>         assert(bitmap_table == NULL);
>>                ~~~~~~~~~~~~ ^
>>
>> Reported-by: Clang Static Analyzer
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  block/qcow2-bitmap.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 3e8735a20d..fe72df5057 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -254,7 +254,6 @@ static int free_bitmap_clusters(BlockDriverState *bs, Qcow2BitmapTable *tb)
>>  
>>      ret = bitmap_table_load(bs, tb, &bitmap_table);
>>      if (ret < 0) {
>> -        assert(bitmap_table == NULL);
> 
> Rather, we should fix bitmap_table_load() to ensure that bitmap_table is
> always assigned, even on error.

I think it's even better to initialize bitmap_table to NULL in
free_bitmap_clusters, as all other callers do.

Paolo

[Qemu-devel] [PATCH for 2.10 07/35] qcow2: fix null pointer dereference

[Philippe Mathieu-Daudé]

If find_bitmap_by_name() fails we have bm=NULL and go to the 'fail' label, then
call bitmap_free(bm) which does g_free(bm->name) with bm=NULL...

Clang's scan-build-5.0 output:
block/qcow2-bitmap.c:492:12: warning: Access to field 'name' results in a dereference of a null pointer (loaded from variable 'bm')
    g_free(bm->name);
           ^~~~~~~~

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

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index fe72df5057..2fd75781ce 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1259,7 +1259,7 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
 
     bm = find_bitmap_by_name(bm_list, name);
     if (bm == NULL) {
-        goto fail;
+        goto fail_list;
     }
 
     QSIMPLEQ_REMOVE(bm_list, bm, Qcow2Bitmap, entry);
@@ -1274,6 +1274,7 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
 
 fail:
     bitmap_free(bm);
+fail_list:
     bitmap_list_free(bm_list);
 }
 
-- 
2.13.3

Re: [Qemu-devel] [PATCH for 2.10 07/35] qcow2: fix null pointer dereference

[Eric Blake]

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

On 07/24/2017 01:27 PM, Philippe Mathieu-Daudé wrote:
> If find_bitmap_by_name() fails we have bm=NULL and go to the 'fail' label, then
> call bitmap_free(bm) which does g_free(bm->name) with bm=NULL...
> 
> Clang's scan-build-5.0 output:
> block/qcow2-bitmap.c:492:12: warning: Access to field 'name' results in a dereference of a null pointer (loaded from variable 'bm')
>     g_free(bm->name);
>            ^~~~~~~~
> 
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  block/qcow2-bitmap.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Max already has a better patch from Vladimir pending on his block queue:
https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg04398.html

-- 
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 --]

Re: [Qemu-devel] [PATCH for 2.10 07/35] qcow2: fix null pointer dereference

[Philippe Mathieu-Daudé]

Hi Eric,

On 07/24/2017 03:46 PM, Eric Blake wrote:
> On 07/24/2017 01:27 PM, Philippe Mathieu-Daudé wrote:
>> If find_bitmap_by_name() fails we have bm=NULL and go to the 'fail' label, then
>> call bitmap_free(bm) which does g_free(bm->name) with bm=NULL...
>>
>> Clang's scan-build-5.0 output:
>> block/qcow2-bitmap.c:492:12: warning: Access to field 'name' results in a dereference of a null pointer (loaded from variable 'bm')
>>      g_free(bm->name);
>>             ^~~~~~~~
>>
>> Reported-by: Clang Static Analyzer
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   block/qcow2-bitmap.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Max already has a better patch from Vladimir pending on his block queue:
> https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg04398.html

Oh I missed that.
I'm not sure it is "better", it may be safer although ;)

Patch dropped.

Regards,

Phil.

[Qemu-devel] [PATCH for 2.10 08/35] 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>
---
 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

Re: [Qemu-devel] [PATCH for 2.10 08/35] qcow2: fix null pointer dereference

[Eric Blake]

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

On 07/24/2017 01:27 PM, Philippe Mathieu-Daudé wrote:
> 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>
> ---
>  block/qcow2-refcount.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> 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]);
>      }
> 

-- 
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 09/35] 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>
---
 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

Re: [Qemu-devel] [PATCH for 2.10 09/35] ui/vnc: fix leak of SocketAddress **

[Daniel P. Berrange]

On Mon, Jul 24, 2017 at 03:27:25PM -0300, Philippe Mathieu-Daudé wrote:
> 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>
> ---
>  ui/vnc.c | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[Qemu-devel] [PATCH for 2.10 10/35] 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>
---
 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

Re: [Qemu-devel] [PATCH for 2.10 10/35] net/eth: fix incorrect check of iov_to_buf() return value

[Dmitry Fleytman]

> On 24 Jul 2017, at 21:27 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> 
> 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>
> ---
> 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);
>         }
> 

Reviewed-by: Dmitry Fleytman <dmitry@daynix.com>

>         opt_offset += optlen;
> -- 
> 2.13.3
> 

[Qemu-devel] [PATCH for 2.10 11/35] i2c/exynos4210: correctly check i2c_recv() return value

[Philippe Mathieu-Daudé]

i2c_recv() returns -1 on error, if the I2CCON_ACK_GEN bit was not set this code
was setting i2cds = -1.

i2c/exynos4210_i2c.c:117:20: warning: Loss of sign in implicit conversion
        s->i2cds = ret;
                   ^~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/i2c/exynos4210_i2c.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/i2c/exynos4210_i2c.c b/hw/i2c/exynos4210_i2c.c
index c96fa7d7be..4424dbd233 100644
--- a/hw/i2c/exynos4210_i2c.c
+++ b/hw/i2c/exynos4210_i2c.c
@@ -111,10 +111,12 @@ static void exynos4210_i2c_data_receive(void *opaque)
     s->i2cstat &= ~I2CSTAT_LAST_BIT;
     s->scl_free = false;
     ret = i2c_recv(s->bus);
-    if (ret < 0 && (s->i2ccon & I2CCON_ACK_GEN)) {
-        s->i2cstat |= I2CSTAT_LAST_BIT;  /* Data is not acknowledged */
-    } else {
+    if (ret >= 0) {
         s->i2cds = ret;
+    } else {
+        if (s->i2ccon & I2CCON_ACK_GEN) {
+            s->i2cstat |= I2CSTAT_LAST_BIT;  /* Data is not acknowledged */
+        }
     }
     exynos4210_i2c_raise_interrupt(s);
 }
-- 
2.13.3

Re: [Qemu-devel] [PATCH for 2.10 11/35] i2c/exynos4210: correctly check i2c_recv() return value

[Peter Maydell]

On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> i2c_recv() returns -1 on error, if the I2CCON_ACK_GEN bit was not set this code
> was setting i2cds = -1.
>
> i2c/exynos4210_i2c.c:117:20: warning: Loss of sign in implicit conversion
>         s->i2cds = ret;
>                    ^~~
>
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/i2c/exynos4210_i2c.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/hw/i2c/exynos4210_i2c.c b/hw/i2c/exynos4210_i2c.c
> index c96fa7d7be..4424dbd233 100644
> --- a/hw/i2c/exynos4210_i2c.c
> +++ b/hw/i2c/exynos4210_i2c.c
> @@ -111,10 +111,12 @@ static void exynos4210_i2c_data_receive(void *opaque)
>      s->i2cstat &= ~I2CSTAT_LAST_BIT;
>      s->scl_free = false;
>      ret = i2c_recv(s->bus);
> -    if (ret < 0 && (s->i2ccon & I2CCON_ACK_GEN)) {
> -        s->i2cstat |= I2CSTAT_LAST_BIT;  /* Data is not acknowledged */
> -    } else {
> +    if (ret >= 0) {
>          s->i2cds = ret;
> +    } else {
> +        if (s->i2ccon & I2CCON_ACK_GEN) {
> +            s->i2cstat |= I2CSTAT_LAST_BIT;  /* Data is not acknowledged */
> +        }
>      }
>      exynos4210_i2c_raise_interrupt(s);
>  }
> --

Have you checked this change against the data sheet for
the device?

thanks
-- PMM

Re: [Qemu-devel] [PATCH for 2.10 11/35] i2c/exynos4210: correctly check i2c_recv() return value

[Philippe Mathieu-Daudé]

On 07/24/2017 06:13 PM, Peter Maydell wrote:
> On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> i2c_recv() returns -1 on error, if the I2CCON_ACK_GEN bit was not set this code
>> was setting i2cds = -1.
>>
>> i2c/exynos4210_i2c.c:117:20: warning: Loss of sign in implicit conversion
>>          s->i2cds = ret;
>>                     ^~~
>>
>> Reported-by: Clang Static Analyzer
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   hw/i2c/exynos4210_i2c.c | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/i2c/exynos4210_i2c.c b/hw/i2c/exynos4210_i2c.c
>> index c96fa7d7be..4424dbd233 100644
>> --- a/hw/i2c/exynos4210_i2c.c
>> +++ b/hw/i2c/exynos4210_i2c.c
>> @@ -111,10 +111,12 @@ static void exynos4210_i2c_data_receive(void *opaque)
>>       s->i2cstat &= ~I2CSTAT_LAST_BIT;
>>       s->scl_free = false;
>>       ret = i2c_recv(s->bus);
>> -    if (ret < 0 && (s->i2ccon & I2CCON_ACK_GEN)) {
>> -        s->i2cstat |= I2CSTAT_LAST_BIT;  /* Data is not acknowledged */
>> -    } else {
>> +    if (ret >= 0) {
>>           s->i2cds = ret;
>> +    } else {
>> +        if (s->i2ccon & I2CCON_ACK_GEN) {
>> +            s->i2cstat |= I2CSTAT_LAST_BIT;  /* Data is not acknowledged */
>> +        }
>>       }
>>       exynos4210_i2c_raise_interrupt(s);
>>   }
>> --
> 
> Have you checked this change against the data sheet for
> the device?

Here is the relevant part of the Exynos4210_UM DS:

[*] 14.3.5 Data Transfer Format
... If the I2C-bus is operating in Master mode, master transmits the 
address field. Each byte should be followed by an acknowledgement (ACK) bit.

[*] 14.3.6 ACK Signal Transmission
To complete a one-byte transfer operation, the receiver sends an ACK bit 
to the transmitter. The ACK pulse occurs at the ninth clock of the SCL 
line. ...
The software (I2CSTAT) enables or disables ACK bit transmit function. 
However, the ACK pulse on the ninth clock of SCL is required to complete 
the one-byte data transfer operation.

[*] 14.3.9 Abort Conditions
If a slave receiver cannot acknowledge the confirmation of the slave 
address, it holds the level of the SDA line High. In this case, the 
master generates a Stop condition and cancels the transfer.
If a master receiver is involved in the aborted transfer, it signals the 
end of slave transmit operation by canceling the generation of an ACK 
after the last data byte received from the slave. The slave transmitter 
releases the SDA to allow a master to generate a Stop condition.

I2C-bus last-received bit status flag bit. (I2CSTAT_LAST_BIT)
0 = Last-received bit is 0 (ACK was received).
1 = Last-received bit is 1 (ACK was not received).

An I2C-bus interrupt occurs if 1) if a 1-byte transmit or receive 
operation is complete. In other words, ack period is finished. 2) A 
general call or a slave address match occurs, 3) Bus arbitration fails.

--

So the current code is not wrong and matches the datashit, crap is 
shifted into I2CDS and the guest has to poll I2CSTAT to check the peer 
ACK... Still it is weird to shift 0xff as of the current implementation, 
but it might have some usefulness while debugging, who knows...

I might add few comments in that file during 2.11 cycle.

Thank for the review!

Patch dropped.

Regards,

Phil.

[Qemu-devel] [PATCH for 2.10 12/35] 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>
---
 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

Re: [Qemu-devel] [PATCH for 2.10 12/35] vfio/platform: fix use of freed memory

[Paolo Bonzini]

On 24/07/2017 20:27, Philippe Mathieu-Daudé wrote:
> 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>
> ---
>  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;
>  }
>  
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH for 2.10 12/35] vfio/platform: fix use of freed memory

[Alex Williamson]

On Mon, 24 Jul 2017 15:27:28 -0300
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> 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>
> ---
>  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;
>  }
>  

I'll snag this and 13/35 for a pull request with Paolo's R-b.  Connie
has already put a fix in for the other vfio related one, 14/35.  Thanks!

Alex

[Qemu-devel] [PATCH for 2.10 13/35] 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>
---
 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

Re: [Qemu-devel] [PATCH for 2.10 13/35] vfio/pci: fix use of freed memory

[Paolo Bonzini]

On 24/07/2017 20:27, Philippe Mathieu-Daudé wrote:
> 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>
> ---
>  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)
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

[Qemu-devel] [PATCH for 2.10 14/35] vfio/ccw: fix incorrect malloc() size

[Philippe Mathieu-Daudé]

Since sizeof(struct vfio_irq_info) < sizeof(struct vfio_irq_set) a heap overflow
never occured. Still, let's use the correct size.

hw/vfio/ccw.c:170:16: warning: Cast a region whose size is not a multiple of the destination type size
    irq_info = g_malloc0(sizeof(*irq_set));
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~

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

diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 12d0262336..8d97b53e77 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -168,7 +168,7 @@ static void vfio_ccw_register_io_notifier(VFIOCCWDevice *vcdev, Error **errp)
         return;
     }
 
-    argsz = sizeof(*irq_set);
+    argsz = sizeof(*irq_info);
     irq_info = g_malloc0(argsz);
     irq_info->index = VFIO_CCW_IO_IRQ_INDEX;
     irq_info->argsz = argsz;
-- 
2.13.3

Re: [Qemu-devel] [PATCH for 2.10 14/35] vfio/ccw: fix incorrect malloc() size

[Cornelia Huck]

On Mon, 24 Jul 2017 15:27:30 -0300
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> Since sizeof(struct vfio_irq_info) < sizeof(struct vfio_irq_set) a heap overflow
> never occured. Still, let's use the correct size.
> 
> hw/vfio/ccw.c:170:16: warning: Cast a region whose size is not a multiple of the destination type size
>     irq_info = g_malloc0(sizeof(*irq_set));
>                ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/vfio/ccw.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 12d0262336..8d97b53e77 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -168,7 +168,7 @@ static void vfio_ccw_register_io_notifier(VFIOCCWDevice *vcdev, Error **errp)
>          return;
>      }
>  
> -    argsz = sizeof(*irq_set);
> +    argsz = sizeof(*irq_info);
>      irq_info = g_malloc0(argsz);
>      irq_info->index = VFIO_CCW_IO_IRQ_INDEX;
>      irq_info->argsz = argsz;

Thanks for the patch, but I already have "vfio/ccw: allocate irq info
with the right size" queued in my s390-next branch (for which I plan to
send a pull req today).

[Qemu-devel] [PATCH for 2.10 15/35] usb: correctly handle Zero Length Packets

[Philippe Mathieu-Daudé]

USB Specification Revision 2.0, §5.5.3:
  The Data stage of a control transfer from an endpoint to the host is complete when the endpoint does one of the following:
  • Has transferred exactly the amount of data specified during the Setup stage
  • Transfers a packet with a payload size less than wMaxPacketSize or transfers a zero-length packet"

hw/usb/redirect.c:802:9: warning: Declared variable-length array (VLA) has zero size
        uint8_t buf[size];
        ^~~~~~~~~~~ ~~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/usb/redirect.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 5e42730449..5b7073d2cd 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -795,7 +795,7 @@ static void usbredir_handle_bulk_data(USBRedirDevice *dev, USBPacket *p,
            usbredirparser_peer_has_cap(dev->parser,
                                        usb_redir_cap_32bits_bulk_length));
 
-    if (ep & USB_DIR_IN) {
+    if (ep & USB_DIR_IN || size == 0) {
         usbredirparser_send_bulk_packet(dev->parser, p->id,
                                         &bulk_packet, NULL, 0);
     } else {
-- 
2.13.3

Re: [Qemu-devel] [PATCH for 2.10 15/35] usb: correctly handle Zero Length Packets

[Philippe Mathieu-Daudé]

On 07/24/2017 03:27 PM, Philippe Mathieu-Daudé wrote:
> USB Specification Revision 2.0, §5.5.3:
>   The Data stage of a control transfer from an endpoint to the host is complete when the endpoint does one of the following:
>   • Has transferred exactly the amount of data specified during the Setup stage
>   • Transfers a packet with a payload size less than wMaxPacketSize or transfers a zero-length packet"
> 
> hw/usb/redirect.c:802:9: warning: Declared variable-length array (VLA) has zero size
>         uint8_t buf[size];
>         ^~~~~~~~~~~ ~~~~
> 
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/usb/redirect.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> index 5e42730449..5b7073d2cd 100644
> --- a/hw/usb/redirect.c
> +++ b/hw/usb/redirect.c
> @@ -795,7 +795,7 @@ static void usbredir_handle_bulk_data(USBRedirDevice *dev, USBPacket *p,
>             usbredirparser_peer_has_cap(dev->parser,
>                                         usb_redir_cap_32bits_bulk_length));
>  
> -    if (ep & USB_DIR_IN) {
> +    if (ep & USB_DIR_IN || size == 0) {
>          usbredirparser_send_bulk_packet(dev->parser, p->id,
>                                          &bulk_packet, NULL, 0);
>      } else {
> 

Ping?

Re: [Qemu-devel] [PATCH for 2.10 15/35] usb: correctly handle Zero Length Packets

[Gerd Hoffmann]

On Tue, May 29, 2018 at 11:22:46AM -0300, Philippe Mathieu-Daudé wrote:
> On 07/24/2017 03:27 PM, Philippe Mathieu-Daudé wrote:
> > USB Specification Revision 2.0, §5.5.3:
> >   The Data stage of a control transfer from an endpoint to the host is complete when the endpoint does one of the following:
> >   • Has transferred exactly the amount of data specified during the Setup stage
> >   • Transfers a packet with a payload size less than wMaxPacketSize or transfers a zero-length packet"
> > 
> > hw/usb/redirect.c:802:9: warning: Declared variable-length array (VLA) has zero size
> >         uint8_t buf[size];
> >         ^~~~~~~~~~~ ~~~~
> > 
> > Reported-by: Clang Static Analyzer
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> >  hw/usb/redirect.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> > index 5e42730449..5b7073d2cd 100644
> > --- a/hw/usb/redirect.c
> > +++ b/hw/usb/redirect.c
> > @@ -795,7 +795,7 @@ static void usbredir_handle_bulk_data(USBRedirDevice *dev, USBPacket *p,
> >             usbredirparser_peer_has_cap(dev->parser,
> >                                         usb_redir_cap_32bits_bulk_length));
> >  
> > -    if (ep & USB_DIR_IN) {
> > +    if (ep & USB_DIR_IN || size == 0) {
> >          usbredirparser_send_bulk_packet(dev->parser, p->id,
> >                                          &bulk_packet, NULL, 0);
> >      } else {
> > 
> 
> Ping?

Whoops, pretty old one.  Don't have this in my qemu-devel archive any
more.  Can you rebase + resend?

thanks,
  Gerd

[Qemu-devel] [PATCH for 2.10 16/35] usb/dev-mtp: fix use of uninitialized values

[Philippe Mathieu-Daudé]

hw/usb/dev-mtp.c:1212:13: warning: 2nd function call argument is an uninitialized value
        o = usb_mtp_object_lookup(s, c->argv[0]);
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

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

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 94c2e94f10..6dfece9ea9 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1209,7 +1209,9 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
         }
         break;
     case CMD_GET_OBJECT_INFO:
-        o = usb_mtp_object_lookup(s, c->argv[0]);
+        if (c->argc > 0) {
+            o = usb_mtp_object_lookup(s, c->argv[0]);
+        }
         if (o == NULL) {
             usb_mtp_queue_result(s, RES_INVALID_OBJECT_HANDLE,
                                  c->trans, 0, 0, 0);
@@ -1218,7 +1220,9 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
         data_in = usb_mtp_get_object_info(s, c, o);
         break;
     case CMD_GET_OBJECT:
-        o = usb_mtp_object_lookup(s, c->argv[0]);
+        if (c->argc > 0) {
+            o = usb_mtp_object_lookup(s, c->argv[0]);
+        }
         if (o == NULL) {
             usb_mtp_queue_result(s, RES_INVALID_OBJECT_HANDLE,
                                  c->trans, 0, 0, 0);
@@ -1237,7 +1241,9 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
         }
         break;
     case CMD_GET_PARTIAL_OBJECT:
-        o = usb_mtp_object_lookup(s, c->argv[0]);
+        if (c->argc > 0) {
+            o = usb_mtp_object_lookup(s, c->argv[0]);
+        }
         if (o == NULL) {
             usb_mtp_queue_result(s, RES_INVALID_OBJECT_HANDLE,
                                  c->trans, 0, 0, 0);
@@ -1281,7 +1287,9 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
         }
         break;
     case CMD_GET_OBJECT_PROP_VALUE:
-        o = usb_mtp_object_lookup(s, c->argv[0]);
+        if (c->argc > 0) {
+            o = usb_mtp_object_lookup(s, c->argv[0]);
+        }
         if (o == NULL) {
             usb_mtp_queue_result(s, RES_INVALID_OBJECT_HANDLE,
                                  c->trans, 0, 0, 0);
-- 
2.13.3

Re: [Qemu-devel] [PATCH for 2.10 16/35] usb/dev-mtp: fix use of uninitialized values

[Gerd Hoffmann]

     case CMD_GET_OBJECT_INFO:
> -        o = usb_mtp_object_lookup(s, c->argv[0]);
> +        if (c->argc > 0) {
> +            o = usb_mtp_object_lookup(s, c->argv[0]);
> +        }

How about zero-initializing c->argv instead?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH for 2.10 16/35] usb/dev-mtp: fix use of uninitialized values

[Philippe Mathieu-Daudé]

On 07/25/2017 09:34 AM, Gerd Hoffmann wrote:
>       case CMD_GET_OBJECT_INFO:
>> -        o = usb_mtp_object_lookup(s, c->argv[0]);
>> +        if (c->argc > 0) {
>> +            o = usb_mtp_object_lookup(s, c->argv[0]);
>> +        }
> 
> How about zero-initializing c->argv instead?

I checked the MTP specs rev. 1.1 and I understand the case argc == 0 
fits in "Invalid Parameter" section (F.2.30, code 0x201d).

So the correct patch is to queue a RES_INVALID_PARAMETER result.

I'll send another patch but since this require heavy testing this is 
probably 2.11 material now.

Regards,

Phil.

[Qemu-devel] [PATCH for 2.10 17/35] usb/dev-mtp: fix use of uninitialized values

[Philippe Mathieu-Daudé]

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/usb/dev-mtp.c | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 6dfece9ea9..ad64495f05 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1134,7 +1134,7 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
                                  c->trans, 1, s->session, 0);
             return;
         }
-        if (c->argv[0] == 0) {
+        if (c->argc == 0 || c->argv[0] == 0) {
             usb_mtp_queue_result(s, RES_INVALID_PARAMETER,
                                  c->trans, 0, 0, 0);
             return;
@@ -1162,8 +1162,9 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
         data_in = usb_mtp_get_storage_ids(s, c);
         break;
     case CMD_GET_STORAGE_INFO:
-        if (c->argv[0] != QEMU_STORAGE_ID &&
-            c->argv[0] != 0xffffffff) {
+        if (c->argc == 0 ||
+           (c->argv[0] != QEMU_STORAGE_ID &&
+            c->argv[0] != 0xffffffff)) {
             usb_mtp_queue_result(s, RES_INVALID_STORAGE_ID,
                                  c->trans, 0, 0, 0);
             return;
@@ -1172,22 +1173,25 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
         break;
     case CMD_GET_NUM_OBJECTS:
     case CMD_GET_OBJECT_HANDLES:
-        if (c->argv[0] != QEMU_STORAGE_ID &&
-            c->argv[0] != 0xffffffff) {
+        if (c->argc == 0 ||
+           (c->argv[0] != QEMU_STORAGE_ID &&
+            c->argv[0] != 0xffffffff)) {
             usb_mtp_queue_result(s, RES_INVALID_STORAGE_ID,
                                  c->trans, 0, 0, 0);
             return;
         }
-        if (c->argv[1] != 0x00000000) {
+        if (c->argc > 1 && c->argv[1] != 0x00000000) {
             usb_mtp_queue_result(s, RES_SPEC_BY_FORMAT_UNSUPPORTED,
                                  c->trans, 0, 0, 0);
             return;
         }
-        if (c->argv[2] == 0x00000000 ||
-            c->argv[2] == 0xffffffff) {
-            o = QTAILQ_FIRST(&s->objects);
-        } else {
-            o = usb_mtp_object_lookup(s, c->argv[2]);
+        if (c->argc > 2) {
+            if (c->argv[2] == 0x00000000 ||
+                c->argv[2] == 0xffffffff) {
+                o = QTAILQ_FIRST(&s->objects);
+            } else {
+                o = usb_mtp_object_lookup(s, c->argv[2]);
+            }
         }
         if (o == NULL) {
             usb_mtp_queue_result(s, RES_INVALID_OBJECT_HANDLE,
@@ -1264,8 +1268,9 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
         res0 = data_in->length;
         break;
     case CMD_GET_OBJECT_PROPS_SUPPORTED:
-        if (c->argv[0] != FMT_UNDEFINED_OBJECT &&
-            c->argv[0] != FMT_ASSOCIATION) {
+        if (c->argc == 0 ||
+           (c->argv[0] != FMT_UNDEFINED_OBJECT &&
+            c->argv[0] != FMT_ASSOCIATION)) {
             usb_mtp_queue_result(s, RES_INVALID_OBJECT_FORMAT_CODE,
                                  c->trans, 0, 0, 0);
             return;
@@ -1273,8 +1278,9 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
         data_in = usb_mtp_get_object_props_supported(s, c);
         break;
     case CMD_GET_OBJECT_PROP_DESC:
-        if (c->argv[1] != FMT_UNDEFINED_OBJECT &&
-            c->argv[1] != FMT_ASSOCIATION) {
+        if (c->argc > 1 &&
+           (c->argv[1] != FMT_UNDEFINED_OBJECT &&
+            c->argv[1] != FMT_ASSOCIATION)) {
             usb_mtp_queue_result(s, RES_INVALID_OBJECT_FORMAT_CODE,
                                  c->trans, 0, 0, 0);
             return;
-- 
2.13.3

Re: [Qemu-devel] [PATCH for 2.10 17/35] usb/dev-mtp: fix use of uninitialized values

[Philippe Mathieu-Daudé]

On 07/24/2017 03:27 PM, Philippe Mathieu-Daudé wrote:
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/usb/dev-mtp.c | 36 +++++++++++++++++++++---------------
>   1 file changed, 21 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> index 6dfece9ea9..ad64495f05 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -1134,7 +1134,7 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
>                                    c->trans, 1, s->session, 0);
>               return;
>           }
> -        if (c->argv[0] == 0) {
> +        if (c->argc == 0 || c->argv[0] == 0) {
>               usb_mtp_queue_result(s, RES_INVALID_PARAMETER,
>                                    c->trans, 0, 0, 0);

^ This is OK,

but part below is incorrect, after reading the MTP specs 1.1 I 
understood the correct code to return is RES_INVALID_PARAMETER.

>               return;
> @@ -1162,8 +1162,9 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
>           data_in = usb_mtp_get_storage_ids(s, c);
>           break;
>       case CMD_GET_STORAGE_INFO:
> -        if (c->argv[0] != QEMU_STORAGE_ID &&
> -            c->argv[0] != 0xffffffff) {
> +        if (c->argc == 0 ||
> +           (c->argv[0] != QEMU_STORAGE_ID &&
> +            c->argv[0] != 0xffffffff)) {
>               usb_mtp_queue_result(s, RES_INVALID_STORAGE_ID,
>                                    c->trans, 0, 0, 0);
>               return;
> @@ -1172,22 +1173,25 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
>           break;
>       case CMD_GET_NUM_OBJECTS:
>       case CMD_GET_OBJECT_HANDLES:
> -        if (c->argv[0] != QEMU_STORAGE_ID &&
> -            c->argv[0] != 0xffffffff) {
> +        if (c->argc == 0 ||
> +           (c->argv[0] != QEMU_STORAGE_ID &&
> +            c->argv[0] != 0xffffffff)) {
>               usb_mtp_queue_result(s, RES_INVALID_STORAGE_ID,
>                                    c->trans, 0, 0, 0);
>               return;
>           }
> -        if (c->argv[1] != 0x00000000) {
> +        if (c->argc > 1 && c->argv[1] != 0x00000000) {
>               usb_mtp_queue_result(s, RES_SPEC_BY_FORMAT_UNSUPPORTED,
>                                    c->trans, 0, 0, 0);
>               return;
>           }
> -        if (c->argv[2] == 0x00000000 ||
> -            c->argv[2] == 0xffffffff) {
> -            o = QTAILQ_FIRST(&s->objects);
> -        } else {
> -            o = usb_mtp_object_lookup(s, c->argv[2]);
> +        if (c->argc > 2) {
> +            if (c->argv[2] == 0x00000000 ||
> +                c->argv[2] == 0xffffffff) {
> +                o = QTAILQ_FIRST(&s->objects);
> +            } else {
> +                o = usb_mtp_object_lookup(s, c->argv[2]);
> +            }
>           }
>           if (o == NULL) {
>               usb_mtp_queue_result(s, RES_INVALID_OBJECT_HANDLE,
> @@ -1264,8 +1268,9 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
>           res0 = data_in->length;
>           break;
>       case CMD_GET_OBJECT_PROPS_SUPPORTED:
> -        if (c->argv[0] != FMT_UNDEFINED_OBJECT &&
> -            c->argv[0] != FMT_ASSOCIATION) {
> +        if (c->argc == 0 ||
> +           (c->argv[0] != FMT_UNDEFINED_OBJECT &&
> +            c->argv[0] != FMT_ASSOCIATION)) {
>               usb_mtp_queue_result(s, RES_INVALID_OBJECT_FORMAT_CODE,
>                                    c->trans, 0, 0, 0);
>               return;
> @@ -1273,8 +1278,9 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
>           data_in = usb_mtp_get_object_props_supported(s, c);
>           break;
>       case CMD_GET_OBJECT_PROP_DESC:
> -        if (c->argv[1] != FMT_UNDEFINED_OBJECT &&
> -            c->argv[1] != FMT_ASSOCIATION) {
> +        if (c->argc > 1 &&
> +           (c->argv[1] != FMT_UNDEFINED_OBJECT &&
> +            c->argv[1] != FMT_ASSOCIATION)) {
>               usb_mtp_queue_result(s, RES_INVALID_OBJECT_FORMAT_CODE,
>                                    c->trans, 0, 0, 0);
>               return;
> 

[Qemu-devel] [PATCH for 2.10 18/35] usb/dev-mtp: fix use of uninitialized values

[Philippe Mathieu-Daudé]

hw/usb/dev-mtp.c:971:5: warning: 4th function call argument is an uninitialized value
    trace_usb_mtp_op_get_partial_object(s->dev.addr, o->handle, o->path,
                                         c->argv[1], c->argv[2]);
                                                     ^~~~~~~~~~
hw/usb/dev-mtp.c:981:12: warning: Assigned value is garbage or undefined
    offset = c->argv[1];
           ^ ~~~~~~~~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/usb/dev-mtp.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index ad64495f05..b4f0b5dafd 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -965,12 +965,16 @@ static MTPData *usb_mtp_get_object(MTPState *s, MTPControl *c,
 static MTPData *usb_mtp_get_partial_object(MTPState *s, MTPControl *c,
                                            MTPObject *o)
 {
-    MTPData *d = usb_mtp_data_alloc(c);
+    MTPData *d;
     off_t offset;
 
+    if (c->argc <= 2) {
+        return NULL;
+    }
     trace_usb_mtp_op_get_partial_object(s->dev.addr, o->handle, o->path,
                                         c->argv[1], c->argv[2]);
 
+    d = usb_mtp_data_alloc(c);
     d->fd = open(o->path, O_RDONLY);
     if (d->fd == -1) {
         usb_mtp_data_free(d);
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 19/35] usb/dev-mtp: fix use of uninitialized variable

[Philippe Mathieu-Daudé]

hw/usb/dev-mtp.c:1200:15: warning: The left operand of '==' is a garbage value
        if (o == NULL) {
            ~ ^

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/usb/dev-mtp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index b4f0b5dafd..5615fb3634 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1117,7 +1117,7 @@ static MTPData *usb_mtp_get_object_prop_value(MTPState *s, MTPControl *c,
 static void usb_mtp_command(MTPState *s, MTPControl *c)
 {
     MTPData *data_in = NULL;
-    MTPObject *o;
+    MTPObject *o = NULL;
     uint32_t nres = 0, res0 = 0;
 
     /* sanity checks */
-- 
2.13.3

[Qemu-devel] [PATCH for 2.10 20/35] arm/boot: fix undefined instruction on secondary smp cpu bootloader

[Philippe Mathieu-Daudé]

In a ARM multicore system, write_secondary_boot() only initializes fixups for
FIXUP_GIC_CPU_IF and FIXUP_BOOTREG, while smpboot[] also uses FIXUP_DSB.
This results in write_bootloader() using uninitialized fixupcontext[FIXUP_DSB]
instruction in the bootloader code...
Zero-initialize fixupcontext[] to avoid this issue.

hw/arm/boot.c:157:18: warning: Assigned value is garbage or undefined
            insn = fixupcontext[fixup];
                 ^ ~~~~~~~~~~~~~~~~~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/boot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index c2720c8046..fb21f113c2 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -170,7 +170,7 @@ static void write_bootloader(const char *name, hwaddr addr,
 static void default_write_secondary(ARMCPU *cpu,
                                     const struct arm_boot_info *info)
 {
-    uint32_t fixupcontext[FIXUP_MAX];
+    uint32_t fixupcontext[FIXUP_MAX] = {};
 
     fixupcontext[FIXUP_GIC_CPU_IF] = info->gic_cpu_if_addr;
     fixupcontext[FIXUP_BOOTREG] = info->smp_bootreg_addr;
-- 
2.13.3

Re: [Qemu-devel] [PATCH for 2.10 20/35] arm/boot: fix undefined instruction on secondary smp cpu bootloader

[Peter Maydell]

On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> In a ARM multicore system, write_secondary_boot() only initializes fixups for
> FIXUP_GIC_CPU_IF and FIXUP_BOOTREG, while smpboot[] also uses FIXUP_DSB.
> This results in write_bootloader() using uninitialized fixupcontext[FIXUP_DSB]
> instruction in the bootloader code...

Hmm? The code does:

    if (arm_feature(&cpu->env, ARM_FEATURE_V7)) {
        fixupcontext[FIXUP_DSB] = DSB_INSN;
    } else {
        fixupcontext[FIXUP_DSB] = CP15_DSB_INSN;
    }

so fixupcontext[FIXUP_DSB] is guaranteed initialized,
as are FIXUP_GIC_CPU_IF and FIXUP_BOOTREG, which are
the only fixups that the smpboot[] code uses.

thanks
-- PMM

Re: [Qemu-devel] [PATCH for 2.10 20/35] arm/boot: fix undefined instruction on secondary smp cpu bootloader

[Philippe Mathieu-Daudé]

On 07/24/2017 06:06 PM, Peter Maydell wrote:
> On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> In a ARM multicore system, write_secondary_boot() only initializes fixups for
>> FIXUP_GIC_CPU_IF and FIXUP_BOOTREG, while smpboot[] also uses FIXUP_DSB.
>> This results in write_bootloader() using uninitialized fixupcontext[FIXUP_DSB]
>> instruction in the bootloader code...
> 
> Hmm? The code does:
> 
>      if (arm_feature(&cpu->env, ARM_FEATURE_V7)) {
>          fixupcontext[FIXUP_DSB] = DSB_INSN;
>      } else {
>          fixupcontext[FIXUP_DSB] = CP15_DSB_INSN;
>      }
> 
> so fixupcontext[FIXUP_DSB] is guaranteed initialized,
> as are FIXUP_GIC_CPU_IF and FIXUP_BOOTREG, which are
> the only fixups that the smpboot[] code uses.

Indeed :)

Sorry for the noise, I'll add few hints to the analyzer.

[Qemu-devel] [PATCH for 2.10 21/35] arm/sysbus-fdt: fix null pointer dereference

[Philippe Mathieu-Daudé]

Use error_report() + exit() instead of error_setg(&error_fatal).

hw/arm/sysbus-fdt.c:322:9: warning: Array access (from variable 'node_path') results in a null pointer dereference
    if (node_path[1]) {
        ^~~~~~~~~~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/sysbus-fdt.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index d68e3dcdbd..ad0cc49b19 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -315,15 +315,14 @@ static int add_amd_xgbe_fdt_node(SysBusDevice *sbdev, void *opaque)
     node_path = qemu_fdt_node_path(host_fdt, dt_name, vdev->compat,
                                    &error_fatal);
     if (!node_path || !node_path[0]) {
-        error_setg(&error_fatal, "%s unable to retrieve node path for %s/%s",
+        error_report("%s unable to retrieve node path for %s/%s",
                    __func__, dt_name, vdev->compat);
-    }
-
-    if (node_path[1]) {
-        error_setg(&error_fatal, "%s more than one node matching %s/%s!",
+        exit(1);
+    } else if (node_path[1]) {
+        error_report("%s more than one node matching %s/%s!",
                    __func__, dt_name, vdev->compat);
+        exit(1);
     }
-
     g_free(dt_name);
 
     if (vbasedev->num_regions != 5) {
-- 
2.13.3

Re: [Qemu-devel] [PATCH for 2.10 21/35] arm/sysbus-fdt: fix null pointer dereference

[Peter Maydell]

On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Use error_report() + exit() instead of error_setg(&error_fatal).
>
> hw/arm/sysbus-fdt.c:322:9: warning: Array access (from variable 'node_path') results in a null pointer dereference
>     if (node_path[1]) {
>         ^~~~~~~~~~~~

I don't understand what this warning is trying to say.
We can't get to this point with a NULL node_path,
because of the previous conditional, which is using
error_setg(&error_fatal).

> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/arm/sysbus-fdt.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index d68e3dcdbd..ad0cc49b19 100644
> --- a/hw/arm/sysbus-fdt.c
> +++ b/hw/arm/sysbus-fdt.c
> @@ -315,15 +315,14 @@ static int add_amd_xgbe_fdt_node(SysBusDevice *sbdev, void *opaque)
>      node_path = qemu_fdt_node_path(host_fdt, dt_name, vdev->compat,
>                                     &error_fatal);
>      if (!node_path || !node_path[0]) {
> -        error_setg(&error_fatal, "%s unable to retrieve node path for %s/%s",
> +        error_report("%s unable to retrieve node path for %s/%s",
>                     __func__, dt_name, vdev->compat);
> -    }
> -
> -    if (node_path[1]) {
> -        error_setg(&error_fatal, "%s more than one node matching %s/%s!",
> +        exit(1);
> +    } else if (node_path[1]) {
> +        error_report("%s more than one node matching %s/%s!",
>                     __func__, dt_name, vdev->compat);
> +        exit(1);
>      }
> -
>      g_free(dt_name);

This doesn't seem like an improvement. Now the
error handling in the function is an inconsistent
mix of error_report()+exit() and error_setg(&error_fatal).

thanks
-- PMM

Re: [Qemu-devel] [PATCH for 2.10 21/35] arm/sysbus-fdt: fix null pointer dereference

[Philippe Mathieu-Daudé]

On 07/24/2017 06:09 PM, Peter Maydell wrote:
> On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
...
>> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
>> index d68e3dcdbd..ad0cc49b19 100644
>> --- a/hw/arm/sysbus-fdt.c
>> +++ b/hw/arm/sysbus-fdt.c
>> @@ -315,15 +315,14 @@ static int add_amd_xgbe_fdt_node(SysBusDevice *sbdev, void *opaque)
>>       node_path = qemu_fdt_node_path(host_fdt, dt_name, vdev->compat,
>>                                      &error_fatal);
>>       if (!node_path || !node_path[0]) {
>> -        error_setg(&error_fatal, "%s unable to retrieve node path for %s/%s",
>> +        error_report("%s unable to retrieve node path for %s/%s",
>>                      __func__, dt_name, vdev->compat);
>> -    }
>> -
>> -    if (node_path[1]) {
>> -        error_setg(&error_fatal, "%s more than one node matching %s/%s!",
>> +        exit(1);
>> +    } else if (node_path[1]) {
>> +        error_report("%s more than one node matching %s/%s!",
>>                      __func__, dt_name, vdev->compat);
>> +        exit(1);
>>       }
>> -
>>       g_free(dt_name);
> 
> This doesn't seem like an improvement. Now the
> error handling in the function is an inconsistent
> mix of error_report()+exit() and error_setg(&error_fatal).

I got this from "qapi/error.h":

156 * Please don't error_setg(&error_fatal, ...), use error_report() and 

157 * exit(), because that's more obvious. 

158 * Likewise, don't error_setg(&error_abort, ...), use assert(). 


Is this comment outdated?

Re: [Qemu-devel] [PATCH for 2.10 21/35] arm/sysbus-fdt: fix null pointer dereference

[Peter Maydell]

On 24 July 2017 at 22:20, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 07/24/2017 06:09 PM, Peter Maydell wrote:
>>
>> On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> ...
>
>>> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
>>> index d68e3dcdbd..ad0cc49b19 100644
>>> --- a/hw/arm/sysbus-fdt.c
>>> +++ b/hw/arm/sysbus-fdt.c
>>> @@ -315,15 +315,14 @@ static int add_amd_xgbe_fdt_node(SysBusDevice
>>> *sbdev, void *opaque)
>>>       node_path = qemu_fdt_node_path(host_fdt, dt_name, vdev->compat,
>>>                                      &error_fatal);
>>>       if (!node_path || !node_path[0]) {
>>> -        error_setg(&error_fatal, "%s unable to retrieve node path for
>>> %s/%s",
>>> +        error_report("%s unable to retrieve node path for %s/%s",
>>>                      __func__, dt_name, vdev->compat);
>>> -    }
>>> -
>>> -    if (node_path[1]) {
>>> -        error_setg(&error_fatal, "%s more than one node matching
>>> %s/%s!",
>>> +        exit(1);
>>> +    } else if (node_path[1]) {
>>> +        error_report("%s more than one node matching %s/%s!",
>>>                      __func__, dt_name, vdev->compat);
>>> +        exit(1);
>>>       }
>>> -
>>>       g_free(dt_name);
>>
>>
>> This doesn't seem like an improvement. Now the
>> error handling in the function is an inconsistent
>> mix of error_report()+exit() and error_setg(&error_fatal).
>
>
> I got this from "qapi/error.h":
>
> 156 * Please don't error_setg(&error_fatal, ...), use error_report() and
> 157 * exit(), because that's more obvious.
> 158 * Likewise, don't error_setg(&error_abort, ...), use assert().
>
> Is this comment outdated?

Probably not, I expect this code predates it. However
my point about inconsistency still stands.

thanks
-- PMM

Re: [Qemu-devel] [PATCH for 2.10 21/35] arm/sysbus-fdt: fix null pointer dereference

[Philippe Mathieu-Daudé]

On 07/24/2017 06:09 PM, Peter Maydell wrote:
> On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Use error_report() + exit() instead of error_setg(&error_fatal).
>>
>> hw/arm/sysbus-fdt.c:322:9: warning: Array access (from variable 'node_path') results in a null pointer dereference
>>      if (node_path[1]) {
>>          ^~~~~~~~~~~~
> 
> I don't understand what this warning is trying to say.
> We can't get to this point with a NULL node_path,
> because of the previous conditional, which is using
> error_setg(&error_fatal).

Ok I see, Clang is unaware than error_setg(&error_fatal) is a noreturn.

Patch dropped.

Thanks,

Phil.

Re: [Qemu-devel] [PATCH for 2.10 21/35] arm/sysbus-fdt: fix null pointer dereference

[Eric Blake]

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

On 07/24/2017 04:48 PM, Philippe Mathieu-Daudé wrote:
> On 07/24/2017 06:09 PM, Peter Maydell wrote:
>> On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>> Use error_report() + exit() instead of error_setg(&error_fatal).
>>>
>>> hw/arm/sysbus-fdt.c:322:9: warning: Array access (from variable
>>> 'node_path') results in a null pointer dereference
>>>      if (node_path[1]) {
>>>          ^~~~~~~~~~~~
>>
>> I don't understand what this warning is trying to say.
>> We can't get to this point with a NULL node_path,
>> because of the previous conditional, which is using
>> error_setg(&error_fatal).
> 
> Ok I see, Clang is unaware than error_setg(&error_fatal) is a noreturn.

Indeed, and that's because error_setg(&error_fatal) is not in preferred
form.

> 
> Patch dropped.

That's a shame.  Rather, we should patch this file (and others) to avoid
all the inconsistent uses of error_setg(&error_*), to comply with the
error.h documentation.

-- 
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 --]

Re: [Qemu-devel] [PATCH for 2.10 21/35] arm/sysbus-fdt: fix null pointer dereference

[Eric Blake]

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

On 07/24/2017 04:52 PM, Eric Blake wrote:
> On 07/24/2017 04:48 PM, Philippe Mathieu-Daudé wrote:
>> On 07/24/2017 06:09 PM, Peter Maydell wrote:
>>> On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>> Use error_report() + exit() instead of error_setg(&error_fatal).
>>>>
>>>> hw/arm/sysbus-fdt.c:322:9: warning: Array access (from variable
>>>> 'node_path') results in a null pointer dereference
>>>>      if (node_path[1]) {
>>>>          ^~~~~~~~~~~~
>>>
>>> I don't understand what this warning is trying to say.
>>> We can't get to this point with a NULL node_path,
>>> because of the previous conditional, which is using
>>> error_setg(&error_fatal).
>>
>> Ok I see, Clang is unaware than error_setg(&error_fatal) is a noreturn.
> 
> Indeed, and that's because error_setg(&error_fatal) is not in preferred
> form.
> 
>>
>> Patch dropped.
> 
> That's a shame.  Rather, we should patch this file (and others) to avoid
> all the inconsistent uses of error_setg(&error_*), to comply with the
> error.h documentation.

In other words, switching to the preferred spelling in the following files:
device_tree.c
hw/arm/sysbus-fdt.c
hw/block/fdc.c
hw/ppc/spapr_drc.c

is desirable, and has the added benefit of also silencing a Coverity
false positive.  But it should be done in terms of switching to the
preferred spelling, as it touches more instances than just the one that
shuts up Coverity.

-- 
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 --]

Re: [Qemu-devel] [PATCH for 2.10 21/35] arm/sysbus-fdt: fix null pointer dereference

[Philippe Mathieu-Daudé]

Hi Eric,

On 07/24/2017 06:57 PM, Eric Blake wrote:
> On 07/24/2017 04:52 PM, Eric Blake wrote:
>> On 07/24/2017 04:48 PM, Philippe Mathieu-Daudé wrote:
>>> On 07/24/2017 06:09 PM, Peter Maydell wrote:
>>>> On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>>> Use error_report() + exit() instead of error_setg(&error_fatal).
>>>>>
>>>>> hw/arm/sysbus-fdt.c:322:9: warning: Array access (from variable
>>>>> 'node_path') results in a null pointer dereference
>>>>>      if (node_path[1]) {
>>>>>          ^~~~~~~~~~~~
>>>>
>>>> I don't understand what this warning is trying to say.
>>>> We can't get to this point with a NULL node_path,
>>>> because of the previous conditional, which is using
>>>> error_setg(&error_fatal).
>>>
>>> Ok I see, Clang is unaware than error_setg(&error_fatal) is a noreturn.
>>
>> Indeed, and that's because error_setg(&error_fatal) is not in preferred
>> form.
>>
>>>
>>> Patch dropped.
>>
>> That's a shame.  Rather, we should patch this file (and others) to avoid
>> all the inconsistent uses of error_setg(&error_*), to comply with the
>> error.h documentation.

I started to port/clean this up.
To avoid future inconsistencies, do you think we should/can enforce this
check in checkpatch (which is stricter than human review)?
Is the "Qemu error function tests" section a good place to put this check?

> 
> In other words, switching to the preferred spelling in the following files:
> device_tree.c
> hw/arm/sysbus-fdt.c
> hw/block/fdc.c
> hw/ppc/spapr_drc.c
> 
> is desirable, and has the added benefit of also silencing a Coverity
> false positive.  But it should be done in terms of switching to the
> preferred spelling, as it touches more instances than just the one that
> shuts up Coverity.
> 

Re: [Qemu-devel] [PATCH for 2.10 21/35] arm/sysbus-fdt: fix null pointer dereference

[Eric Blake]

[adding Markus, as the error maintainer]

On 05/29/2018 09:33 AM, Philippe Mathieu-Daudé wrote:

>>>> Ok I see, Clang is unaware than error_setg(&error_fatal) is a noreturn.
>>>
>>> Indeed, and that's because error_setg(&error_fatal) is not in preferred
>>> form.
>>>
>>>>
>>>> Patch dropped.
>>>
>>> That's a shame.  Rather, we should patch this file (and others) to avoid
>>> all the inconsistent uses of error_setg(&error_*), to comply with the
>>> error.h documentation.
> 
> I started to port/clean this up.
> To avoid future inconsistencies, do you think we should/can enforce this
> check in checkpatch (which is stricter than human review)?

Sure, automating good code style is worthwhile.

> Is the "Qemu error function tests" section a good place to put this check?

Probably works.  I'm not familiar enough with checkpatch as a whole to 
have any different suggestions off the top of my head (basically, any 
time I've touched that file, it's been grepping for something that is 
similar to what I want to be tweaking, rather than reading the whole thing).

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

[Qemu-devel] [PATCH for 2.10 22/35] arm/vexpress: fix potential memory leak

[Philippe Mathieu-Daudé]

Reorder to only allocate if required.

hw/arm/vexpress.c:667:13: warning: Potential leak of memory pointed to by 'flashalias'

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

diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index 528c65ddb6..76c4d84482 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -553,8 +553,6 @@ static void vexpress_common_init(MachineState *machine)
     MemoryRegion *sysmem = get_system_memory();
     MemoryRegion *vram = g_new(MemoryRegion, 1);
     MemoryRegion *sram = g_new(MemoryRegion, 1);
-    MemoryRegion *flashalias = g_new(MemoryRegion, 1);
-    MemoryRegion *flash0mem;
     const hwaddr *map = daughterboard->motherboard_map;
     int i;
 
@@ -657,6 +655,9 @@ static void vexpress_common_init(MachineState *machine)
     }
 
     if (map[VE_NORFLASHALIAS] != -1) {
+        MemoryRegion *flashalias = g_new(MemoryRegion, 1);
+        MemoryRegion *flash0mem;
+
         /* Map flash 0 as an alias into low memory */
         flash0mem = sysbus_mmio_get_region(SYS_BUS_DEVICE(pflash0), 0);
         memory_region_init_alias(flashalias, NULL, "vexpress.flashalias",
-- 
2.13.3

Re: [Qemu-devel] [PATCH for 2.10 22/35] arm/vexpress: fix potential memory leak

[Peter Maydell]

On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Reorder to only allocate if required.
>
> hw/arm/vexpress.c:667:13: warning: Potential leak of memory pointed to by 'flashalias'
>
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/arm/vexpress.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
> index 528c65ddb6..76c4d84482 100644
> --- a/hw/arm/vexpress.c
> +++ b/hw/arm/vexpress.c
> @@ -553,8 +553,6 @@ static void vexpress_common_init(MachineState *machine)
>      MemoryRegion *sysmem = get_system_memory();
>      MemoryRegion *vram = g_new(MemoryRegion, 1);
>      MemoryRegion *sram = g_new(MemoryRegion, 1);
> -    MemoryRegion *flashalias = g_new(MemoryRegion, 1);
> -    MemoryRegion *flash0mem;
>      const hwaddr *map = daughterboard->motherboard_map;
>      int i;
>
> @@ -657,6 +655,9 @@ static void vexpress_common_init(MachineState *machine)
>      }
>
>      if (map[VE_NORFLASHALIAS] != -1) {
> +        MemoryRegion *flashalias = g_new(MemoryRegion, 1);
> +        MemoryRegion *flash0mem;
> +
>          /* Map flash 0 as an alias into low memory */
>          flash0mem = sysbus_mmio_get_region(SYS_BUS_DEVICE(pflash0), 0);
>          memory_region_init_alias(flashalias, NULL, "vexpress.flashalias",
> --
> 2.13.3

flashalias is no different from any of the other MemoryRegion*
variables we allocate here with g_new() -- they all
leak, and it doesn't matter because the function
runs once and we want the MemoryRegions to survive
until the end of the simulation.

These days we could in theory clean this up by
switching from doing g_new() to having a set of
MemoryRegion fields in the VexpressMachineState
struct (compare hw/arm/mps2.c).

thanks
-- PMM

Re: [Qemu-devel] [PATCH for 2.10 22/35] arm/vexpress: fix potential memory leak

[Philippe Mathieu-Daudé]

On 07/24/2017 06:11 PM, Peter Maydell wrote:
> On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Reorder to only allocate if required.
>>
>> hw/arm/vexpress.c:667:13: warning: Potential leak of memory pointed to by 'flashalias'
>>
>> Reported-by: Clang Static Analyzer
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   hw/arm/vexpress.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
>> index 528c65ddb6..76c4d84482 100644
>> --- a/hw/arm/vexpress.c
>> +++ b/hw/arm/vexpress.c
>> @@ -553,8 +553,6 @@ static void vexpress_common_init(MachineState *machine)
>>       MemoryRegion *sysmem = get_system_memory();
>>       MemoryRegion *vram = g_new(MemoryRegion, 1);
>>       MemoryRegion *sram = g_new(MemoryRegion, 1);
>> -    MemoryRegion *flashalias = g_new(MemoryRegion, 1);
>> -    MemoryRegion *flash0mem;
>>       const hwaddr *map = daughterboard->motherboard_map;
>>       int i;
>>
>> @@ -657,6 +655,9 @@ static void vexpress_common_init(MachineState *machine)
>>       }
>>
>>       if (map[VE_NORFLASHALIAS] != -1) {
>> +        MemoryRegion *flashalias = g_new(MemoryRegion, 1);
>> +        MemoryRegion *flash0mem;
>> +
>>           /* Map flash 0 as an alias into low memory */
>>           flash0mem = sysbus_mmio_get_region(SYS_BUS_DEVICE(pflash0), 0);
>>           memory_region_init_alias(flashalias, NULL, "vexpress.flashalias",
>> --
>> 2.13.3
> 
> flashalias is no different from any of the other MemoryRegion*
> variables we allocate here with g_new() -- they all
> leak, and it doesn't matter because the function
> runs once and we want the MemoryRegions to survive
> until the end of the simulation.
> 
> These days we could in theory clean this up by
> switching from doing g_new() to having a set of
> MemoryRegion fields in the VexpressMachineState
> struct (compare hw/arm/mps2.c).

I agree with the analyzer here, why allocate something you'll never use?
However this is not the case here since in your commit 6ec1588e097 both 
legacy/a9 and aseries/a15 use NORFLASHALIAS to remap their flash at 0.

Anyway patch dropped.

Thanks,

Phil.

[Qemu-devel] [PATCH for 2.10 23/35] 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>
---
 target/m68k/translate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index ada2a91b64..1a2f421aab 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -1321,7 +1321,8 @@ 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 ||
+            op == CC_OP_LOGIC) {
             tcond = TCG_COND_NE;
             c->v1 = QREG_CC_X;
             goto done;
-- 
2.13.3

Re: [Qemu-devel] [PATCH for 2.10 23/35] m68k/translate: fix incorrect copy/paste

[Laurent Vivier]

Le 24/07/2017 à 20:27, Philippe Mathieu-Daudé a écrit :
> 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>
> ---
>  target/m68k/translate.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
> index ada2a91b64..1a2f421aab 100644
> --- a/target/m68k/translate.c
> +++ b/target/m68k/translate.c
> @@ -1321,7 +1321,8 @@ 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 ||
> +            op == CC_OP_LOGIC) {

According to commit (db3d7945) introducing the incorrect copy/paste, I
don't think we need the CC_OP_LOGIC here. Logic operations never
generates X flags (whereas they can generate Z and and N).

Thanks,
Laurent

Re: [Qemu-devel] [PATCH for 2.10 23/35] m68k/translate: fix incorrect copy/paste

[Richard Henderson]

On 07/24/2017 11:54 AM, Laurent Vivier wrote:
> Le 24/07/2017 à 20:27, Philippe Mathieu-Daudé a écrit :
>> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
>> index ada2a91b64..1a2f421aab 100644
>> --- a/target/m68k/translate.c
>> +++ b/target/m68k/translate.c
>> @@ -1321,7 +1321,8 @@ 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 ||
>> +            op == CC_OP_LOGIC) {
> 
> According to commit (db3d7945) introducing the incorrect copy/paste, I
> don't think we need the CC_OP_LOGIC here. Logic operations never
> generates X flags (whereas they can generate Z and and N).

Indeed, look at the comment below.  LOGIC is supposed to fall through to the 
code that produces TCG_COND_NEVER.


r~

Re: [Qemu-devel] [PATCH for 2.10 23/35] m68k/translate: fix incorrect copy/paste

[Philippe Mathieu-Daudé]

On 07/24/2017 04:01 PM, Richard Henderson wrote:
> On 07/24/2017 11:54 AM, Laurent Vivier wrote:
>> Le 24/07/2017 à 20:27, Philippe Mathieu-Daudé a écrit :
>>> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
>>> index ada2a91b64..1a2f421aab 100644
>>> --- a/target/m68k/translate.c
>>> +++ b/target/m68k/translate.c
>>> @@ -1321,7 +1321,8 @@ 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 ||
>>> +            op == CC_OP_LOGIC) {
>>
>> According to commit (db3d7945) introducing the incorrect copy/paste, I
>> don't think we need the CC_OP_LOGIC here. Logic operations never
>> generates X flags (whereas they can generate Z and and N).
> 
> Indeed, look at the comment below.  LOGIC is supposed to fall through to 
> the code that produces TCG_COND_NEVER.

Indeed I also miss-copypasted :)

So the correct fix would be:?

          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) {

Re: [Qemu-devel] [PATCH for 2.10 23/35] m68k/translate: fix incorrect copy/paste

[Laurent Vivier]

Le 24/07/2017 à 21:19, Philippe Mathieu-Daudé a écrit :
> On 07/24/2017 04:01 PM, Richard Henderson wrote:
>> On 07/24/2017 11:54 AM, Laurent Vivier wrote:
>>> Le 24/07/2017 à 20:27, Philippe Mathieu-Daudé a écrit :
>>>> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
>>>> index ada2a91b64..1a2f421aab 100644
>>>> --- a/target/m68k/translate.c
>>>> +++ b/target/m68k/translate.c
>>>> @@ -1321,7 +1321,8 @@ 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 ||
>>>> +            op == CC_OP_LOGIC) {
>>>
>>> According to commit (db3d7945) introducing the incorrect copy/paste, I
>>> don't think we need the CC_OP_LOGIC here. Logic operations never
>>> generates X flags (whereas they can generate Z and and N).
>>
>> Indeed, look at the comment below.  LOGIC is supposed to fall through
>> to the code that produces TCG_COND_NEVER.
> 
> Indeed I also miss-copypasted :)
> 
> So the correct fix would be:?
> 
>          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) {

yes

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

Laurent

[Qemu-devel] [PATCH for 2.10 24/35] 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>
---
 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

Re: [Qemu-devel] [PATCH for 2.10 24/35] linux-user/sh4: fix incorrect memory write

[Laurent Vivier]

Le 24/07/2017 à 20:27, Philippe Mathieu-Daudé a écrit :
> 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>
> ---
>  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);
> 

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

[Qemu-devel] [PATCH for 2.10 25/35] linux-user: extract is_error() out of syscall.c

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 linux-user/qemu.h    | 5 +++++
 linux-user/syscall.c | 5 -----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 4edd7d0c08..97b7cfdb63 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -614,6 +614,11 @@ static inline void *lock_user_string(abi_ulong guest_addr)
 
 #include <pthread.h>
 
+static inline int is_error(abi_long ret)
+{
+    return (abi_ulong)ret >= (abi_ulong)(-4096);
+}
+
 /* Include target-specific struct and function definitions;
  * they may need access to the target-independent structures
  * above, so include them last.
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 003943b736..fcd20fa276 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -837,11 +837,6 @@ static inline abi_long get_errno(abi_long ret)
         return ret;
 }
 
-static inline int is_error(abi_long ret)
-{
-    return (abi_ulong)ret >= (abi_ulong)(-4096);
-}
-
 const char *target_strerror(int err)
 {
     if (err == TARGET_ERESTARTSYS) {
-- 
2.13.3

Re: [Qemu-devel] [PATCH for 2.10 25/35] linux-user: extract is_error() out of syscall.c

[Laurent Vivier]

Le 24/07/2017 à 20:27, Philippe Mathieu-Daudé a écrit :
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  linux-user/qemu.h    | 5 +++++
>  linux-user/syscall.c | 5 -----
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index 4edd7d0c08..97b7cfdb63 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -614,6 +614,11 @@ static inline void *lock_user_string(abi_ulong guest_addr)
>  
>  #include <pthread.h>
>  
> +static inline int is_error(abi_long ret)
> +{
> +    return (abi_ulong)ret >= (abi_ulong)(-4096);
> +}
> +
>  /* Include target-specific struct and function definitions;
>   * they may need access to the target-independent structures
>   * above, so include them last.
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 003943b736..fcd20fa276 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -837,11 +837,6 @@ static inline abi_long get_errno(abi_long ret)
>          return ret;
>  }
>  
> -static inline int is_error(abi_long ret)
> -{
> -    return (abi_ulong)ret >= (abi_ulong)(-4096);
> -}
> -
>  const char *target_strerror(int err)
>  {
>      if (err == TARGET_ERESTARTSYS) {
> 
I think you should fold this one into the following one.

Laurent

[Qemu-devel] [PATCH for 2.10 26/35] linux-user: use is_error() to avoid warnings and make the code clearer

[Philippe Mathieu-Daudé]

linux-user/flatload.c:740:9: warning: Loss of sign in implicit conversion
    if (res > (unsigned long)-4096)
        ^~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 linux-user/flatload.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/linux-user/flatload.c b/linux-user/flatload.c
index a35a560904..10c529910f 100644
--- a/linux-user/flatload.c
+++ b/linux-user/flatload.c
@@ -224,8 +224,9 @@ static int decompress_exec(
 		ret = bprm->file->f_op->read(bprm->file, buf, LBUFSIZE, &fpos);
 		if (ret <= 0)
 			break;
-		if (ret >= (unsigned long) -4096)
+                if (is_error(ret)) {
 			break;
+                }
 		len -= ret;
 
 		strm.next_in = buf;
@@ -283,8 +284,7 @@ calc_reloc(abi_ulong r, struct lib_info *p, int curid, int internalp)
                     "in same module (%d != %d)\n",
                     (unsigned) r, curid, id);
             goto failed;
-        } else if ( ! p[id].loaded &&
-                    load_flat_shared_library(id, p) > (unsigned long) -4096) {
+        } else if (!p[id].loaded && is_error(load_flat_shared_library(id, p))) {
             fprintf(stderr, "BINFMT_FLAT: failed to load library %d\n", id);
             goto failed;
         }
@@ -523,9 +523,10 @@ static int load_flat_file(struct linux_binprm * bprm,
                 fpos = 0;
                 result = bprm->file->f_op->read(bprm->file,
                                 (char *) textpos, text_len, &fpos);
-                if (result < (unsigned long) -4096)
+                if (!is_error(result)) {
                         result = decompress_exec(bprm, text_len, (char *) datapos,
                                          data_len + (relocs * sizeof(unsigned long)), 0);
+                }
         }
         else
 #endif
@@ -693,8 +694,9 @@ static int load_flat_shared_library(int id, struct lib_info *libs)
 
 	res = prepare_binprm(&bprm);
 
-	if (res <= (unsigned long)-4096)
+        if (!is_error(res)) {
 		res = load_flat_file(&bprm, libs, id, NULL);
+        }
 	if (bprm.file) {
 		allow_write_access(bprm.file);
 		fput(bprm.file);
@@ -737,8 +739,9 @@ int load_flt_binary(struct linux_binprm *bprm, struct image_info *info)
 
 
     res = load_flat_file(bprm, libinfo, 0, &stack_len);
-    if (res > (unsigned long)-4096)
+    if (is_error(res)) {
             return res;
+    }
 
     /* Update data segment pointers for all libraries */
     for (i=0; i<MAX_SHARED_LIBS; i++) {
-- 
2.13.3

Re: [Qemu-devel] [PATCH for 2.10 26/35] linux-user: use is_error() to avoid warnings and make the code clearer

[Laurent Vivier]

Le 24/07/2017 à 20:27, Philippe Mathieu-Daudé a écrit :
> linux-user/flatload.c:740:9: warning: Loss of sign in implicit conversion
>     if (res > (unsigned long)-4096)
>         ^~~
> 
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

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

> ---
>  linux-user/flatload.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/linux-user/flatload.c b/linux-user/flatload.c
> index a35a560904..10c529910f 100644
> --- a/linux-user/flatload.c
> +++ b/linux-user/flatload.c
> @@ -224,8 +224,9 @@ static int decompress_exec(
>  		ret = bprm->file->f_op->read(bprm->file, buf, LBUFSIZE, &fpos);
>  		if (ret <= 0)
>  			break;
> -		if (ret >= (unsigned long) -4096)
> +                if (is_error(ret)) {
>  			break;
> +                }
>  		len -= ret;
>  
>  		strm.next_in = buf;
> @@ -283,8 +284,7 @@ calc_reloc(abi_ulong r, struct lib_info *p, int curid, int internalp)
>                      "in same module (%d != %d)\n",
>                      (unsigned) r, curid, id);
>              goto failed;
> -        } else if ( ! p[id].loaded &&
> -                    load_flat_shared_library(id, p) > (unsigned long) -4096) {
> +        } else if (!p[id].loaded && is_error(load_flat_shared_library(id, p))) {
>              fprintf(stderr, "BINFMT_FLAT: failed to load library %d\n", id);
>              goto failed;
>          }
> @@ -523,9 +523,10 @@ static int load_flat_file(struct linux_binprm * bprm,
>                  fpos = 0;
>                  result = bprm->file->f_op->read(bprm->file,
>                                  (char *) textpos, text_len, &fpos);
> -                if (result < (unsigned long) -4096)
> +                if (!is_error(result)) {
>                          result = decompress_exec(bprm, text_len, (char *) datapos,
>                                           data_len + (relocs * sizeof(unsigned long)), 0);
> +                }
>          }
>          else
>  #endif
> @@ -693,8 +694,9 @@ static int load_flat_shared_library(int id, struct lib_info *libs)
>  
>  	res = prepare_binprm(&bprm);
>  
> -	if (res <= (unsigned long)-4096)
> +        if (!is_error(res)) {
>  		res = load_flat_file(&bprm, libs, id, NULL);
> +        }
>  	if (bprm.file) {
>  		allow_write_access(bprm.file);
>  		fput(bprm.file);
> @@ -737,8 +739,9 @@ int load_flt_binary(struct linux_binprm *bprm, struct image_info *info)
>  
>  
>      res = load_flat_file(bprm, libinfo, 0, &stack_len);
> -    if (res > (unsigned long)-4096)
> +    if (is_error(res)) {
>              return res;
> +    }
>  
>      /* Update data segment pointers for all libraries */
>      for (i=0; i<MAX_SHARED_LIBS; i++) {
> 

Re: [Qemu-devel] [PATCH for 2.10 26/35] linux-user: use is_error() to avoid warnings and make the code clearer

[Philippe Mathieu-Daudé]

Hi Laurent,

On 07/24/2017 04:16 PM, Laurent Vivier wrote:
> Le 24/07/2017 à 20:27, Philippe Mathieu-Daudé a écrit :
>> linux-user/flatload.c:740:9: warning: Loss of sign in implicit conversion
>>     if (res > (unsigned long)-4096)
>>         ^~~
>>
>> Reported-by: Clang Static Analyzer
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> Reviewed-by: Laurent Vivier <laurent@vivier.eu>
> 
>> ---
>>  linux-user/flatload.c | 15 +++++++++------
>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/linux-user/flatload.c b/linux-user/flatload.c
>> index a35a560904..10c529910f 100644
>> --- a/linux-user/flatload.c
>> +++ b/linux-user/flatload.c
>> @@ -224,8 +224,9 @@ static int decompress_exec(
>>  		ret = bprm->file->f_op->read(bprm->file, buf, LBUFSIZE, &fpos);
>>  		if (ret <= 0)
>>  			break;
>> -		if (ret >= (unsigned long) -4096)
>> +                if (is_error(ret)) {
>>  			break;
>> +                }
>>  		len -= ret;
>>  
>>  		strm.next_in = buf;
>> @@ -283,8 +284,7 @@ calc_reloc(abi_ulong r, struct lib_info *p, int curid, int internalp)
>>                      "in same module (%d != %d)\n",
>>                      (unsigned) r, curid, id);
>>              goto failed;
>> -        } else if ( ! p[id].loaded &&
>> -                    load_flat_shared_library(id, p) > (unsigned long) -4096) {
>> +        } else if (!p[id].loaded && is_error(load_flat_shared_library(id, p))) {
>>              fprintf(stderr, "BINFMT_FLAT: failed to load library %d\n", id);
>>              goto failed;
>>          }
>> @@ -523,9 +523,10 @@ static int load_flat_file(struct linux_binprm * bprm,
>>                  fpos = 0;
>>                  result = bprm->file->f_op->read(bprm->file,
>>                                  (char *) textpos, text_len, &fpos);
>> -                if (result < (unsigned long) -4096)
>> +                if (!is_error(result)) {
>>                          result = decompress_exec(bprm, text_len, (char *) datapos,
>>                                           data_len + (relocs * sizeof(unsigned long)), 0);
>> +                }
>>          }
>>          else
>>  #endif
>> @@ -693,8 +694,9 @@ static int load_flat_shared_library(int id, struct lib_info *libs)
>>  
>>  	res = prepare_binprm(&bprm);
>>  
>> -	if (res <= (unsigned long)-4096)
>> +        if (!is_error(res)) {
>>  		res = load_flat_file(&bprm, libs, id, NULL);
>> +        }
>>  	if (bprm.file) {
>>  		allow_write_access(bprm.file);
>>  		fput(bprm.file);
>> @@ -737,8 +739,9 @@ int load_flt_binary(struct linux_binprm *bprm, struct image_info *info)
>>  
>>  
>>      res = load_flat_file(bprm, libinfo, 0, &stack_len);
>> -    if (res > (unsigned long)-4096)
>> +    if (is_error(res)) {
>>              return res;
>> +    }
>>  
>>      /* Update data segment pointers for all libraries */
>>      for (i=0; i<MAX_SHARED_LIBS; i++) {
>>

Can you take this via your linux-user tree?

Thanks,

Phil.

Re: [Qemu-devel] [PATCH for 2.10 26/35] linux-user: use is_error() to avoid warnings and make the code clearer

[Laurent Vivier]

Le 29/05/2018 à 16:25, Philippe Mathieu-Daudé a écrit :
> Hi Laurent,

Hi Philippe,

> On 07/24/2017 04:16 PM, Laurent Vivier wrote:
>> Le 24/07/2017 à 20:27, Philippe Mathieu-Daudé a écrit :
>>> linux-user/flatload.c:740:9: warning: Loss of sign in implicit conversion
>>>     if (res > (unsigned long)-4096)
>>>         ^~~
>>>
>>> Reported-by: Clang Static Analyzer
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>
>> Reviewed-by: Laurent Vivier <laurent@vivier.eu>
>>
>>> ---
>>>  linux-user/flatload.c | 15 +++++++++------
>>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/linux-user/flatload.c b/linux-user/flatload.c
>>> index a35a560904..10c529910f 100644
>>> --- a/linux-user/flatload.c
>>> +++ b/linux-user/flatload.c
>>> @@ -224,8 +224,9 @@ static int decompress_exec(
>>>  		ret = bprm->file->f_op->read(bprm->file, buf, LBUFSIZE, &fpos);
>>>  		if (ret <= 0)
>>>  			break;
>>> -		if (ret >= (unsigned long) -4096)
>>> +                if (is_error(ret)) {
>>>  			break;
>>> +                }
>>>  		len -= ret;
>>>  
>>>  		strm.next_in = buf;
>>> @@ -283,8 +284,7 @@ calc_reloc(abi_ulong r, struct lib_info *p, int curid, int internalp)
>>>                      "in same module (%d != %d)\n",
>>>                      (unsigned) r, curid, id);
>>>              goto failed;
>>> -        } else if ( ! p[id].loaded &&
>>> -                    load_flat_shared_library(id, p) > (unsigned long) -4096) {
>>> +        } else if (!p[id].loaded && is_error(load_flat_shared_library(id, p))) {
>>>              fprintf(stderr, "BINFMT_FLAT: failed to load library %d\n", id);
>>>              goto failed;
>>>          }
>>> @@ -523,9 +523,10 @@ static int load_flat_file(struct linux_binprm * bprm,
>>>                  fpos = 0;
>>>                  result = bprm->file->f_op->read(bprm->file,
>>>                                  (char *) textpos, text_len, &fpos);
>>> -                if (result < (unsigned long) -4096)
>>> +                if (!is_error(result)) {
>>>                          result = decompress_exec(bprm, text_len, (char *) datapos,
>>>                                           data_len + (relocs * sizeof(unsigned long)), 0);
>>> +                }
>>>          }
>>>          else
>>>  #endif
>>> @@ -693,8 +694,9 @@ static int load_flat_shared_library(int id, struct lib_info *libs)
>>>  
>>>  	res = prepare_binprm(&bprm);
>>>  
>>> -	if (res <= (unsigned long)-4096)
>>> +        if (!is_error(res)) {
>>>  		res = load_flat_file(&bprm, libs, id, NULL);
>>> +        }
>>>  	if (bprm.file) {
>>>  		allow_write_access(bprm.file);
>>>  		fput(bprm.file);
>>> @@ -737,8 +739,9 @@ int load_flt_binary(struct linux_binprm *bprm, struct image_info *info)
>>>  
>>>  
>>>      res = load_flat_file(bprm, libinfo, 0, &stack_len);
>>> -    if (res > (unsigned long)-4096)
>>> +    if (is_error(res)) {
>>>              return res;
>>> +    }
>>>  
>>>      /* Update data segment pointers for all libraries */
>>>      for (i=0; i<MAX_SHARED_LIBS; i++) {
>>>
> 
> Can you take this via your linux-user tree?
> 

Applied, thanks.

Laurent

Re: [Qemu-devel] [PATCH for 2.10 26/35] linux-user: use is_error() to avoid warnings and make the code clearer

[Laurent Vivier]

Le 29/05/2018 à 17:19, Laurent Vivier a écrit :
> Le 29/05/2018 à 16:25, Philippe Mathieu-Daudé a écrit :
>> Hi Laurent,
> 
> Hi Philippe,
> 
>> On 07/24/2017 04:16 PM, Laurent Vivier wrote:
>>> Le 24/07/2017 à 20:27, Philippe Mathieu-Daudé a écrit :
>>>> linux-user/flatload.c:740:9: warning: Loss of sign in implicit conversion
>>>>     if (res > (unsigned long)-4096)
>>>>         ^~~
>>>>
>>>> Reported-by: Clang Static Analyzer
>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>
>>> Reviewed-by: Laurent Vivier <laurent@vivier.eu>
>>>
>>>> ---
>>>>  linux-user/flatload.c | 15 +++++++++------
>>>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/linux-user/flatload.c b/linux-user/flatload.c
>>>> index a35a560904..10c529910f 100644
>>>> --- a/linux-user/flatload.c
>>>> +++ b/linux-user/flatload.c
>>>> @@ -224,8 +224,9 @@ static int decompress_exec(
>>>>  		ret = bprm->file->f_op->read(bprm->file, buf, LBUFSIZE, &fpos);
>>>>  		if (ret <= 0)
>>>>  			break;
>>>> -		if (ret >= (unsigned long) -4096)
>>>> +                if (is_error(ret)) {
>>>>  			break;
>>>> +                }
>>>>  		len -= ret;
>>>>  
>>>>  		strm.next_in = buf;
>>>> @@ -283,8 +284,7 @@ calc_reloc(abi_ulong r, struct lib_info *p, int curid, int internalp)
>>>>                      "in same module (%d != %d)\n",
>>>>                      (unsigned) r, curid, id);
>>>>              goto failed;
>>>> -        } else if ( ! p[id].loaded &&
>>>> -                    load_flat_shared_library(id, p) > (unsigned long) -4096) {
>>>> +        } else if (!p[id].loaded && is_error(load_flat_shared_library(id, p))) {
>>>>              fprintf(stderr, "BINFMT_FLAT: failed to load library %d\n", id);
>>>>              goto failed;
>>>>          }
>>>> @@ -523,9 +523,10 @@ static int load_flat_file(struct linux_binprm * bprm,
>>>>                  fpos = 0;
>>>>                  result = bprm->file->f_op->read(bprm->file,
>>>>                                  (char *) textpos, text_len, &fpos);
>>>> -                if (result < (unsigned long) -4096)
>>>> +                if (!is_error(result)) {
>>>>                          result = decompress_exec(bprm, text_len, (char *) datapos,
>>>>                                           data_len + (relocs * sizeof(unsigned long)), 0);
>>>> +                }
>>>>          }
>>>>          else
>>>>  #endif
>>>> @@ -693,8 +694,9 @@ static int load_flat_shared_library(int id, struct lib_info *libs)
>>>>  
>>>>  	res = prepare_binprm(&bprm);
>>>>  
>>>> -	if (res <= (unsigned long)-4096)
>>>> +        if (!is_error(res)) {
>>>>  		res = load_flat_file(&bprm, libs, id, NULL);
>>>> +        }
>>>>  	if (bprm.file) {
>>>>  		allow_write_access(bprm.file);
>>>>  		fput(bprm.file);
>>>> @@ -737,8 +739,9 @@ int load_flt_binary(struct linux_binprm *bprm, struct image_info *info)
>>>>  
>>>>  
>>>>      res = load_flat_file(bprm, libinfo, 0, &stack_len);
>>>> -    if (res > (unsigned long)-4096)
>>>> +    if (is_error(res)) {
>>>>              return res;
>>>> +    }
>>>>  
>>>>      /* Update data segment pointers for all libraries */
>>>>      for (i=0; i<MAX_SHARED_LIBS; i++) {
>>>>
>>
>> Can you take this via your linux-user tree?
>>
> 
> Applied, thanks.

Unapplied, it needs a rebase:

qemu/linux-user/flatload.c: In function 'load_flt_binary':
qemu/linux-user/flatload.c:742:9: error: implicit declaration of
function 'is_error'; did you mean 'g_error'?
[-Werror=implicit-function-declaration]
     if (is_error(res)) {
         ^~~~~~~~
         g_error
qemu/linux-user/flatload.c:742:9: error: nested extern declaration of
'is_error' [-Werror=nested-externs]

Thanks,
Laurent

Re: [Qemu-devel] [PATCH for 2.10 26/35] linux-user: use is_error() to avoid warnings and make the code clearer

[Philippe Mathieu-Daudé]

On 06/03/2018 08:33 PM, Laurent Vivier wrote:
> Le 29/05/2018 à 17:19, Laurent Vivier a écrit :
>> Le 29/05/2018 à 16:25, Philippe Mathieu-Daudé a écrit :
>>> Hi Laurent,
>>
>> Hi Philippe,
>>
>>> On 07/24/2017 04:16 PM, Laurent Vivier wrote:
>>>> Le 24/07/2017 à 20:27, Philippe Mathieu-Daudé a écrit :
>>>>> linux-user/flatload.c:740:9: warning: Loss of sign in implicit conversion
>>>>>     if (res > (unsigned long)-4096)
>>>>>         ^~~
>>>>>
>>>>> Reported-by: Clang Static Analyzer
>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>
>>>> Reviewed-by: Laurent Vivier <laurent@vivier.eu>
>>>>
>>>>> ---
>>>>>  linux-user/flatload.c | 15 +++++++++------
>>>>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/linux-user/flatload.c b/linux-user/flatload.c
>>>>> index a35a560904..10c529910f 100644
>>>>> --- a/linux-user/flatload.c
>>>>> +++ b/linux-user/flatload.c
>>>>> @@ -224,8 +224,9 @@ static int decompress_exec(
>>>>>  		ret = bprm->file->f_op->read(bprm->file, buf, LBUFSIZE, &fpos);
>>>>>  		if (ret <= 0)
>>>>>  			break;
>>>>> -		if (ret >= (unsigned long) -4096)
>>>>> +                if (is_error(ret)) {
>>>>>  			break;
>>>>> +                }
>>>>>  		len -= ret;
>>>>>  
>>>>>  		strm.next_in = buf;
>>>>> @@ -283,8 +284,7 @@ calc_reloc(abi_ulong r, struct lib_info *p, int curid, int internalp)
>>>>>                      "in same module (%d != %d)\n",
>>>>>                      (unsigned) r, curid, id);
>>>>>              goto failed;
>>>>> -        } else if ( ! p[id].loaded &&
>>>>> -                    load_flat_shared_library(id, p) > (unsigned long) -4096) {
>>>>> +        } else if (!p[id].loaded && is_error(load_flat_shared_library(id, p))) {
>>>>>              fprintf(stderr, "BINFMT_FLAT: failed to load library %d\n", id);
>>>>>              goto failed;
>>>>>          }
>>>>> @@ -523,9 +523,10 @@ static int load_flat_file(struct linux_binprm * bprm,
>>>>>                  fpos = 0;
>>>>>                  result = bprm->file->f_op->read(bprm->file,
>>>>>                                  (char *) textpos, text_len, &fpos);
>>>>> -                if (result < (unsigned long) -4096)
>>>>> +                if (!is_error(result)) {
>>>>>                          result = decompress_exec(bprm, text_len, (char *) datapos,
>>>>>                                           data_len + (relocs * sizeof(unsigned long)), 0);
>>>>> +                }
>>>>>          }
>>>>>          else
>>>>>  #endif
>>>>> @@ -693,8 +694,9 @@ static int load_flat_shared_library(int id, struct lib_info *libs)
>>>>>  
>>>>>  	res = prepare_binprm(&bprm);
>>>>>  
>>>>> -	if (res <= (unsigned long)-4096)
>>>>> +        if (!is_error(res)) {
>>>>>  		res = load_flat_file(&bprm, libs, id, NULL);
>>>>> +        }
>>>>>  	if (bprm.file) {
>>>>>  		allow_write_access(bprm.file);
>>>>>  		fput(bprm.file);
>>>>> @@ -737,8 +739,9 @@ int load_flt_binary(struct linux_binprm *bprm, struct image_info *info)
>>>>>  
>>>>>  
>>>>>      res = load_flat_file(bprm, libinfo, 0, &stack_len);
>>>>> -    if (res > (unsigned long)-4096)
>>>>> +    if (is_error(res)) {
>>>>>              return res;
>>>>> +    }
>>>>>  
>>>>>      /* Update data segment pointers for all libraries */
>>>>>      for (i=0; i<MAX_SHARED_LIBS; i++) {
>>>>>
>>>
>>> Can you take this via your linux-user tree?
>>>
>>
>> Applied, thanks.
> 
> Unapplied, it needs a rebase:

No rebase required, it just need the previous patch applied too :)
But I didn't think of explicit it :/
Can you take both of them directly or do you prefer I RESEND?

> 
> qemu/linux-user/flatload.c: In function 'load_flt_binary':
> qemu/linux-user/flatload.c:742:9: error: implicit declaration of
> function 'is_error'; did you mean 'g_error'?
> [-Werror=implicit-function-declaration]
>      if (is_error(res)) {
>          ^~~~~~~~
>          g_error
> qemu/linux-user/flatload.c:742:9: error: nested extern declaration of
> 'is_error' [-Werror=nested-externs]
> 
> Thanks,
> Laurent
> 
> 
> 

Re: [Qemu-devel] [PATCH for 2.10 26/35] linux-user: use is_error() to avoid warnings and make the code clearer

[Laurent Vivier]

Le 04/06/2018 à 16:20, Philippe Mathieu-Daudé a écrit :
> On 06/03/2018 08:33 PM, Laurent Vivier wrote:
>> Le 29/05/2018 à 17:19, Laurent Vivier a écrit :
>>> Le 29/05/2018 à 16:25, Philippe Mathieu-Daudé a écrit :
>>>> Hi Laurent,
>>>
>>> Hi Philippe,
>>>
>>>> On 07/24/2017 04:16 PM, Laurent Vivier wrote:
>>>>> Le 24/07/2017 à 20:27, Philippe Mathieu-Daudé a écrit :
>>>>>> linux-user/flatload.c:740:9: warning: Loss of sign in implicit conversion
>>>>>>     if (res > (unsigned long)-4096)
>>>>>>         ^~~
>>>>>>
>>>>>> Reported-by: Clang Static Analyzer
>>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>>
>>>>> Reviewed-by: Laurent Vivier <laurent@vivier.eu>
>>>>>
>>>>>> ---
>>>>>>  linux-user/flatload.c | 15 +++++++++------
>>>>>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/linux-user/flatload.c b/linux-user/flatload.c
>>>>>> index a35a560904..10c529910f 100644
>>>>>> --- a/linux-user/flatload.c
>>>>>> +++ b/linux-user/flatload.c
>>>>>> @@ -224,8 +224,9 @@ static int decompress_exec(
>>>>>>  		ret = bprm->file->f_op->read(bprm->file, buf, LBUFSIZE, &fpos);
>>>>>>  		if (ret <= 0)
>>>>>>  			break;
>>>>>> -		if (ret >= (unsigned long) -4096)
>>>>>> +                if (is_error(ret)) {
>>>>>>  			break;
>>>>>> +                }
>>>>>>  		len -= ret;
>>>>>>  
>>>>>>  		strm.next_in = buf;
>>>>>> @@ -283,8 +284,7 @@ calc_reloc(abi_ulong r, struct lib_info *p, int curid, int internalp)
>>>>>>                      "in same module (%d != %d)\n",
>>>>>>                      (unsigned) r, curid, id);
>>>>>>              goto failed;
>>>>>> -        } else if ( ! p[id].loaded &&
>>>>>> -                    load_flat_shared_library(id, p) > (unsigned long) -4096) {
>>>>>> +        } else if (!p[id].loaded && is_error(load_flat_shared_library(id, p))) {
>>>>>>              fprintf(stderr, "BINFMT_FLAT: failed to load library %d\n", id);
>>>>>>              goto failed;
>>>>>>          }
>>>>>> @@ -523,9 +523,10 @@ static int load_flat_file(struct linux_binprm * bprm,
>>>>>>                  fpos = 0;
>>>>>>                  result = bprm->file->f_op->read(bprm->file,
>>>>>>                                  (char *) textpos, text_len, &fpos);
>>>>>> -                if (result < (unsigned long) -4096)
>>>>>> +                if (!is_error(result)) {
>>>>>>                          result = decompress_exec(bprm, text_len, (char *) datapos,
>>>>>>                                           data_len + (relocs * sizeof(unsigned long)), 0);
>>>>>> +                }
>>>>>>          }
>>>>>>          else
>>>>>>  #endif
>>>>>> @@ -693,8 +694,9 @@ static int load_flat_shared_library(int id, struct lib_info *libs)
>>>>>>  
>>>>>>  	res = prepare_binprm(&bprm);
>>>>>>  
>>>>>> -	if (res <= (unsigned long)-4096)
>>>>>> +        if (!is_error(res)) {
>>>>>>  		res = load_flat_file(&bprm, libs, id, NULL);
>>>>>> +        }
>>>>>>  	if (bprm.file) {
>>>>>>  		allow_write_access(bprm.file);
>>>>>>  		fput(bprm.file);
>>>>>> @@ -737,8 +739,9 @@ int load_flt_binary(struct linux_binprm *bprm, struct image_info *info)
>>>>>>  
>>>>>>  
>>>>>>      res = load_flat_file(bprm, libinfo, 0, &stack_len);
>>>>>> -    if (res > (unsigned long)-4096)
>>>>>> +    if (is_error(res)) {
>>>>>>              return res;
>>>>>> +    }
>>>>>>  
>>>>>>      /* Update data segment pointers for all libraries */
>>>>>>      for (i=0; i<MAX_SHARED_LIBS; i++) {
>>>>>>
>>>>
>>>> Can you take this via your linux-user tree?
>>>>
>>>
>>> Applied, thanks.
>>
>> Unapplied, it needs a rebase:
> 
> No rebase required, it just need the previous patch applied too :)
> But I didn't think of explicit it :/
> Can you take both of them directly or do you prefer I RESEND?
> 
>>
>> qemu/linux-user/flatload.c: In function 'load_flt_binary':
>> qemu/linux-user/flatload.c:742:9: error: implicit declaration of
>> function 'is_error'; did you mean 'g_error'?
>> [-Werror=implicit-function-declaration]
>>      if (is_error(res)) {
>>          ^~~~~~~~
>>          g_error
>> qemu/linux-user/flatload.c:742:9: error: nested extern declaration of
>> 'is_error' [-Werror=nested-externs]
>>

I have already my next pull-request in test phase without this one, so
the better is to resend a series with the patches you want to be merged.

Thanks,
Laurent

[Qemu-devel] [PATCH for 2.10 27/35] syscall: fix dereference of undefined pointer

[Philippe Mathieu-Daudé]

Clang's scan-build-5.0 reports:

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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index fcd20fa276..e79b5baec4 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -5524,7 +5524,7 @@ static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
     int target_size;
     void *argptr;
     abi_ulong *target_rt_dev_ptr;
-    unsigned long *host_rt_dev_ptr;
+    unsigned long *host_rt_dev_ptr = NULL;
     abi_long ret;
     int i;
 
@@ -5570,6 +5570,7 @@ static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
     unlock_user(argptr, arg, 0);
 
     ret = get_errno(safe_ioctl(fd, ie->host_cmd, buf_temp));
+    assert(host_rt_dev_ptr);
     if (*host_rt_dev_ptr != 0) {
         unlock_user((void *)*host_rt_dev_ptr,
                     *target_rt_dev_ptr, 0);
-- 
2.13.3

Re: [Qemu-devel] [PATCH for 2.10 27/35] syscall: fix dereference of undefined pointer

[Laurent Vivier]

Le 24/07/2017 à 20:27, Philippe Mathieu-Daudé a écrit :
> Clang's scan-build-5.0 reports:
> 
> 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 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index fcd20fa276..e79b5baec4 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -5524,7 +5524,7 @@ static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
>      int target_size;
>      void *argptr;
>      abi_ulong *target_rt_dev_ptr;
> -    unsigned long *host_rt_dev_ptr;
> +    unsigned long *host_rt_dev_ptr = NULL;
>      abi_long ret;
>      int i;
>  
> @@ -5570,6 +5570,7 @@ static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
>      unlock_user(argptr, arg, 0);
>  
>      ret = get_errno(safe_ioctl(fd, ie->host_cmd, buf_temp));
> +    assert(host_rt_dev_ptr);
>      if (*host_rt_dev_ptr != 0) {
>          unlock_user((void *)*host_rt_dev_ptr,
>                      *target_rt_dev_ptr, 0);
> 

I think if you add the assert you should not have to add the "= NULL".
Could you check?

Thanks,
Laurent

Re: [Qemu-devel] [PATCH for 2.10 27/35] syscall: fix dereference of undefined pointer

[Philippe Mathieu-Daudé]

On 07/24/2017 04:33 PM, Laurent Vivier wrote:
> Le 24/07/2017 à 20:27, Philippe Mathieu-Daudé a écrit :
>> Clang's scan-build-5.0 reports:
>>
>> 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 | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index fcd20fa276..e79b5baec4 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -5524,7 +5524,7 @@ static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
>>       int target_size;
>>       void *argptr;
>>       abi_ulong *target_rt_dev_ptr;
>> -    unsigned long *host_rt_dev_ptr;
>> +    unsigned long *host_rt_dev_ptr = NULL;
>>       abi_long ret;
>>       int i;
>>   
>> @@ -5570,6 +5570,7 @@ static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
>>       unlock_user(argptr, arg, 0);
>>   
>>       ret = get_errno(safe_ioctl(fd, ie->host_cmd, buf_temp));
>> +    assert(host_rt_dev_ptr);
>>       if (*host_rt_dev_ptr != 0) {
>>           unlock_user((void *)*host_rt_dev_ptr,
>>                       *target_rt_dev_ptr, 0);
>>
> 
> I think if you add the assert you should not have to add the "= NULL".
> Could you check?

Indeed!

Thanks :)

Phil.

Re: [Qemu-devel] [PATCH for 2.10 27/35] syscall: fix dereference of undefined pointer

[Peter Maydell]

On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Clang's scan-build-5.0 reports:
>
> 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 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index fcd20fa276..e79b5baec4 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -5524,7 +5524,7 @@ static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
>      int target_size;
>      void *argptr;
>      abi_ulong *target_rt_dev_ptr;
> -    unsigned long *host_rt_dev_ptr;
> +    unsigned long *host_rt_dev_ptr = NULL;
>      abi_long ret;
>      int i;
>
> @@ -5570,6 +5570,7 @@ static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
>      unlock_user(argptr, arg, 0);
>
>      ret = get_errno(safe_ioctl(fd, ie->host_cmd, buf_temp));
> +    assert(host_rt_dev_ptr);

There's not much point in this assert because it doesn't
tell us anything we're not about to find out immediately
by dereferencing the pointer...

>      if (*host_rt_dev_ptr != 0) {
>          unlock_user((void *)*host_rt_dev_ptr,
>                      *target_rt_dev_ptr, 0);
> --
> 2.13.3

thanks
-- PMM

Re: [Qemu-devel] [PATCH for 2.10 27/35] syscall: fix dereference of undefined pointer

[Laurent Vivier]

Le 24/07/2017 à 23:26, Peter Maydell a écrit :
> On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Clang's scan-build-5.0 reports:
>>
>> 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 | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index fcd20fa276..e79b5baec4 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -5524,7 +5524,7 @@ static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
>>      int target_size;
>>      void *argptr;
>>      abi_ulong *target_rt_dev_ptr;
>> -    unsigned long *host_rt_dev_ptr;
>> +    unsigned long *host_rt_dev_ptr = NULL;
>>      abi_long ret;
>>      int i;
>>
>> @@ -5570,6 +5570,7 @@ static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
>>      unlock_user(argptr, arg, 0);
>>
>>      ret = get_errno(safe_ioctl(fd, ie->host_cmd, buf_temp));
>> +    assert(host_rt_dev_ptr);
> 
> There's not much point in this assert because it doesn't
> tell us anything we're not about to find out immediately
> by dereferencing the pointer...

It's just to shut off the warning.

What I said in the comment of the previous version of this patch:

I think we should "assert(host_rt_dev_ptr)" here. It's a bug if
host_rt_dev_ptr is not set.

The "for" loop scans the structure to find the rt_dev field, and we
should always enter in the first "if", so "host_rt_dev_ptr" is always set.

Thanks,
Laurent

[Qemu-devel] [PATCH for 2.10 28/35] syscall: check dup/dup2/dup3() errors, return EBADFD/EINVAL if required

[Philippe Mathieu-Daudé]

Linux dup(2) manpage:

ERRORS
  EBADF
    newfd is out of the allowed range for file descriptors (like negative)
  EINVAL (dup3())
    oldfd was equal to newfd

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 linux-user/syscall.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index e79b5baec4..637270a02d 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -8336,9 +8336,13 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
         break;
 #endif
     case TARGET_NR_dup:
-        ret = get_errno(dup(arg1));
-        if (ret >= 0) {
-            fd_trans_dup(arg1, ret);
+        if (arg1 < 0) {
+            ret = -TARGET_EBADFD;
+        } else {
+            ret = get_errno(dup(arg1));
+            if (ret >= 0) {
+                fd_trans_dup(arg1, ret);
+            }
         }
         break;
 #ifdef TARGET_NR_pipe
@@ -8436,17 +8440,27 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
 #endif
 #ifdef TARGET_NR_dup2
     case TARGET_NR_dup2:
-        ret = get_errno(dup2(arg1, arg2));
-        if (ret >= 0) {
-            fd_trans_dup(arg1, arg2);
+        if (arg1 < 0 || arg2 < 0) {
+            ret = -TARGET_EBADFD;
+        } else {
+            ret = get_errno(dup2(arg1, arg2));
+            if (ret >= 0) {
+                fd_trans_dup(arg1, arg2);
+            }
         }
         break;
 #endif
 #if defined(CONFIG_DUP3) && defined(TARGET_NR_dup3)
     case TARGET_NR_dup3:
-        ret = get_errno(dup3(arg1, arg2, arg3));
-        if (ret >= 0) {
-            fd_trans_dup(arg1, arg2);
+        if (arg1 < 0 || arg2 < 0) {
+            ret = -TARGET_EBADFD;
+        } else if (arg1 == arg2) {
+            ret = -TARGET_EINVAL;
+        } else {
+            ret = get_errno(dup3(arg1, arg2, arg3));
+            if (ret >= 0) {
+                fd_trans_dup(arg1, arg2);
+            }
         }
         break;
 #endif
-- 
2.13.3

Re: [Qemu-devel] [PATCH for 2.10 28/35] syscall: check dup/dup2/dup3() errors, return EBADFD/EINVAL if required

[Laurent Vivier]

Le 24/07/2017 à 20:27, Philippe Mathieu-Daudé a écrit :
> Linux dup(2) manpage:
> 
> ERRORS
>   EBADF
>     newfd is out of the allowed range for file descriptors (like negative)
>   EINVAL (dup3())
>     oldfd was equal to newfd
> 
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  linux-user/syscall.c | 32 +++++++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 9 deletions(-)
> 


I don't understand why we need that: the dup()/dup2()/dup3() functions
will return the good error number.

Could you be more explicit on the reason?

Thanks,
Laurent

[Qemu-devel] [PATCH for 2.10 29/35] syscall: fix out-of-bound memory access

[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
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 637270a02d..26450d235f 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -544,6 +544,7 @@ static void fd_trans_register(int fd, TargetFdTrans *trans)
 {
     unsigned int oldmax;
 
+    assert(fd >= 0);
     if (fd >= target_fd_max) {
         oldmax = target_fd_max;
         target_fd_max = ((fd >> 6) + 1) << 6; /* by slice of 64 entries */
-- 
2.13.3

Re: [Qemu-devel] [PATCH for 2.10 29/35] syscall: fix out-of-bound memory access

[Laurent Vivier]

Le 24/07/2017 à 20:27, 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
> 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 637270a02d..26450d235f 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -544,6 +544,7 @@ static void fd_trans_register(int fd, TargetFdTrans *trans)
>  {
>      unsigned int oldmax;
>  
> +    assert(fd >= 0);
>      if (fd >= target_fd_max) {
>          oldmax = target_fd_max;
>          target_fd_max = ((fd >> 6) + 1) << 6; /* by slice of 64 entries */
> 

I think we should fix inotify_init(), inotify_init1(), eventfd() and
eventfd2() that call fd_trans_register() without checking the value of
fd before adding this assert...

Thanks,
Laurent

[Qemu-devel] [PATCH for 2.10 30/35] 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>
---
 linux-user/syscall.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 26450d235f..963b9c8f4b 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1618,6 +1618,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

Re: [Qemu-devel] [PATCH for 2.10 30/35] syscall: fix use of uninitialized values

[Laurent Vivier]

Le 24/07/2017 à 20:27, Philippe Mathieu-Daudé a écrit :
> 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>
> ---
>  linux-user/syscall.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 26450d235f..963b9c8f4b 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -1618,6 +1618,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)
> 

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

[Qemu-devel] [PATCH for 2.10 31/35] syscall: replace strcpy() by g_strlcpy()

[Philippe Mathieu-Daudé]

linux-user/syscall.c:9860:17: warning: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119
                strcpy (buf->machine, cpu_to_uname_machine(cpu_env));
                ^~~~~~

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

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 963b9c8f4b..847f729834 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -9853,7 +9853,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
             if (!is_error(ret)) {
                 /* Overwrite the native machine name with whatever is being
                    emulated. */
-                strcpy (buf->machine, cpu_to_uname_machine(cpu_env));
+                g_strlcpy(buf->machine, cpu_to_uname_machine(cpu_env),
+                          sizeof(buf->machine));
                 /* Allow the user to override the reported release.  */
                 if (qemu_uname_release && *qemu_uname_release) {
                     g_strlcpy(buf->release, qemu_uname_release,
-- 
2.13.3

Re: [Qemu-devel] [PATCH for 2.10 31/35] syscall: replace strcpy() by g_strlcpy()

[Laurent Vivier]

Le 24/07/2017 à 20:27, Philippe Mathieu-Daudé a écrit :
> linux-user/syscall.c:9860:17: warning: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119
>                 strcpy (buf->machine, cpu_to_uname_machine(cpu_env));
>                 ^~~~~~
> 
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  linux-user/syscall.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 963b9c8f4b..847f729834 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -9853,7 +9853,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>              if (!is_error(ret)) {
>                  /* Overwrite the native machine name with whatever is being
>                     emulated. */
> -                strcpy (buf->machine, cpu_to_uname_machine(cpu_env));
> +                g_strlcpy(buf->machine, cpu_to_uname_machine(cpu_env),
> +                          sizeof(buf->machine));
>                  /* Allow the user to override the reported release.  */
>                  if (qemu_uname_release && *qemu_uname_release) {
>                      g_strlcpy(buf->release, qemu_uname_release,
> 

We should not have a problem here as cpu_to_uname_machine() is "const
char *" and the string is defined inside QEMU (so it should fit into
machine[]).

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

Re: [Qemu-devel] [PATCH for 2.10 31/35] syscall: replace strcpy() by g_strlcpy()

[Philippe Mathieu-Daudé]

Hi Laurent,

On 07/24/2017 04:28 PM, Laurent Vivier wrote:
> Le 24/07/2017 à 20:27, Philippe Mathieu-Daudé a écrit :
>> linux-user/syscall.c:9860:17: warning: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119
>>                 strcpy (buf->machine, cpu_to_uname_machine(cpu_env));
>>                 ^~~~~~
>>
>> Reported-by: Clang Static Analyzer
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  linux-user/syscall.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index 963b9c8f4b..847f729834 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -9853,7 +9853,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>>              if (!is_error(ret)) {
>>                  /* Overwrite the native machine name with whatever is being
>>                     emulated. */
>> -                strcpy (buf->machine, cpu_to_uname_machine(cpu_env));
>> +                g_strlcpy(buf->machine, cpu_to_uname_machine(cpu_env),
>> +                          sizeof(buf->machine));
>>                  /* Allow the user to override the reported release.  */
>>                  if (qemu_uname_release && *qemu_uname_release) {
>>                      g_strlcpy(buf->release, qemu_uname_release,
>>
> 
> We should not have a problem here as cpu_to_uname_machine() is "const
> char *" and the string is defined inside QEMU (so it should fit into
> machine[]).
> 
> Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Do you mind queuing this patch in your linux-user tree?

Thanks,

Phil.

Re: [Qemu-devel] [PATCH for 2.10 31/35] syscall: replace strcpy() by g_strlcpy()

[Laurent Vivier]

Le 29/05/2018 à 16:19, Philippe Mathieu-Daudé a écrit :
> Hi Laurent,
> 
> On 07/24/2017 04:28 PM, Laurent Vivier wrote:
>> Le 24/07/2017 à 20:27, Philippe Mathieu-Daudé a écrit :
>>> linux-user/syscall.c:9860:17: warning: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119
>>>                 strcpy (buf->machine, cpu_to_uname_machine(cpu_env));
>>>                 ^~~~~~
>>>
>>> Reported-by: Clang Static Analyzer
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>  linux-user/syscall.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>>> index 963b9c8f4b..847f729834 100644
>>> --- a/linux-user/syscall.c
>>> +++ b/linux-user/syscall.c
>>> @@ -9853,7 +9853,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>>>              if (!is_error(ret)) {
>>>                  /* Overwrite the native machine name with whatever is being
>>>                     emulated. */
>>> -                strcpy (buf->machine, cpu_to_uname_machine(cpu_env));
>>> +                g_strlcpy(buf->machine, cpu_to_uname_machine(cpu_env),
>>> +                          sizeof(buf->machine));
>>>                  /* Allow the user to override the reported release.  */
>>>                  if (qemu_uname_release && *qemu_uname_release) {
>>>                      g_strlcpy(buf->release, qemu_uname_release,
>>>
>>
>> We should not have a problem here as cpu_to_uname_machine() is "const
>> char *" and the string is defined inside QEMU (so it should fit into
>> machine[]).
>>
>> Reviewed-by: Laurent Vivier <laurent@vivier.eu>
> 
> Do you mind queuing this patch in your linux-user tree?

Applied, thanks

Laurent

[Qemu-devel] [PATCH for 2.10 32/35] timer/pxa2xx: silent warning about out-of-bound memory access

[Philippe Mathieu-Daudé]

Unlikely to happen.

hw/timer/pxa2xx_timer.c:145:19: warning: Out of bound memory access (accessed memory precedes memory block)
        counter = counters[n];
                  ^~~~~~~~~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/timer/pxa2xx_timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/timer/pxa2xx_timer.c b/hw/timer/pxa2xx_timer.c
index 68ba5a70b3..d47f463636 100644
--- a/hw/timer/pxa2xx_timer.c
+++ b/hw/timer/pxa2xx_timer.c
@@ -139,7 +139,7 @@ static void pxa2xx_timer_update4(void *opaque, uint64_t now_qemu, int n)
     if (s->tm4[n].control & (1 << 7))
         counter = n;
     else
-        counter = counters[n];
+        counter = counters[n & 7];
 
     if (!s->tm4[counter].freq) {
         timer_del(s->tm4[n].tm.qtimer);
-- 
2.13.3

Re: [Qemu-devel] [PATCH for 2.10 32/35] timer/pxa2xx: silent warning about out-of-bound memory access

[Peter Maydell]

On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Unlikely to happen.
>
> hw/timer/pxa2xx_timer.c:145:19: warning: Out of bound memory access (accessed memory precedes memory block)
>         counter = counters[n];
>                   ^~~~~~~~~~~
>
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/timer/pxa2xx_timer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/timer/pxa2xx_timer.c b/hw/timer/pxa2xx_timer.c
> index 68ba5a70b3..d47f463636 100644
> --- a/hw/timer/pxa2xx_timer.c
> +++ b/hw/timer/pxa2xx_timer.c
> @@ -139,7 +139,7 @@ static void pxa2xx_timer_update4(void *opaque, uint64_t now_qemu, int n)
>      if (s->tm4[n].control & (1 << 7))
>          counter = n;
>      else
> -        counter = counters[n];
> +        counter = counters[n & 7];
>
>      if (!s->tm4[counter].freq) {
>          timer_del(s->tm4[n].tm.qtimer);
> --

This looks rather odd, because we use a mask to guard
the counters[] array index, but we do an access into
another 8-element array with n both immediately
above and immediately below that.

It's not actually possible to call this function
with n not between 0 and 7 -- if the static
analyser can't figure that out does adding an
assert at the top of the function help it out?

thanks
-- PMM

Re: [Qemu-devel] [PATCH for 2.10 32/35] timer/pxa2xx: silent warning about out-of-bound memory access

[Philippe Mathieu-Daudé]

On 07/24/2017 06:01 PM, Peter Maydell wrote:
> On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Unlikely to happen.
>>
>> hw/timer/pxa2xx_timer.c:145:19: warning: Out of bound memory access (accessed memory precedes memory block)
>>          counter = counters[n];
>>                    ^~~~~~~~~~~
>>
>> Reported-by: Clang Static Analyzer
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   hw/timer/pxa2xx_timer.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/timer/pxa2xx_timer.c b/hw/timer/pxa2xx_timer.c
>> index 68ba5a70b3..d47f463636 100644
>> --- a/hw/timer/pxa2xx_timer.c
>> +++ b/hw/timer/pxa2xx_timer.c
>> @@ -139,7 +139,7 @@ static void pxa2xx_timer_update4(void *opaque, uint64_t now_qemu, int n)
>>       if (s->tm4[n].control & (1 << 7))
>>           counter = n;
>>       else
>> -        counter = counters[n];
>> +        counter = counters[n & 7];
>>
>>       if (!s->tm4[counter].freq) {
>>           timer_del(s->tm4[n].tm.qtimer);
>> --
> 
> This looks rather odd, because we use a mask to guard
> the counters[] array index, but we do an access into
> another 8-element array with n both immediately
> above and immediately below that.
> 
> It's not actually possible to call this function
> with n not between 0 and 7 -- if the static
> analyser can't figure that out does adding an
> assert at the top of the function help it out?

Yep, I'm keeping patches with "static analyzer hints" for 2.11 unless 
there is interest in having them in 2.10 (this patch now included in 
that 2.11 series).

Thanks,

Phil.

[Qemu-devel] [RFC PATCH for 2.10 33/35] configure: clang does not support -Wexpansion-to-defined

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 configure | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index 6b52e19ee3..48295aa1e6 100755
--- a/configure
+++ b/configure
@@ -1582,8 +1582,12 @@ gcc_flags="-Wold-style-declaration -Wold-style-definition -Wtype-limits"
 gcc_flags="-Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers $gcc_flags"
 gcc_flags="-Wno-missing-include-dirs -Wempty-body -Wnested-externs $gcc_flags"
 gcc_flags="-Wendif-labels -Wno-shift-negative-value $gcc_flags"
-gcc_flags="-Wno-initializer-overrides -Wexpansion-to-defined $gcc_flags"
+gcc_flags="-Wno-initializer-overrides $gcc_flags"
 gcc_flags="-Wno-string-plus-int $gcc_flags"
+# clang does not support -Wexpansion-to-defined
+if ! echo | $cc -dM -E - | grep __clang__ > /dev/null 2>&1 ; then
+    gcc_flags="-Wexpansion-to-defined $gcc_flags"
+fi
 # Note that we do not add -Werror to gcc_flags here, because that would
 # enable it for all configure tests. If a configure test failed due
 # to -Werror this would just silently disable some features,
-- 
2.13.3

Re: [Qemu-devel] [RFC PATCH for 2.10 33/35] configure: clang does not support -Wexpansion-to-defined

[Paolo Bonzini]

On 24/07/2017 20:27, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  configure | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/configure b/configure
> index 6b52e19ee3..48295aa1e6 100755
> --- a/configure
> +++ b/configure
> @@ -1582,8 +1582,12 @@ gcc_flags="-Wold-style-declaration -Wold-style-definition -Wtype-limits"
>  gcc_flags="-Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers $gcc_flags"
>  gcc_flags="-Wno-missing-include-dirs -Wempty-body -Wnested-externs $gcc_flags"
>  gcc_flags="-Wendif-labels -Wno-shift-negative-value $gcc_flags"
> -gcc_flags="-Wno-initializer-overrides -Wexpansion-to-defined $gcc_flags"
> +gcc_flags="-Wno-initializer-overrides $gcc_flags"
>  gcc_flags="-Wno-string-plus-int $gcc_flags"
> +# clang does not support -Wexpansion-to-defined
> +if ! echo | $cc -dM -E - | grep __clang__ > /dev/null 2>&1 ; then
> +    gcc_flags="-Wexpansion-to-defined $gcc_flags"
> +fi
>  # Note that we do not add -Werror to gcc_flags here, because that would
>  # enable it for all configure tests. If a configure test failed due
>  # to -Werror this would just silently disable some features,

Clang 3.9 does.

Paolo

Re: [Qemu-devel] [RFC PATCH for 2.10 33/35] configure: clang does not support -Wexpansion-to-defined

[Peter Maydell]

On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  configure | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/configure b/configure
> index 6b52e19ee3..48295aa1e6 100755
> --- a/configure
> +++ b/configure
> @@ -1582,8 +1582,12 @@ gcc_flags="-Wold-style-declaration -Wold-style-definition -Wtype-limits"
>  gcc_flags="-Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers $gcc_flags"
>  gcc_flags="-Wno-missing-include-dirs -Wempty-body -Wnested-externs $gcc_flags"
>  gcc_flags="-Wendif-labels -Wno-shift-negative-value $gcc_flags"
> -gcc_flags="-Wno-initializer-overrides -Wexpansion-to-defined $gcc_flags"
> +gcc_flags="-Wno-initializer-overrides $gcc_flags"
>  gcc_flags="-Wno-string-plus-int $gcc_flags"
> +# clang does not support -Wexpansion-to-defined
> +if ! echo | $cc -dM -E - | grep __clang__ > /dev/null 2>&1 ; then
> +    gcc_flags="-Wexpansion-to-defined $gcc_flags"
> +fi

We already have a mechanism for testing that the warning
flags we pass here are actually supported by the compiler
(the cc_has_warning_flag() function, which we run on
every flag in $gcc_flags). Why does -Wexpansion-to-defined
need special casing?

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH for 2.10 33/35] configure: clang does not support -Wexpansion-to-defined

[Philippe Mathieu-Daudé]

On 07/24/2017 06:22 PM, Peter Maydell wrote:
> On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   configure | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/configure b/configure
>> index 6b52e19ee3..48295aa1e6 100755
>> --- a/configure
>> +++ b/configure
>> @@ -1582,8 +1582,12 @@ gcc_flags="-Wold-style-declaration -Wold-style-definition -Wtype-limits"
>>   gcc_flags="-Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers $gcc_flags"
>>   gcc_flags="-Wno-missing-include-dirs -Wempty-body -Wnested-externs $gcc_flags"
>>   gcc_flags="-Wendif-labels -Wno-shift-negative-value $gcc_flags"
>> -gcc_flags="-Wno-initializer-overrides -Wexpansion-to-defined $gcc_flags"
>> +gcc_flags="-Wno-initializer-overrides $gcc_flags"
>>   gcc_flags="-Wno-string-plus-int $gcc_flags"
>> +# clang does not support -Wexpansion-to-defined
>> +if ! echo | $cc -dM -E - | grep __clang__ > /dev/null 2>&1 ; then
>> +    gcc_flags="-Wexpansion-to-defined $gcc_flags"
>> +fi
> 
> We already have a mechanism for testing that the warning
> flags we pass here are actually supported by the compiler
> (the cc_has_warning_flag() function, which we run on
> every flag in $gcc_flags). Why does -Wexpansion-to-defined
> need special casing?

Yeah no idea, it works with clang 3.9 but not 5.0, I didn't investigate 
further. I prefixed this patch 'RFC' to not consider it but provide it 
if someone wanted to reproduce the analysis. Maybe I should prefix it 
'XXX' next time or 'NOT FOR MERGE'.

Regards,

Phil.

[Qemu-devel] [RFC PATCH for 2.10 34/35] docker: add debian 'bleeding' image with gcc7 and clang5

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 .../docker/dockerfiles/debian-bleeding-dev.docker  | 94 ++++++++++++++++++++++
 1 file changed, 94 insertions(+)
 create mode 100644 tests/docker/dockerfiles/debian-bleeding-dev.docker

diff --git a/tests/docker/dockerfiles/debian-bleeding-dev.docker b/tests/docker/dockerfiles/debian-bleeding-dev.docker
new file mode 100644
index 0000000000..d6ae20692c
--- /dev/null
+++ b/tests/docker/dockerfiles/debian-bleeding-dev.docker
@@ -0,0 +1,94 @@
+#
+# Docker x86_64 bleeding image
+#
+# This docker target builds on the debian unstable base image. Further
+# libraries which are not widely available are installed by hand.
+#
+FROM debian:unstable
+
+MAINTAINER Philippe Mathieu-Daudé <f4bug@amsat.org>
+
+RUN echo 'Acquire::ForceIPv4 "true";' >> /etc/apt/apt.conf.d/01network
+
+# Duplicate deb line as deb-src
+RUN cat /etc/apt/sources.list | sed "s/^deb\ /deb-src /" >> /etc/apt/sources.list
+
+# Install common build utilities
+RUN apt-get update && \
+    DEBIAN_FRONTEND=noninteractive apt-get install -yy eatmydata
+RUN DEBIAN_FRONTEND=noninteractive eatmydata \
+    apt-get install -y --no-install-recommends \
+        bison \
+        build-essential \
+        ca-certificates \
+        ccache \
+        flex \
+        git \
+        less \
+        pkg-config \
+        psmisc \
+        python \
+        texinfo \
+        $(apt-get -s build-dep qemu | egrep ^Inst | fgrep '[all]' | cut -d\  -f2)
+
+RUN DEBIAN_FRONTEND=noninteractive eatmydata \
+    apt-get install -y --no-install-recommends \
+        binutils-multiarch \
+        gdb-multiarch \
+        libnewlib-dev \
+        musl-dev \
+        uclibc-source
+
+############################################################################
+# GCC 7
+
+RUN echo deb http://httpredir.debian.org/debian experimental main >> \
+    /etc/apt/sources.list.d/experimental.list && apt-get update
+RUN DEBIAN_FRONTEND=noninteractive eatmydata \
+    apt-get install -y --no-install-recommends \
+        gcc-7 \
+        gcc-7-cross-base-ports \
+        gcc-7-multilib \
+        g++-7
+
+############################################################################
+# Clang 5
+
+RUN DEBIAN_FRONTEND=noninteractive eatmydata \
+    apt-get install -y --no-install-recommends \
+        curl gnupg
+RUN curl http://apt.llvm.org/llvm-snapshot.gpg.key | apt-key add - && \
+    echo 'deb http://apt.llvm.org/unstable/ llvm-toolchain main' \
+        > /etc/apt/sources.list.d/llvm.list && \
+    apt-get update
+RUN DEBIAN_FRONTEND=noninteractive eatmydata \
+    apt-get install -y --no-install-recommends \
+        clang-5.0 \
+        lldb-5.0 \
+        llvm-5.0 && \
+    test -x /usr/bin/clang || ln -s clang-5.0 /usr/bin/clang
+
+############################################################################
+# QEMU specific starts here
+
+RUN DEBIAN_FRONTEND=noninteractive eatmydata \
+    apt-get build-dep -yy qemu
+
+RUN DEBIAN_FRONTEND=noninteractive eatmydata \
+    apt-get install -y --no-install-recommends \
+        libbz2-dev \
+        liblzo2-dev \
+        librdmacm-dev \
+        libsnappy-dev \
+        libvte-dev
+
+# virgl
+RUN DEBIAN_FRONTEND=noninteractive eatmydata \
+    apt-get install -y --no-install-recommends \
+        libegl1-mesa-dev \
+        libepoxy-dev \
+        libgbm-dev
+RUN git clone git://anongit.freedesktop.org/virglrenderer /usr/src/virglrenderer
+RUN cd /usr/src/virglrenderer && ./autogen.sh && ./configure --with-glx --disable-tests && make install
+
+ENV QEMU_CONFIGURE_OPTS --host-cc=clang-5.0 --cc=clang-5.0 --cxx=clang++-5.0
-- 
2.13.3

Re: [Qemu-devel] [RFC PATCH for 2.10 34/35] docker: add debian 'bleeding' image with gcc7 and clang5

[Alex Bennée]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  .../docker/dockerfiles/debian-bleeding-dev.docker  | 94 ++++++++++++++++++++++
>  1 file changed, 94 insertions(+)
>  create mode 100644 tests/docker/dockerfiles/debian-bleeding-dev.docker
>
> diff --git a/tests/docker/dockerfiles/debian-bleeding-dev.docker b/tests/docker/dockerfiles/debian-bleeding-dev.docker
> new file mode 100644
> index 0000000000..d6ae20692c
> --- /dev/null
> +++ b/tests/docker/dockerfiles/debian-bleeding-dev.docker
> @@ -0,0 +1,94 @@
> +#
> +# Docker x86_64 bleeding image
> +#
> +# This docker target builds on the debian unstable base image. Further
> +# libraries which are not widely available are installed by hand.
> +#

Not for 2.10 please.

> +FROM debian:unstable
> +
> +MAINTAINER Philippe Mathieu-Daudé <f4bug@amsat.org>
> +
> +RUN echo 'Acquire::ForceIPv4 "true";' >> /etc/apt/apt.conf.d/01network
> +
> +# Duplicate deb line as deb-src
> +RUN cat /etc/apt/sources.list | sed "s/^deb\ /deb-src /" >> /etc/apt/sources.list
> +
> +# Install common build utilities
> +RUN apt-get update && \
> +    DEBIAN_FRONTEND=noninteractive apt-get install -yy eatmydata
> +RUN DEBIAN_FRONTEND=noninteractive eatmydata \
> +    apt-get install -y --no-install-recommends \
> +        bison \
> +        build-essential \
> +        ca-certificates \
> +        ccache \
> +        flex \
> +        git \
> +        less \
> +        pkg-config \
> +        psmisc \
> +        python \
> +        texinfo \
> +        $(apt-get -s build-dep qemu | egrep ^Inst | fgrep '[all]' | cut -d\  -f2)
> +
> +RUN DEBIAN_FRONTEND=noninteractive eatmydata \
> +    apt-get install -y --no-install-recommends \
> +        binutils-multiarch \
> +        gdb-multiarch \
> +        libnewlib-dev \
> +        musl-dev \
> +        uclibc-source
> +
> +############################################################################
> +# GCC 7
> +
> +RUN echo deb http://httpredir.debian.org/debian experimental main >> \
> +    /etc/apt/sources.list.d/experimental.list && apt-get update
> +RUN DEBIAN_FRONTEND=noninteractive eatmydata \
> +    apt-get install -y --no-install-recommends \
> +        gcc-7 \
> +        gcc-7-cross-base-ports \
> +        gcc-7-multilib \
> +        g++-7
> +
> +############################################################################
> +# Clang 5
> +
> +RUN DEBIAN_FRONTEND=noninteractive eatmydata \
> +    apt-get install -y --no-install-recommends \
> +        curl gnupg
> +RUN curl http://apt.llvm.org/llvm-snapshot.gpg.key | apt-key add - && \
> +    echo 'deb http://apt.llvm.org/unstable/ llvm-toolchain main' \
> +        > /etc/apt/sources.list.d/llvm.list && \
> +    apt-get update
> +RUN DEBIAN_FRONTEND=noninteractive eatmydata \
> +    apt-get install -y --no-install-recommends \
> +        clang-5.0 \
> +        lldb-5.0 \
> +        llvm-5.0 && \
> +    test -x /usr/bin/clang || ln -s clang-5.0 /usr/bin/clang
> +
> +############################################################################
> +# QEMU specific starts here
> +
> +RUN DEBIAN_FRONTEND=noninteractive eatmydata \
> +    apt-get build-dep -yy qemu
> +
> +RUN DEBIAN_FRONTEND=noninteractive eatmydata \
> +    apt-get install -y --no-install-recommends \
> +        libbz2-dev \
> +        liblzo2-dev \
> +        librdmacm-dev \
> +        libsnappy-dev \
> +        libvte-dev
> +
> +# virgl
> +RUN DEBIAN_FRONTEND=noninteractive eatmydata \
> +    apt-get install -y --no-install-recommends \
> +        libegl1-mesa-dev \
> +        libepoxy-dev \
> +        libgbm-dev
> +RUN git clone git://anongit.freedesktop.org/virglrenderer /usr/src/virglrenderer
> +RUN cd /usr/src/virglrenderer && ./autogen.sh && ./configure
> --with-glx --disable-tests && make install

There are a lot of moving parts basing this in debian unstable and
compiling extra bleeding edge stuff. What does this buy that the clang
and toolchain builds in Travis don't already cover?

> +
> +ENV QEMU_CONFIGURE_OPTS --host-cc=clang-5.0 --cc=clang-5.0 --cxx=clang++-5.0


--
Alex Bennée

Re: [Qemu-devel] [RFC PATCH for 2.10 34/35] docker: add debian 'bleeding' image with gcc7 and clang5

[Daniel P. Berrange]

On Tue, Jul 25, 2017 at 03:15:07PM +0100, Alex Bennée wrote:
> 
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> 
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> >  .../docker/dockerfiles/debian-bleeding-dev.docker  | 94 ++++++++++++++++++++++
> >  1 file changed, 94 insertions(+)
> >  create mode 100644 tests/docker/dockerfiles/debian-bleeding-dev.docker
> >
> > diff --git a/tests/docker/dockerfiles/debian-bleeding-dev.docker b/tests/docker/dockerfiles/debian-bleeding-dev.docker
> > new file mode 100644
> > index 0000000000..d6ae20692c
> > --- /dev/null
> > +++ b/tests/docker/dockerfiles/debian-bleeding-dev.docker


> > +RUN git clone git://anongit.freedesktop.org/virglrenderer /usr/src/virglrenderer
> > +RUN cd /usr/src/virglrenderer && ./autogen.sh && ./configure
> > --with-glx --disable-tests && make install
> 
> There are a lot of moving parts basing this in debian unstable and
> compiling extra bleeding edge stuff. What does this buy that the clang
> and toolchain builds in Travis don't already cover?

FWIW, the clang version in Travis is somewhat old compared to the version
that Peter uses during merge testing. I recently had a pull request that
passed travis tests, but failed with modern clang.

Doesn't neccessarily mean we need debian bleeding edge though - a Fedora
26 image would have detected that since it has new clang.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [RFC PATCH for 2.10 34/35] docker: add debian 'bleeding' image with gcc7 and clang5

[Alex Bennée]

Daniel P. Berrange <berrange@redhat.com> writes:

> On Tue, Jul 25, 2017 at 03:15:07PM +0100, Alex Bennée wrote:
>>
>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>>
>> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> > ---
>> >  .../docker/dockerfiles/debian-bleeding-dev.docker  | 94 ++++++++++++++++++++++
>> >  1 file changed, 94 insertions(+)
>> >  create mode 100644 tests/docker/dockerfiles/debian-bleeding-dev.docker
>> >
>> > diff --git a/tests/docker/dockerfiles/debian-bleeding-dev.docker b/tests/docker/dockerfiles/debian-bleeding-dev.docker
>> > new file mode 100644
>> > index 0000000000..d6ae20692c
>> > --- /dev/null
>> > +++ b/tests/docker/dockerfiles/debian-bleeding-dev.docker
>
>
>> > +RUN git clone git://anongit.freedesktop.org/virglrenderer /usr/src/virglrenderer
>> > +RUN cd /usr/src/virglrenderer && ./autogen.sh && ./configure
>> > --with-glx --disable-tests && make install
>>
>> There are a lot of moving parts basing this in debian unstable and
>> compiling extra bleeding edge stuff. What does this buy that the clang
>> and toolchain builds in Travis don't already cover?
>
> FWIW, the clang version in Travis is somewhat old compared to the version
> that Peter uses during merge testing. I recently had a pull request that
> passed travis tests, but failed with modern clang.
>
> Doesn't neccessarily mean we need debian bleeding edge though - a Fedora
> 26 image would have detected that since it has new clang.

Yeah I think from a compiler testing point of view it would be nice to
have two images, one for latest clang, one for latest gcc that are
pre-set up to build with them for QEMU_CONFIGURE_OPTS. I'd rather those
on a stable base distro than taking a potshot on the status of a rolling
distro on any given day.

The virgl and other tip of tree installs are done once and probably
don't need repeating in other trees.

--
Alex Bennée

Re: [Qemu-devel] [RFC PATCH for 2.10 34/35] docker: add debian 'bleeding' image with gcc7 and clang5

[Philippe Mathieu-Daudé]

On 07/25/2017 11:15 AM, Alex Bennée wrote:
>> +++ b/tests/docker/dockerfiles/debian-bleeding-dev.docker
>> @@ -0,0 +1,94 @@
>> +#
>> +# Docker x86_64 bleeding image
>> +#
>> +# This docker target builds on the debian unstable base image. Further
>> +# libraries which are not widely available are installed by hand.
>> +#
> 
> Not for 2.10 please.

Surely not ;) Also unlikely to get merged in mainline...

[...]
> There are a lot of moving parts basing this in debian unstable and
> compiling extra bleeding edge stuff. What does this buy that the clang
> and toolchain builds in Travis don't already cover?

Bleeding is bleeding :) This is an implicit No Warranty clause...
It might trigger something useful or give you slow death.

https://wiki.debian.org/DebianUnstable
"If you are a hardcore developer or tester you should use this release."

https://www.debian.org/doc/manuals/securing-debian-howto/ch10.en.html#s10.1.3
"you should not use Debian's unstable branch for production-level systems"

Re: [Qemu-devel] [RFC PATCH for 2.10 34/35] docker: add debian 'bleeding' image with gcc7 and clang5

[Alex Bennée]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> On 07/25/2017 11:15 AM, Alex Bennée wrote:
>>> +++ b/tests/docker/dockerfiles/debian-bleeding-dev.docker
>>> @@ -0,0 +1,94 @@
>>> +#
>>> +# Docker x86_64 bleeding image
>>> +#
>>> +# This docker target builds on the debian unstable base image. Further
>>> +# libraries which are not widely available are installed by hand.
>>> +#
>>
>> Not for 2.10 please.
>
> Surely not ;) Also unlikely to get merged in mainline...
>
> [...]
>> There are a lot of moving parts basing this in debian unstable and
>> compiling extra bleeding edge stuff. What does this buy that the clang
>> and toolchain builds in Travis don't already cover?
>
> Bleeding is bleeding :) This is an implicit No Warranty clause...
> It might trigger something useful or give you slow death.
>
> https://wiki.debian.org/DebianUnstable
> "If you are a hardcore developer or tester you should use this release."
>
> https://www.debian.org/doc/manuals/securing-debian-howto/ch10.en.html#s10.1.3
> "you should not use Debian's unstable branch for production-level systems"

Quite but from a QEMU point of view these are being used for CI loops
and proving a known state of something for users to test on. As I said
in reply elsewhere I think to get the benefit of recent Clang and GCCs
it would be best to base of some sort of stable base.

--
Alex Bennée

[Qemu-devel] [RFC PATCH for 2.10 35/35] script to run docker image

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 scripts/dockershell | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100755 scripts/dockershell

diff --git a/scripts/dockershell b/scripts/dockershell
new file mode 100755
index 0000000000..f6a2276172
--- /dev/null
+++ b/scripts/dockershell
@@ -0,0 +1,30 @@
+#! /bin/sh
+
+# This script starts a docker shell with QEMU directory mounted
+#
+# Copyright (C) 2017 Philippe Mathieu-Daudé. GPLv2+.
+#
+# Usage:
+# ./scripts/dockershell qemu:debian-bleeding-dev
+
+if [ $# -lt 1 ]; then
+    echo
+    echo "error: missing image name"
+    echo
+    docker images qemu --no-trunc --format="- {{.Repository}}:{{.Tag}}"
+    exit 1
+fi
+
+SDIR=$(cd "$(dirname "$0")/.." && pwd -P)
+CDIR=$(pwd -P)
+
+VOLS="-v /tmp:/tmp"
+if [ "${SDIR}" = "${CDIR}" ]; then
+    VOLS="${VOLS} -v ${SDIR}:${SDIR} -w ${SDIR}"
+else
+    VOLS="${VOLS} -v ${SDIR}:${SDIR}:ro -v ${CDIR}:${CDIR} -w ${CDIR}"
+fi
+
+docker run --rm -it ${VOLS} \
+    -u $(id -u) \
+    $1
-- 
2.13.3

Re: [Qemu-devel] [RFC PATCH for 2.10 35/35] script to run docker image

[Alex Bennée]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  scripts/dockershell | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>  create mode 100755 scripts/dockershell
>
> diff --git a/scripts/dockershell b/scripts/dockershell
> new file mode 100755
> index 0000000000..f6a2276172
> --- /dev/null
> +++ b/scripts/dockershell
> @@ -0,0 +1,30 @@
> +#! /bin/sh
> +
> +# This script starts a docker shell with QEMU directory mounted
> +#
> +# Copyright (C) 2017 Philippe Mathieu-Daudé. GPLv2+.
> +#
> +# Usage:
> +# ./scripts/dockershell qemu:debian-bleeding-dev
<snip>

This isn't appropriate for 2.10 now we are feature frozen. That said I
have pondered a helper script for this sort of thing before. As it stand
I just rely on my shell to remember the correct invocation for me.

This is the sort of functionality we would need for maybe cross
compiling tests using docker.

--
Alex Bennée

Re: [Qemu-devel] [RFC PATCH for 2.10 35/35] script to run docker image

[Philippe Mathieu-Daudé]

> This isn't appropriate for 2.10 now we are feature frozen. That said I

Surely. From the cover:

"... provided at the end of this series as RFC for people wanted
to give a try (also include 'dockershell' script and a fix in 
./configure). These RFC patches (33-35) are not expected to enter 2.10."

Peter also commented on the "./configure" patch, so I probably 
mis-titled "RFC" those patch

http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg07584.html:
"I prefixed this patch 'RFC' to not consider it but provide it
if someone wanted to reproduce the analysis. Maybe I should prefix it
'XXX' next time or 'NOT FOR MERGE'."

better "NOT FOR REVIEW" or "!RFC"? :p

> have pondered a helper script for this sort of thing before. As it stand
> I just rely on my shell to remember the correct invocation for me.
> 
> This is the sort of functionality we would need for maybe cross
> compiling tests using docker.

This script is way incomplete and far being finished.

Regards,

Phil.

Re: [Qemu-devel] [RFC PATCH for 2.10 35/35] script to run docker image

[Alex Bennée]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

>> This isn't appropriate for 2.10 now we are feature frozen. That said I
>
> Surely. From the cover:
>
> "... provided at the end of this series as RFC for people wanted
> to give a try (also include 'dockershell' script and a fix in
> ./configure). These RFC patches (33-35) are not expected to enter
> 2.10."

My mistake - I hadn't gone over the cover letter. I would usually expect
a series with [PATCH for 2.10] just to contain stuff being considered
for the immediate future.

>
> Peter also commented on the "./configure" patch, so I probably
> mis-titled "RFC" those patch
>
> http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg07584.html:
> "I prefixed this patch 'RFC' to not consider it but provide it
> if someone wanted to reproduce the analysis. Maybe I should prefix it
> 'XXX' next time or 'NOT FOR MERGE'."
>
> better "NOT FOR REVIEW" or "!RFC"? :p

Better to post a logically separate series. It's fine to make an
in-flight series a pre-requisite though (not that I think you need it
here).

>> have pondered a helper script for this sort of thing before. As it stand
>> I just rely on my shell to remember the correct invocation for me.
>>
>> This is the sort of functionality we would need for maybe cross
>> compiling tests using docker.
>
> This script is way incomplete and far being finished.
>
> Regards,
>
> Phil.


--
Alex Bennée

Re: [Qemu-devel] [RFC PATCH for 2.10 35/35] script to run docker image

[Philippe Mathieu-Daudé]

On Tue, Jul 25, 2017 at 12:03 PM, Alex Bennée <alex.bennee@linaro.org> wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>>> This isn't appropriate for 2.10 now we are feature frozen. That said I
>>
>> Surely. From the cover:
>>
>> "... provided at the end of this series as RFC for people wanted
>> to give a try (also include 'dockershell' script and a fix in
>> ./configure). These RFC patches (33-35) are not expected to enter
>> 2.10."
>
> My mistake - I hadn't gone over the cover letter. I would usually expect
> a series with [PATCH for 2.10] just to contain stuff being considered
> for the immediate future.

Ah I see, I used --subject-prefix='PATCH for 2.10' thinking "I'v to
remember to mark the 3 last patches as "RFC" and when I prefixed with
"RFC" I forgot to remove the "for 2.10" suffix. My bad!

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

[Peter Maydell]

On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi,
>
> This series is the result of having fun with Clang's Static Analyzer during
> the week-end (see https://clang-analyzer.llvm.org/).
>
> No need to delay the first batch for -rc0.

I had a quick look through the mostly ARM related ones,
and there seem to be an awful lot of false positives :-(
For getting this into 2.10 it would be helpful to trim
down the series so that it has a more manageable number
of patches in it and they're fixing actual bugs...

thanks
-- PMM