[Qemu-devel] [PATCH v4 0/8] off-by-one and NULL pointer accesses detected by static analysis

[Qemu-devel] [PATCH v4 0/8] off-by-one and NULL pointer accesses detected by static analysis

[Liam Merwick]

Below are a number of fixes to some off-by-one, read outside array bounds, and
NULL pointer accesses detected by an internal Oracle static analysis tool (Parfait).
https://labs.oracle.com/pls/apex/f?p=labs:49:::::P49_PROJECT_ID:13

I have also included a patch to add a command-line option to configure to
select if AVX2 is used or not (keeping the existing behaviour by default).
My motivation was avoiding an issue with the static analysis tool but NetSpectre
was announced as I was working on this and I felt it may have more general uses.

v1 -> v2
Based on feedback from Eric Blake:
patch2: reworded commit message to clarify issue
patch6: Reverted common qlist routines and added assert to qlist_dump instead
patch7: Fixed incorrect logic
patch8: Added QEMU_BUILD_BUG_ON to catch future іnstance at compile-time

v2 -> v3
Based on feedback from Eric Blake:
patch6: removed double space from commit message
patch8: removed unnecessary comment and updated QEMU_BUILD_BUG_ON to use ARRAY_SIZE
Added Eric's R-b to patches 6,7,8

v3 -> v4
Based on feedback from Max Reitz:
patch2: Added R-b from John Snow
patch3: fixed blk_get_attached_dev_id() instead of checking return value
patch4: switched to assert()
patch5: numerous changes based on feedback from Max
patch6: updated commit message
patch7: (was patch8): Added Max's R-b
patch8: (new): patch fixing NULL pointer dereference in kvm_arch_init_vcpu()

I also dropped the 'io: potential unnecessary check in qio_channel_command_new_spawn()'
patch from v3 - it was correct but of no benefit to staic analysis checking

Liam Merwick (8):
  configure: Provide option to explicitly disable AVX2
  job: Fix off-by-one assert checks for JobSTT and JobVerbTable
  block: Null pointer dereference in blk_root_get_parent_desc()
  qemu-img: assert block_job_get() does not return NULL in img_commit()
  block: Fix potential Null pointer dereferences in vvfat.c
  block: dump_qlist() may dereference a Null pointer
  qcow2: Read outside array bounds in qcow2_pre_write_overlap_check()
  kvm: Potential NULL pointer dereference in kvm_arch_init_vcpu()

 block/block-backend.c  |  6 +++++-
 block/qapi.c           |  2 ++
 block/qcow2-refcount.c | 18 ++++++++++--------
 block/vvfat.c          | 33 ++++++++++++++++++++++++++++-----
 configure              | 11 +++++++++--
 dtc                    |  2 +-
 job.c                  |  4 ++--
 qemu-img.c             |  1 +
 target/i386/kvm.c      |  4 +++-
 9 files changed, 61 insertions(+), 20 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v4 1/8] configure: Provide option to explicitly disable AVX2

[Liam Merwick]

The configure script detects if the compiler has AVX2 support and
automatically sets avx2_opt="yes" which in turn defines CONFIG_AVX2_OPT.
There is no way of explicitly overriding this setting so this commit adds
two command-line options: --enable-avx2 and --disable-avx2.

The default behaviour, when no option is specified, is to maintain the
current behaviour and enable AVX2 if the compiler supports it.

Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
Reviewed-by: Darren Kenny <Darren.Kenny@oracle.com>
Reviewed-by: Mark Kanda <Mark.Kanda@oracle.com>
---
 configure | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index 9138af37f8a0..3a3e5f7004ce 100755
--- a/configure
+++ b/configure
@@ -428,7 +428,7 @@ usb_redir=""
 opengl=""
 opengl_dmabuf="no"
 cpuid_h="no"
-avx2_opt="no"
+avx2_opt=""
 zlib="yes"
 capstone=""
 lzo=""
@@ -1332,6 +1332,10 @@ for opt do
   ;;
   --disable-glusterfs) glusterfs="no"
   ;;
+  --disable-avx2) avx2_opt="no"
+  ;;
+  --enable-avx2) avx2_opt="yes"
+  ;;
   --enable-glusterfs) glusterfs="yes"
   ;;
   --disable-virtio-blk-data-plane|--enable-virtio-blk-data-plane)
@@ -1706,6 +1710,7 @@ disabled with --disable-FEATURE, default is enabled if available:
   libxml2         for Parallels image format
   tcmalloc        tcmalloc support
   jemalloc        jemalloc support
