qemu-commits
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Qemu-commits] [qemu/qemu] ba4dba: json-streamer: Don't leak tokens on i


From: GitHub
Subject: [Qemu-commits] [qemu/qemu] ba4dba: json-streamer: Don't leak tokens on incomplete par...
Date: Fri, 01 Jul 2016 04:00:04 -0700

  Branch: refs/heads/master
  Home:   https://github.com/qemu/qemu
  Commit: ba4dba54347d5062436a8553f527dbbed6dcf069
      
https://github.com/qemu/qemu/commit/ba4dba54347d5062436a8553f527dbbed6dcf069
  Author: Eric Blake <address@hidden>
  Date:   2016-06-30 (Thu, 30 Jun 2016)

  Changed paths:
    M qobject/json-streamer.c

  Log Message:
  -----------
  json-streamer: Don't leak tokens on incomplete parse

Valgrind complained about a number of leaks in
tests/check-qobject-json:

==12657==    definitely lost: 17,247 bytes in 1,234 blocks

All of which had the same root cause: on an incomplete parse,
we were abandoning the token queue without cleaning up the
allocated data within each queue element.  Introduced in
commit 95385fe, when we switched from QList (which recursively
frees contents) to g_queue (which does not).

We don't yet require glib 2.32 with its g_queue_free_full(),
so open-code it instead.

CC: address@hidden
Signed-off-by: Eric Blake <address@hidden>
Message-Id: <address@hidden>
Reviewed-by: Markus Armbruster <address@hidden>
Signed-off-by: Markus Armbruster <address@hidden>


  Commit: ff5394ad5b1039efef9844f5844f952aec93ef37
      
https://github.com/qemu/qemu/commit/ff5394ad5b1039efef9844f5844f952aec93ef37
  Author: Eric Blake <address@hidden>
  Date:   2016-06-30 (Thu, 30 Jun 2016)

  Changed paths:
    M qobject/json-lexer.c

  Log Message:
  -----------
  qobject: Correct JSON lexer grammar comments

Fix the regex comments describing what we parse as JSON.  No change
to the lexer itself, just to the comments:
- The "" and '' string construction was missing alternation between
different escape sequences
- The construction for numbers forgot to handle optional leading '-'
- The construction for numbers was grouped incorrectly so that it
didn't permit '0.1'
- The construction for numbers forgot to mark the exponent as optional
- No mention that our '' string and "\'" are JSON extensions
- No mention of our %d and related extensions when constructing JSON