+  avx2            AVX2 optimization support
   replication     replication support
   vhost-vsock     virtio sockets device support
   opengl          opengl support
@@ -5094,7 +5099,7 @@ fi
 # There is no point enabling this if cpuid.h is not usable,
 # since we won't be able to select the new routines.
 
-if test $cpuid_h = yes; then
+if test "$cpuid_h" = "yes" -a "$avx2_opt" != "no"; then
   cat > $TMPC << EOF
 #pragma GCC push_options
 #pragma GCC target("avx2")
@@ -5108,6 +5113,8 @@ int main(int argc, char *argv[]) { return bar(argv[0]); }
 EOF
   if compile_object "" ; then
     avx2_opt="yes"
+  else
+    avx2_opt="no"
   fi
 fi
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v4 2/8] job: Fix off-by-one assert checks for JobSTT and JobVerbTable

[Liam Merwick]

In the assert checking the array dereference of JobVerbTable[verb]
in job_apply_verb() the check of the index, verb, allows an overrun
because an index equal to the array size is permitted.

Similarly, in the assert check of JobSTT[s0][s1] with index s1
in job_state_transition(), an off-by-one overrun is not flagged
either.

This is not a run-time issue as there are no callers actually
passing in the max value.

Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
Reviewed-by: Darren Kenny <Darren.Kenny@oracle.com>
Reviewed-by: Mark Kanda <Mark.Kanda@oracle.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 job.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/job.c b/job.c
index c65e01bbfa34..da8e4b7bf2f3 100644
--- a/job.c
+++ b/job.c
@@ -159,7 +159,7 @@ bool job_is_internal(Job *job)
 static void job_state_transition(Job *job, JobStatus s1)
 {
     JobStatus s0 = job->status;
-    assert(s1 >= 0 && s1 <= JOB_STATUS__MAX);
+    assert(s1 >= 0 && s1 < JOB_STATUS__MAX);
     trace_job_state_transition(job, job->ret,
                                JobSTT[s0][s1] ? "allowed" : "disallowed",
                                JobStatus_str(s0), JobStatus_str(s1));
@@ -174,7 +174,7 @@ static void job_state_transition(Job *job, JobStatus s1)
 int job_apply_verb(Job *job, JobVerb verb, Error **errp)
 {
     JobStatus s0 = job->status;
-    assert(verb >= 0 && verb <= JOB_VERB__MAX);
+    assert(verb >= 0 && verb < JOB_VERB__MAX);
     trace_job_apply_verb(job, JobStatus_str(s0), JobVerb_str(verb),
                          JobVerbTable[verb][s0] ? "allowed" : "prohibited");
     if (JobVerbTable[verb][s0]) {
-- 
1.8.3.1

[Qemu-devel] [PATCH v4 3/8] block: Null pointer dereference in blk_root_get_parent_desc()

[Liam Merwick]

The dev_id returned by the call to blk_get_attached_dev_id() in
blk_root_get_parent_desc() can be NULL (an internal call to
object_get_canonical_path may have returned NULL).

Instead of just checking this case before before dereferencing,
adjust blk_get_attached_dev_id() to return the empty string if no
object path can be found (similar to the case when blk->dev is NULL
and an empty string is returned).

Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
---
 block/block-backend.c | 6 +++++-
 dtc                   | 2 +-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index dc0cd5772413..e628920f3cd8 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -909,6 +909,7 @@ void *blk_get_attached_dev(BlockBackend *blk)
 char *blk_get_attached_dev_id(BlockBackend *blk)
 {
     DeviceState *dev;
+    char *dev_id;
 
     assert(!blk->legacy_dev);
     dev = blk->dev;
@@ -918,7 +919,10 @@ char *blk_get_attached_dev_id(BlockBackend *blk)
     } else if (dev->id) {
         return g_strdup(dev->id);
     }
-    return object_get_canonical_path(OBJECT(dev));
+
+    dev_id = object_get_canonical_path(OBJECT(dev));
+
+    return dev_id ? dev_id : g_strdup("");
 }
 
 /*
diff --git a/dtc b/dtc
index 88f18909db73..e54388015af1 160000
--- a/dtc
+++ b/dtc
@@ -1 +1 @@
-Subproject commit 88f18909db731a627456f26d779445f84e449536
+Subproject commit e54388015af1fb4bf04d0bca99caba1074d9cc42
-- 
1.8.3.1

[Qemu-devel] [PATCH v4 4/8] qemu-img: assert block_job_get() does not return NULL in img_commit()

[Liam Merwick]

Although the function block_job_get() can return NULL, it would be a
serious bug if it did so (because the job yields before executing anything
(if it started successfully); but otherwise, commit_active_start() would
have returned an error).  However, as a precaution, before dereferencing
the 'job' pointer in img_commit() assert it is not NULL.

Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
---
 qemu-img.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/qemu-img.c b/qemu-img.c
index b12f4cd19b0a..457aa152296b 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1029,6 +1029,7 @@ static int img_commit(int argc, char **argv)
     }
 
     job = block_job_get("commit");
+    assert(job);
     run_block_job(job, &local_err);
     if (local_err) {
         goto unref_backing;
-- 
1.8.3.1

[Qemu-devel] [PATCH v4 5/8] block: Fix potential Null pointer dereferences in vvfat.c

[Liam Merwick]

The calls to find_mapping_for_cluster() may return NULL but it
isn't always checked for before dereferencing the value returned.
Additionally, add some asserts to cover cases where NULL can't
be returned but which might not be obvious at first glance.

Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
---
 block/vvfat.c | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index fc41841a5c3c..19f6725054a0 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -100,6 +100,7 @@ static inline void array_free(array_t* array)
 /* does not automatically grow */
 static inline void* array_get(array_t* array,unsigned int index) {
     assert(index < array->next);
+    assert(array->pointer);
     return array->pointer + index * array->item_size;
 }
 
@@ -108,8 +109,7 @@ static inline int array_ensure_allocated(array_t* array, int index)
     if((index + 1) * array->item_size > array->size) {
         int new_size = (index + 32) * array->item_size;
         array->pointer = g_realloc(array->pointer, new_size);
-        if (!array->pointer)
-            return -1;
+        assert(array->pointer);
         memset(array->pointer + array->size, 0, new_size - array->size);
         array->size = new_size;
         array->next = index + 1;
@@ -2261,6 +2261,9 @@ static mapping_t* insert_mapping(BDRVVVFATState* s,
     }
     if (index >= s->mapping.next || mapping->begin > begin) {
         mapping = array_insert(&(s->mapping), index, 1);
+        if (mapping == NULL) {
+            return NULL;
+        }
         mapping->path = NULL;
         adjust_mapping_indices(s, index, +1);
     }
@@ -2428,6 +2431,9 @@ static int commit_direntries(BDRVVVFATState* s,
     direntry_t* direntry = array_get(&(s->directory), dir_index);
     uint32_t first_cluster = dir_index == 0 ? 0 : begin_of_direntry(direntry);
     mapping_t* mapping = find_mapping_for_cluster(s, first_cluster);
+    if (mapping == NULL) {
+        return -1;
+    }
 
     int factor = 0x10 * s->sectors_per_cluster;
     int old_cluster_count, new_cluster_count;
@@ -2494,6 +2500,9 @@ DLOG(fprintf(stderr, "commit_direntries for %s, parent_mapping_index %d\n", mapp
         direntry = array_get(&(s->directory), first_dir_index + i);
         if (is_directory(direntry) && !is_dot(direntry)) {
             mapping = find_mapping_for_cluster(s, first_cluster);
+            if (mapping == NULL) {
+                return -1;
+            }
             assert(mapping->mode & MODE_DIRECTORY);
             ret = commit_direntries(s, first_dir_index + i,
                 array_index(&(s->mapping), mapping));
@@ -2522,6 +2531,10 @@ static int commit_one_file(BDRVVVFATState* s,
     assert(offset < size);
     assert((offset % s->cluster_size) == 0);
 
+    if (mapping == NULL) {
+        return -1;
+    }
+
     for (i = s->cluster_size; i < offset; i += s->cluster_size)
         c = modified_fat_get(s, c);
 
@@ -2668,8 +2681,12 @@ static int handle_renames_and_mkdirs(BDRVVVFATState* s)
         if (commit->action == ACTION_RENAME) {
             mapping_t* mapping = find_mapping_for_cluster(s,
                     commit->param.rename.cluster);
-            char* old_path = mapping->path;
+            char *old_path;
 
+            if (mapping == NULL) {
+                return -1;
+            }
+            old_path = mapping->path;
             assert(commit->path);
             mapping->path = commit->path;
             if (rename(old_path, mapping->path))
@@ -2690,10 +2707,15 @@ static int handle_renames_and_mkdirs(BDRVVVFATState* s)
                         direntry_t* d = direntry + i;
 
                         if (is_file(d) || (is_directory(d) && !is_dot(d))) {
+                            int l;
+                            char *new_path;
                             mapping_t* m = find_mapping_for_cluster(s,
                                     begin_of_direntry(d));
-                            int l = strlen(m->path);
-                            char* new_path = g_malloc(l + diff + 1);
+                            if (m == NULL) {
+                                return -1;
+                            }
+                            l = strlen(m->path);
+                            new_path = g_malloc(l + diff + 1);
 
                             assert(!strncmp(m->path, mapping->path, l2));
 
@@ -3193,6 +3215,7 @@ static int enable_write_target(BlockDriverState *bs, Error **errp)
 
     backing = bdrv_new_open_driver(&vvfat_write_target, NULL, BDRV_O_ALLOW_RDWR,
                                    &error_abort);
+    assert(backing);
     *(void**) backing->opaque = s;
 
     bdrv_set_backing_hd(s->bs, backing, &error_abort);
-- 
1.8.3.1

[Qemu-devel] [PATCH v4 6/8] block: dump_qlist() may dereference a Null pointer

[Liam Merwick]

A NULL 'list' passed into function dump_qlist() isn't correctly
validated and can be passed to qlist_first() where it is dereferenced.

Given that dump_qlist() is static, and callers already do the right
thing, just add an assert to catch future potential bugs (plus the
added benefit of suppressing a warning from a static analysis tool
and removing this noise will help us better find real issues).

Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/qapi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/qapi.c b/block/qapi.c
index c66f949db839..e81be604217c 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -740,6 +740,8 @@ static void dump_qlist(fprintf_function func_fprintf, void *f, int indentation,
     const QListEntry *entry;
     int i = 0;
 
+    assert(list);
+
     for (entry = qlist_first(list); entry; entry = qlist_next(entry), i++) {
         QType type = qobject_type(entry->value);
         bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
-- 
1.8.3.1

[Qemu-devel] [PATCH v4 7/8] qcow2: Read outside array bounds in qcow2_pre_write_overlap_check()

[Liam Merwick]

The commit for 0e4e4318eaa5 increments QCOW2_OL_MAX_BITNR but does not
add an array entry for QCOW2_OL_BITMAP_DIRECTORY_BITNR to metadata_ol_names[].
As a result, an array dereference of metadata_ol_names[8] in
qcow2_pre_write_overlap_check() could result in a read outside of the array bounds.

Fixes: 0e4e4318eaa5 ('qcow2: add overlap check for bitmap directory')

Cc: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-refcount.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 3c539f02e5ec..46082aeac1d6 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2719,15 +2719,17 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset,
 }
 
 static const char *metadata_ol_names[] = {
-    [QCOW2_OL_MAIN_HEADER_BITNR]    = "qcow2_header",
-    [QCOW2_OL_ACTIVE_L1_BITNR]      = "active L1 table",
-    [QCOW2_OL_ACTIVE_L2_BITNR]      = "active L2 table",
-    [QCOW2_OL_REFCOUNT_TABLE_BITNR] = "refcount table",
-    [QCOW2_OL_REFCOUNT_BLOCK_BITNR] = "refcount block",
-    [QCOW2_OL_SNAPSHOT_TABLE_BITNR] = "snapshot table",
-    [QCOW2_OL_INACTIVE_L1_BITNR]    = "inactive L1 table",
-    [QCOW2_OL_INACTIVE_L2_BITNR]    = "inactive L2 table",
+    [QCOW2_OL_MAIN_HEADER_BITNR]        = "qcow2_header",
+    [QCOW2_OL_ACTIVE_L1_BITNR]          = "active L1 table",
+    [QCOW2_OL_ACTIVE_L2_BITNR]          = "active L2 table",
+    [QCOW2_OL_REFCOUNT_TABLE_BITNR]     = "refcount table",
+    [QCOW2_OL_REFCOUNT_BLOCK_BITNR]     = "refcount block",
+    [QCOW2_OL_SNAPSHOT_TABLE_BITNR]     = "snapshot table",
+    [QCOW2_OL_INACTIVE_L1_BITNR]        = "inactive L1 table",
+    [QCOW2_OL_INACTIVE_L2_BITNR]        = "inactive L2 table",
+    [QCOW2_OL_BITMAP_DIRECTORY_BITNR]   = "bitmap directory",
 };
+QEMU_BUILD_BUG_ON(QCOW2_OL_MAX_BITNR != ARRAY_SIZE(metadata_ol_names));
 
 /*
  * First performs a check for metadata overlaps (through
-- 
1.8.3.1

[Qemu-devel] [PATCH v4 8/8] kvm: Potential NULL pointer dereference in kvm_arch_init_vcpu()

[Liam Merwick]

In kvm_arch_init_vcpu() a call to cpuid_find_entry() can return
NULL so the pointer returned should be checked before dereferencing it.

Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
---
 target/i386/kvm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index dc4047b02fc5..eb19c87a9d25 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -1177,7 +1177,9 @@ int kvm_arch_init_vcpu(CPUState *cs)
         c->ecx = c->edx = 0;
 
         c = cpuid_find_entry(&cpuid_data.cpuid, kvm_base, 0);
-        c->eax = MAX(c->eax, KVM_CPUID_SIGNATURE | 0x10);
+        if (c) {
+            c->eax = MAX(c->eax, KVM_CPUID_SIGNATURE | 0x10);
+       }
     }
 
     cpuid_data.cpuid.nent = cpuid_i;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v4 3/8] block: Null pointer dereference in blk_root_get_parent_desc()

[Max Reitz]

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

On 19.10.18 22:39, Liam Merwick wrote:
> The dev_id returned by the call to blk_get_attached_dev_id() in
> blk_root_get_parent_desc() can be NULL (an internal call to
> object_get_canonical_path may have returned NULL).
> 
> Instead of just checking this case before before dereferencing,
> adjust blk_get_attached_dev_id() to return the empty string if no
> object path can be found (similar to the case when blk->dev is NULL
> and an empty string is returned).
> 
> Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
> ---
>  block/block-backend.c | 6 +++++-
>  dtc                   | 2 +-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index dc0cd5772413..e628920f3cd8 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -909,6 +909,7 @@ void *blk_get_attached_dev(BlockBackend *blk)
>  char *blk_get_attached_dev_id(BlockBackend *blk)
>  {
>      DeviceState *dev;
> +    char *dev_id;
>  
>      assert(!blk->legacy_dev);
>      dev = blk->dev;
> @@ -918,7 +919,10 @@ char *blk_get_attached_dev_id(BlockBackend *blk)
>      } else if (dev->id) {
>          return g_strdup(dev->id);
>      }
> -    return object_get_canonical_path(OBJECT(dev));
> +
> +    dev_id = object_get_canonical_path(OBJECT(dev));
> +
> +    return dev_id ? dev_id : g_strdup("");
>  }
>  
>  /*

Looks good, but since you'll have to respin anyway because of the hunk
below, you may want to consider

    return object_get_canonical_path(OBJECT(dev)) ?: g_strdup("");

instead.  (We have several instances of binary "?:" in the code already,
so it's fine to use it.  Of course you don't have to, though, if you
don't like it.)

> diff --git a/dtc b/dtc
> index 88f18909db73..e54388015af1 160000
> --- a/dtc
> +++ b/dtc
> @@ -1 +1 @@
> -Subproject commit 88f18909db731a627456f26d779445f84e449536
> +Subproject commit e54388015af1fb4bf04d0bca99caba1074d9cc42

I don't think this hunk belongs here.

Max


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

Re: [Qemu-devel] [PATCH v4 4/8] qemu-img: assert block_job_get() does not return NULL in img_commit()

[Max Reitz]

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

On 19.10.18 22:39, Liam Merwick wrote:
> Although the function block_job_get() can return NULL, it would be a
> serious bug if it did so (because the job yields before executing anything
> (if it started successfully); but otherwise, commit_active_start() would
> have returned an error).  However, as a precaution, before dereferencing
> the 'job' pointer in img_commit() assert it is not NULL.

(In the meantime, Markus has argued to me in some other case that
asserting that something isn't NULL is just as good as just
dereferencing it.  Oh well, I still don't mind either way.)

> Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
> ---
>  qemu-img.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH v4 6/8] block: dump_qlist() may dereference a Null pointer

[Max Reitz]

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

On 19.10.18 22:39, Liam Merwick wrote:
> A NULL 'list' passed into function dump_qlist() isn't correctly
> validated and can be passed to qlist_first() where it is dereferenced.
> 
> Given that dump_qlist() is static, and callers already do the right
> thing, just add an assert to catch future potential bugs (plus the
> added benefit of suppressing a warning from a static analysis tool
> and removing this noise will help us better find real issues).

But can't you fix the tool?  My opinion is still that large parts of our
code do not assert that some parameter is not NULL, and I think it isn't
a good idea to make them assert that.  I don't know what makes this
function special, and I wonder why it is special to your tool -- as I've
said in the last version, dump_qdict() is basically the same in this
regard.  I wonder why your tool doesn't mind that.

Can you not whitelist something as false positives?  I know we have a
lot of those in Coverity, and we just mark them as such, and that's it.

Finally, one could argue that the nonnull GCC function attribute would
be a better fit, actually.

But overall, I just don't think it's a good idea to start changing the
code to accommodate for false positives in static analyzers, because in
my experience the number of false positives only rises with time.

Max

> Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block/qapi.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/block/qapi.c b/block/qapi.c
> index c66f949db839..e81be604217c 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -740,6 +740,8 @@ static void dump_qlist(fprintf_function func_fprintf, void *f, int indentation,
>      const QListEntry *entry;
>      int i = 0;
>  
> +    assert(list);
> +
>      for (entry = qlist_first(list); entry; entry = qlist_next(entry), i++) {
>          QType type = qobject_type(entry->value);
>          bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
> 



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

Re: [Qemu-devel] [PATCH v4 5/8] block: Fix potential Null pointer dereferences in vvfat.c

[Max Reitz]

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

On 19.10.18 22:39, Liam Merwick wrote:
> The calls to find_mapping_for_cluster() may return NULL but it
> isn't always checked for before dereferencing the value returned.
> Additionally, add some asserts to cover cases where NULL can't
> be returned but which might not be obvious at first glance.
> 
> Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
> ---
>  block/vvfat.c | 33 ++++++++++++++++++++++++++++-----
>  1 file changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/block/vvfat.c b/block/vvfat.c
> index fc41841a5c3c..19f6725054a0 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -100,6 +100,7 @@ static inline void array_free(array_t* array)
>  /* does not automatically grow */
>  static inline void* array_get(array_t* array,unsigned int index) {
>      assert(index < array->next);
> +    assert(array->pointer);
>      return array->pointer + index * array->item_size;
>  }
>  
> @@ -108,8 +109,7 @@ static inline int array_ensure_allocated(array_t* array, int index)
>      if((index + 1) * array->item_size > array->size) {
>          int new_size = (index + 32) * array->item_size;
>          array->pointer = g_realloc(array->pointer, new_size);
> -        if (!array->pointer)
> -            return -1;
> +        assert(array->pointer);

It would make sense to make this function not return any value (because
it just always returns 0 now), but I fully understand if you don't want
to mess around with vvfat more than you have to.  (Neither do I.)

>          memset(array->pointer + array->size, 0, new_size - array->size);
>          array->size = new_size;
>          array->next = index + 1;
> @@ -2261,6 +2261,9 @@ static mapping_t* insert_mapping(BDRVVVFATState* s,
>      }
>      if (index >= s->mapping.next || mapping->begin > begin) {
>          mapping = array_insert(&(s->mapping), index, 1);
> +        if (mapping == NULL) {
> +            return NULL;
> +        }

array_insert() will never return NULL.

>          mapping->path = NULL;
>          adjust_mapping_indices(s, index, +1);
>      }
> @@ -2428,6 +2431,9 @@ static int commit_direntries(BDRVVVFATState* s,
>      direntry_t* direntry = array_get(&(s->directory), dir_index);
>      uint32_t first_cluster = dir_index == 0 ? 0 : begin_of_direntry(direntry);
>      mapping_t* mapping = find_mapping_for_cluster(s, first_cluster);
> +    if (mapping == NULL) {
> +        return -1;
> +    }

This should be moved below the declarations that still follow here.

>  
>      int factor = 0x10 * s->sectors_per_cluster;
>      int old_cluster_count, new_cluster_count;

[...]

> @@ -3193,6 +3215,7 @@ static int enable_write_target(BlockDriverState *bs, Error **errp)
>  
>      backing = bdrv_new_open_driver(&vvfat_write_target, NULL, BDRV_O_ALLOW_RDWR,
>                                     &error_abort);
> +    assert(backing);
>      *(void**) backing->opaque = s;

I personally wouldn't use an assert() here because it's clear that the
value is dereferenced immediately, so that is the assertion that it is
non-NULL, but I won't give too much of a fight.

The thing is, I believe we should write code for humans, not machines.
Fixing machines to understand what we produce is possible -- fixing
humans is more difficult.

On top of that, it would be a bug if NULL is returned and it would be
good if a static analyzer could catch that case.  Just fully silencing
it with assert() is not ideal.  Ideal would be if it would know that
setting &error_abort to any value crashes the program, and could thus
infer whether this function will actually ever get to return NULL when
&error_abort has been passed to it.

Max

>  
>      bdrv_set_backing_hd(s->bs, backing, &error_abort);
> 



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

Re: [Qemu-devel] [PATCH v4 3/8] block: Null pointer dereference in blk_root_get_parent_desc()

[Liam Merwick]

On 04/11/18 23:57, Max Reitz wrote:
> On 19.10.18 22:39, Liam Merwick wrote:
>> The dev_id returned by the call to blk_get_attached_dev_id() in
>> blk_root_get_parent_desc() can be NULL (an internal call to
>> object_get_canonical_path may have returned NULL).
>>
>> Instead of just checking this case before before dereferencing,
>> adjust blk_get_attached_dev_id() to return the empty string if no
>> object path can be found (similar to the case when blk->dev is NULL
>> and an empty string is returned).
>>
>> Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
>> ---
>>   block/block-backend.c | 6 +++++-
>>   dtc                   | 2 +-
>>   2 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index dc0cd5772413..e628920f3cd8 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -909,6 +909,7 @@ void *blk_get_attached_dev(BlockBackend *blk)
>>   char *blk_get_attached_dev_id(BlockBackend *blk)
>>   {
>>       DeviceState *dev;
>> +    char *dev_id;
>>   
>>       assert(!blk->legacy_dev);
>>       dev = blk->dev;
>> @@ -918,7 +919,10 @@ char *blk_get_attached_dev_id(BlockBackend *blk)
>>       } else if (dev->id) {
>>           return g_strdup(dev->id);
>>       }
>> -    return object_get_canonical_path(OBJECT(dev));
>> +
>> +    dev_id = object_get_canonical_path(OBJECT(dev));
>> +
>> +    return dev_id ? dev_id : g_strdup("");
>>   }
>>   
>>   /*
> 
> Looks good, but since you'll have to respin anyway because of the hunk
> below, you may want to consider
> 
>      return object_get_canonical_path(OBJECT(dev)) ?: g_strdup("");
> 
> instead.  (We have several instances of binary "?:" in the code already,
> so it's fine to use it.  Of course you don't have to, though, if you
> don't like it.)
> 

I like it. Will make that change in v5


>> diff --git a/dtc b/dtc
>> index 88f18909db73..e54388015af1 160000
>> --- a/dtc
>> +++ b/dtc
>> @@ -1 +1 @@
>> -Subproject commit 88f18909db731a627456f26d779445f84e449536
>> +Subproject commit e54388015af1fb4bf04d0bca99caba1074d9cc42
> 
> I don't think this hunk belongs here.


Indeed.

Regards,
Liam

Re: [Qemu-devel] [PATCH v4 5/8] block: Fix potential Null pointer dereferences in vvfat.c

[Liam Merwick]

On 05/11/18 00:19, Max Reitz wrote:
> On 19.10.18 22:39, Liam Merwick wrote:
>> The calls to find_mapping_for_cluster() may return NULL but it
>> isn't always checked for before dereferencing the value returned.
>> Additionally, add some asserts to cover cases where NULL can't
>> be returned but which might not be obvious at first glance.
>>
>> Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
>> ---
>>   block/vvfat.c | 33 ++++++++++++++++++++++++++++-----
>>   1 file changed, 28 insertions(+), 5 deletions(-)
>>
>> diff --git a/block/vvfat.c b/block/vvfat.c
>> index fc41841a5c3c..19f6725054a0 100644
>> --- a/block/vvfat.c
>> +++ b/block/vvfat.c
>> @@ -100,6 +100,7 @@ static inline void array_free(array_t* array)
>>   /* does not automatically grow */
>>   static inline void* array_get(array_t* array,unsigned int index) {
>>       assert(index < array->next);
>> +    assert(array->pointer);
>>       return array->pointer + index * array->item_size;
>>   }
>>   
>> @@ -108,8 +109,7 @@ static inline int array_ensure_allocated(array_t* array, int index)
>>       if((index + 1) * array->item_size > array->size) {
>>           int new_size = (index + 32) * array->item_size;
>>           array->pointer = g_realloc(array->pointer, new_size);
>> -        if (!array->pointer)
>> -            return -1;
>> +        assert(array->pointer);
> 
> It would make sense to make this function not return any value (because
> it just always returns 0 now), but I fully understand if you don't want
> to mess around with vvfat more than you have to.  (Neither do I.)

It had occurred to me too but wasn't sure if it'd be preferred to roll 
that change in. 3 of the 4 callers ignored the return value already, so 
I bit the bullet and made the change.


> 
>>           memset(array->pointer + array->size, 0, new_size - array->size);
>>           array->size = new_size;
>>           array->next = index + 1;
>> @@ -2261,6 +2261,9 @@ static mapping_t* insert_mapping(BDRVVVFATState* s,
>>       }
>>       if (index >= s->mapping.next || mapping->begin > begin) {
>>           mapping = array_insert(&(s->mapping), index, 1);
>> +        if (mapping == NULL) {
>> +            return NULL;
>> +        }
> 
> array_insert() will never return NULL.


Removed.

> 
>>           mapping->path = NULL;
>>           adjust_mapping_indices(s, index, +1);
>>       }
>> @@ -2428,6 +2431,9 @@ static int commit_direntries(BDRVVVFATState* s,
>>       direntry_t* direntry = array_get(&(s->directory), dir_index);
>>       uint32_t first_cluster = dir_index == 0 ? 0 : begin_of_direntry(direntry);
>>       mapping_t* mapping = find_mapping_for_cluster(s, first_cluster);
>> +    if (mapping == NULL) {
>> +        return -1;
>> +    }
> 
> This should be moved below the declarations that still follow here.

Done. (It resulted in a bit more code rearranging and I had to fix two 
checkpatch warnings in existing code)

> 
>>   
>>       int factor = 0x10 * s->sectors_per_cluster;
>>       int old_cluster_count, new_cluster_count;
> 
> [...]
> 
>> @@ -3193,6 +3215,7 @@ static int enable_write_target(BlockDriverState *bs, Error **errp)
>>   
>>       backing = bdrv_new_open_driver(&vvfat_write_target, NULL, BDRV_O_ALLOW_RDWR,
>>                                      &error_abort);
>> +    assert(backing);
>>       *(void**) backing->opaque = s;
> 
> I personally wouldn't use an assert() here because it's clear that the
> value is dereferenced immediately, so that is the assertion that it is
> non-NULL, but I won't give too much of a fight.
> 
> The thing is, I believe we should write code for humans, not machines.
> Fixing machines to understand what we produce is possible -- fixing
> humans is more difficult.
> 
> On top of that, it would be a bug if NULL is returned and it would be
> good if a static analyzer could catch that case.  Just fully silencing
> it with assert() is not ideal.  Ideal would be if it would know that
> setting &error_abort to any value crashes the program, and could thus
> infer whether this function will actually ever get to return NULL when
> &error_abort has been passed to it.
> 

I'm investigating if the tool's config file syntax can describe that 
error_handle_fatal() exits when particular error_xxx parameters are passed.

I'll drop that assert in any case.

Regards,
Liam


> Max
> 
>>   
>>       bdrv_set_backing_hd(s->bs, backing, &error_abort);
>>
> 
> 

Re: [Qemu-devel] [PATCH v4 6/8] block: dump_qlist() may dereference a Null pointer

[Liam Merwick]

On 05/11/18 00:07, Max Reitz wrote:
> On 19.10.18 22:39, Liam Merwick wrote:
>> A NULL 'list' passed into function dump_qlist() isn't correctly
>> validated and can be passed to qlist_first() where it is dereferenced.
>>
>> Given that dump_qlist() is static, and callers already do the right
>> thing, just add an assert to catch future potential bugs (plus the
>> added benefit of suppressing a warning from a static analysis tool
>> and removing this noise will help us better find real issues).
> 
> But can't you fix the tool? 

I don't have access to the tool source but have been filing bugs against 
it as I run it on the QEMU codebase and discover false positives.

> My opinion is still that large parts of our
> code do not assert that some parameter is not NULL, and I think it isn't
> a good idea to make them assert that.  

Yeah, that can be a slippery slope....

> I don't know what makes this
> function special, and I wonder why it is special to your tool -- as I've
> said in the last version, dump_qdict() is basically the same in this
> regard.  I wonder why your tool doesn't mind that.
> 

I had gone though the code paths to try to see how the tool was happy 
with one and not the other - the implementation differed slightly w.r.t 
macro usage but I couldn't see any obvious reason.

> Can you not whitelist something as false positives?  I know we have a
> lot of those in Coverity, and we just mark them as such, and that's it.

Yeah, I can flag this as a FP and have it fall off my list.

I'll will drop this patch in v5

Regards,
Liam

> 
> Finally, one could argue that the nonnull GCC function attribute would
> be a better fit, actually.
> 
> But overall, I just don't think it's a good idea to start changing the
> code to accommodate for false positives in static analyzers, because in
> my experience the number of false positives only rises with time.
> 
> Max
> 
>> Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>   block/qapi.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/block/qapi.c b/block/qapi.c
>> index c66f949db839..e81be604217c 100644
>> --- a/block/qapi.c
>> +++ b/block/qapi.c
>> @@ -740,6 +740,8 @@ static void dump_qlist(fprintf_function func_fprintf, void *f, int indentation,
>>       const QListEntry *entry;
>>       int i = 0;
>>   
>> +    assert(list);
>> +
>>       for (entry = qlist_first(list); entry; entry = qlist_next(entry), i++) {
>>           QType type = qobject_type(entry->value);
>>           bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
>>
> 
>