Signed-off-by: Eric Blake <address@hidden>
Message-Id: <address@hidden>
Reviewed-by: Markus Armbruster <address@hidden>
[Eric's regexp simplification squashed in]
Signed-off-by: Markus Armbruster <address@hidden>


  Commit: 01fb8e192d2cb139622df22983360836e39a64ff
      
https://github.com/qemu/qemu/commit/01fb8e192d2cb139622df22983360836e39a64ff
  Author: Eric Blake <address@hidden>
  Date:   2016-06-30 (Thu, 30 Jun 2016)

  Changed paths:
    M scripts/checkpatch.pl

  Log Message:
  -----------
  checkpatch: There is no qemu_strtod()

Maybe there should be; but until there is, we should not flag
strtod() calls as something to replaced with qemu_strtod().

We also lack qemu_strtof() and qemu_strtold(), but as no one
has been using strtof() or strtold(), it's not worth complicating
the regex for them.

(Ironically, I had to use 'git commit -n' since checkpatch uses
TAB indents, in violation of its own recommendations.)

Signed-off-by: Eric Blake <address@hidden>
Message-Id: <address@hidden>
Reviewed-by: Markus Armbruster <address@hidden>
Signed-off-by: Markus Armbruster <address@hidden>


  Commit: 9b4e38fe6a35890bb1d995316d7be08de0b30ee5
      
https://github.com/qemu/qemu/commit/9b4e38fe6a35890bb1d995316d7be08de0b30ee5
  Author: Eric Blake <address@hidden>
  Date:   2016-06-30 (Thu, 30 Jun 2016)

  Changed paths:
    M scripts/qapi-visit.py
    M tests/test-qmp-input-visitor.c

  Log Message:
  -----------
  qapi: Fix crash on missing alternate member of QAPI struct

If a QAPI struct has a mandatory alternate member which is not
present on input, the input visitor reports an error for the
missing alternate without setting the discriminator, but the
cleanup code for the struct still tries to use the dealloc
visitor to clean up the alternate.

Commit dbf11922 changed visit_start_alternate to set *obj to NULL
when an error occurs, where it was previously left untouched.
Thus, before the patch, the dealloc visitor is blindly trying to
cleanup whatever branch corresponds to (*obj)->type == 0 (that is,
QTYPE_NONE, because *obj still pointed to zeroed memory), which
selects the default branch of the switch and sets an error, but
this second error is ignored by the way the dealloc visitor is
used; but after the patch, the attempt to switch dereferences NULL.

When cleaning up after a partial object parse, we specifically
check for !*obj after visit_start_struct() (see gen_visit_object());
doing the same for alternates fixes the crash. Enhance the testsuite
to give coverage for both missing struct and missing alternate
members.

Also add an abort - we expect visit_start_alternate() to either set an
error or to set (*obj)->type to a valid QType that corresponds to
actual user input, and QTYPE_NONE should never be reachable from valid
input.  Had the abort() been in place earlier, we might have noticed
the dealloc visitor dereferencing bogus zeroed memory prior to when
commit dbf11922 forced our hand by setting *obj to NULL and causing a
fault.

Test case:

{'execute':'blockdev-add', 'arguments':{'options':{'driver':'raw'}}}

The choice of 'driver':'raw' selects a BlockdevOptionsGenericFormat
struct, which has a mandatory 'file':'BlockdevRef' in QAPI.  Since
'file' is missing as a sibling of 'driver', this should report a
graceful error rather than fault.  After this patch, we are back to:

{"error": {"class": "GenericError", "desc": "Parameter 'file' is missing"}}

Generated code in qapi-visit.c changes as:

|@@ -2444,6 +2444,9 @@ void visit_type_BlockdevRef(Visitor *v,
|     if (err) {
|         goto out;
|     }
|+    if (!*obj) {
|+        goto out_obj;
|+    }
|     switch ((*obj)->type) {
|     case QTYPE_QDICT:
|         visit_start_struct(v, name, NULL, 0, &err);
|@@ -2459,10 +2462,13 @@ void visit_type_BlockdevRef(Visitor *v,
|     case QTYPE_QSTRING:
|         visit_type_str(v, name, &(*obj)->u.reference, &err);
|         break;
|+    case QTYPE_NONE:
|+        abort();
|     default:
|         error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
|                    "BlockdevRef");
|     }
|+out_obj:
|     visit_end_alternate(v);

Reported by Kashyap Chamarthy <address@hidden>
CC: address@hidden
Signed-off-by: Eric Blake <address@hidden>
Message-Id: <address@hidden>
Reviewed-by: Markus Armbruster <address@hidden>
Tested-by: Kashyap Chamarthy <address@hidden>
[Commit message tweaked]
Signed-off-by: Markus Armbruster <address@hidden>


  Commit: fec0fc0a13ac7f1a1130433a6740cd850c3db34a
      
https://github.com/qemu/qemu/commit/fec0fc0a13ac7f1a1130433a6740cd850c3db34a
  Author: Eric Blake <address@hidden>
  Date:   2016-06-30 (Thu, 30 Jun 2016)

  Changed paths:
    M include/qemu/range.h
    M util/Makefile.objs
    A util/range.c

  Log Message:
  -----------
  range: Create range.c for code that should not be inline

g_list_insert_sorted_merged() is rather large to be an inline
function; move it to its own file.  range_merge() and
ranges_can_merge() can likewise move, as they are only used
internally.  Also, it becomes obvious that the condition within
range_merge() is already satisfied by its caller, and that the
return value is not used.

The diffstat is misleading, because of the copyright boilerplate.

Signed-off-by: Eric Blake <address@hidden>
Message-Id: <address@hidden>
Signed-off-by: Markus Armbruster <address@hidden>


  Commit: 7c47959d0cb05db43014141a156ada0b6d53a750
      
https://github.com/qemu/qemu/commit/7c47959d0cb05db43014141a156ada0b6d53a750
  Author: Eric Blake <address@hidden>
  Date:   2016-06-30 (Thu, 30 Jun 2016)

  Changed paths:
    M include/qemu/range.h
    M qapi/string-input-visitor.c
    M qapi/string-output-visitor.c
    M util/range.c

  Log Message:
  -----------
  qapi: Simplify use of range.h

Calling our function g_list_insert_sorted_merged is a misnomer,
since we are NOT writing a glib function.  Furthermore, we are
making every caller pass the same comparator function of
range_merge(): any caller that would try otherwise would break
in weird ways since our internal call to ranges_can_merge() is
hard-coded to operate only on ranges, rather than paying
attention to the caller's comparator.

Better is to fix things so that callers don't have to care about
our internal comparator, by picking a function name and updating
the parameter type away from a gratuitous use of void*, to make
it obvious that we are operating specifically on a list of ranges
and not a generic list.  Plus, refactoring the code here will
make it easier to plug a memory leak in the next patch.

range_compare() is now internal only, and moves to the .c file.

Signed-off-by: Eric Blake <address@hidden>
Message-Id: <address@hidden>
Signed-off-by: Markus Armbruster <address@hidden>


  Commit: db486cc334aafd3dbdaf107388e37fc3d6d3e171
      
https://github.com/qemu/qemu/commit/db486cc334aafd3dbdaf107388e37fc3d6d3e171
  Author: Eric Blake <address@hidden>
  Date:   2016-06-30 (Thu, 30 Jun 2016)

  Changed paths:
    M util/range.c

  Log Message:
  -----------
  qapi: Fix memleak in string visitors on int lists

Commit 7f8f9ef1 introduced the ability to store a list of
integers as a sorted list of ranges, but when merging ranges,
it leaks one or more ranges.  It was also using range_get_last()
incorrectly within range_compare() (a range is a start/end pair,
but range_get_last() is for start/len pairs), and will also
mishandle a range ending in UINT64_MAX (remember, we document
that no range covers 2**64 bytes, but that ranges that end on
UINT64_MAX have end < begin).

The whole merge algorithm was rather complex, and included
unnecessary passes over data within glib functions, and enough
indirection to make it hard to easily plug the data leaks.
Since we are already hard-coding things to a list of ranges,
just rewrite the thing to open-code the traversal and
comparisons, by making the range_compare() helper function give
us an answer that is easier to use, at which point we avoid the
need to pass any callbacks to g_list_*(). Then by reusing
range_extend() instead of duplicating effort with range_merge(),
we cover the corner cases correctly.

Drop the now-unused range_merge() and ranges_can_merge().

Doing this lets test-string-{input,output}-visitor pass under
valgrind without leaks.

Signed-off-by: Eric Blake <address@hidden>
Message-Id: <address@hidden>
Reviewed-by: Markus Armbruster <address@hidden>
[Comment hoisted out of loop]
Signed-off-by: Markus Armbruster <address@hidden>


  Commit: 1fb4c13e4f92bda047f30177d5b8c2b4ca42e8b8
      
https://github.com/qemu/qemu/commit/1fb4c13e4f92bda047f30177d5b8c2b4ca42e8b8
  Author: Peter Maydell <address@hidden>
  Date:   2016-07-01 (Fri, 01 Jul 2016)

  Changed paths:
    M include/qemu/range.h
    M qapi/string-input-visitor.c
    M qapi/string-output-visitor.c
    M qobject/json-lexer.c
    M qobject/json-streamer.c
    M scripts/checkpatch.pl
    M scripts/qapi-visit.py
    M tests/test-qmp-input-visitor.c
    M util/Makefile.objs
    A util/range.c

  Log Message:
  -----------
  Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2016-06-30' into 
staging

QAPI patches 2016-06-30

# gpg: Signature made Thu 30 Jun 2016 14:29:43 BST
# gpg:                using RSA key 0x3870B400EB918653
# gpg: Good signature from "Markus Armbruster <address@hidden>"
# gpg:                 aka "Markus Armbruster <address@hidden>"
# Primary key fingerprint: 354B C8B3 D7EB 2A6B 6867  4E5F 3870 B400 EB91 8653

* remotes/armbru/tags/pull-qapi-2016-06-30:
  qapi: Fix memleak in string visitors on int lists
  qapi: Simplify use of range.h
  range: Create range.c for code that should not be inline
  qapi: Fix crash on missing alternate member of QAPI struct
  checkpatch: There is no qemu_strtod()
  qobject: Correct JSON lexer grammar comments
  json-streamer: Don't leak tokens on incomplete parse

Signed-off-by: Peter Maydell <address@hidden>


Compare: https://github.com/qemu/qemu/compare/ddf31aa853a3...1fb4c13e4f92

reply via email to

[Prev in Thread] Current Thread [Next in Thread]