From: Markus Armbruster <armbru@redhat.com>
To: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Cc: den@openvz.org, vsementsov@virtuozzo.com, qemu-devel@nongnu.org,
mdroth@linux.vnet.ibm.com
Subject: Re: [Qemu-devel] [PATCH v2] make check-unit: use after free in test-opts-visitor
Date: Fri, 02 Aug 2019 13:34:49 +0200 [thread overview]
Message-ID: <87imrfkdgm.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <1564684930-107089-1-git-send-email-andrey.shinkevich@virtuozzo.com> (Andrey Shinkevich's message of "Thu, 1 Aug 2019 21:42:10 +0300")
Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> writes:
> In struct OptsVisitor, repeated_opts member points to a list in the
> unprocessed_opts hash table after the list has been destroyed. A
> subsequent call to visit_type_int() references the deleted list. It
> results in use-after-free issue.
Let's mention the reproducer: valgrind tests/test/opts-visitor.
> Also, the Visitor object call back
> functions are supposed to set the Error parameter in case of failure.
As far as I can tell, they all do. The only place where you set an
error is the new failure you add to lookup_scalar().
> A new mode ListMode::LM_TRAVERSED is declared to mark the list
> traversal completed.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>
> v2:
> 01: A new mode LM_TRAVERSED has been introduced to check instead of the
> repeated_opts pointer for NULL.
>
> qapi/opts-visitor.c | 78 +++++++++++++++++++++++++++++++++--------------------
> 1 file changed, 49 insertions(+), 29 deletions(-)
>
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index 324b197..23ac383 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -22,33 +22,42 @@
>
> enum ListMode
> {
> - LM_NONE, /* not traversing a list of repeated options */
> -
> - LM_IN_PROGRESS, /* opts_next_list() ready to be called.
> - *
> - * Generating the next list link will consume the most
> - * recently parsed QemuOpt instance of the repeated
> - * option.
> - *
> - * Parsing a value into the list link will examine the
> - * next QemuOpt instance of the repeated option, and
> - * possibly enter LM_SIGNED_INTERVAL or
> - * LM_UNSIGNED_INTERVAL.
> - */
> -
> - LM_SIGNED_INTERVAL, /* opts_next_list() has been called.
> - *
> - * Generating the next list link will consume the most
> - * recently stored element from the signed interval,
> - * parsed from the most recent QemuOpt instance of the
> - * repeated option. This may consume QemuOpt itself
> - * and return to LM_IN_PROGRESS.
> - *
> - * Parsing a value into the list link will store the
> - * next element of the signed interval.
> - */
> -
> - LM_UNSIGNED_INTERVAL /* Same as above, only for an unsigned interval. */
> + LM_NONE, /* not traversing a list of repeated options */
> +
> + LM_IN_PROGRESS, /*
> + * opts_next_list() ready to be called.
> + *
> + * Generating the next list link will consume the most
> + * recently parsed QemuOpt instance of the repeated
> + * option.
> + *
> + * Parsing a value into the list link will examine the
> + * next QemuOpt instance of the repeated option, and
> + * possibly enter LM_SIGNED_INTERVAL or
> + * LM_UNSIGNED_INTERVAL.
> + */
> +
> + LM_SIGNED_INTERVAL, /*
> + * opts_next_list() has been called.
> + *
> + * Generating the next list link will consume the most
> + * recently stored element from the signed interval,
> + * parsed from the most recent QemuOpt instance of the
> + * repeated option. This may consume QemuOpt itself
> + * and return to LM_IN_PROGRESS.
> + *
> + * Parsing a value into the list link will store the
> + * next element of the signed interval.
> + */
> +
> + LM_UNSIGNED_INTERVAL, /* Same as above, only for an unsigned interval. */
> +
> + LM_TRAVERSED /*
> + * opts_next_list() has been called.
> + *
> + * No more QemuOpt instance in the list.
> + * The traversal has been completed.
> + */
> };
>
> typedef enum ListMode ListMode;
Please don't change the spacing without need. The hunk should look like
this:
@@ -24,7 +24,8 @@ enum ListMode
{
LM_NONE, /* not traversing a list of repeated options */
- LM_IN_PROGRESS, /* opts_next_list() ready to be called.
+ LM_IN_PROGRESS, /*
+ * opts_next_list() ready to be called.
*
* Generating the next list link will consume the most
* recently parsed QemuOpt instance of the repeated
@@ -36,7 +37,8 @@ enum ListMode
* LM_UNSIGNED_INTERVAL.
*/
- LM_SIGNED_INTERVAL, /* opts_next_list() has been called.
+ LM_SIGNED_INTERVAL, /*
+ * opts_next_list() has been called.
*
* Generating the next list link will consume the most
* recently stored element from the signed interval,
@@ -48,7 +50,14 @@ enum ListMode
* next element of the signed interval.
*/
- LM_UNSIGNED_INTERVAL /* Same as above, only for an unsigned interval. */
+ LM_UNSIGNED_INTERVAL, /* Same as above, only for an unsigned interval. */
+
+ LM_TRAVERSED /*
+ * opts_next_list() has been called.
+ *
+ * No more QemuOpt instance in the list.
+ * The traversal has been completed.
+ */
};
typedef enum ListMode ListMode;
> @@ -238,6 +247,8 @@ opts_next_list(Visitor *v, GenericList *tail, size_t size)
> OptsVisitor *ov = to_ov(v);
>
> switch (ov->list_mode) {
> + case LM_TRAVERSED:
> + return NULL;
> case LM_SIGNED_INTERVAL:
> case LM_UNSIGNED_INTERVAL:
> if (ov->list_mode == LM_SIGNED_INTERVAL) {
> @@ -258,6 +269,8 @@ opts_next_list(Visitor *v, GenericList *tail, size_t size)
> opt = g_queue_pop_head(ov->repeated_opts);
> if (g_queue_is_empty(ov->repeated_opts)) {
> g_hash_table_remove(ov->unprocessed_opts, opt->name);
> + ov->repeated_opts = NULL;
> + ov->list_mode = LM_TRAVERSED;
> return NULL;
> }
> break;
> @@ -289,8 +302,11 @@ opts_end_list(Visitor *v, void **obj)
>
> assert(ov->list_mode == LM_IN_PROGRESS ||
> ov->list_mode == LM_SIGNED_INTERVAL ||
> - ov->list_mode == LM_UNSIGNED_INTERVAL);
> - ov->repeated_opts = NULL;
> + ov->list_mode == LM_UNSIGNED_INTERVAL ||
> + ov->list_mode == LM_TRAVERSED);
> + if (ov->list_mode != LM_TRAVERSED) {
> + ov->repeated_opts = NULL;
> + }
What's wrong with zapping ov->repeated_opts unconditionally?
> ov->list_mode = LM_NONE;
> }
>
> @@ -306,6 +322,10 @@ lookup_scalar(const OptsVisitor *ov, const char *name, Error **errp)
> list = lookup_distinct(ov, name, errp);
> return list ? g_queue_peek_tail(list) : NULL;
> }
> + if (ov->list_mode == LM_TRAVERSED) {
> + error_setg(errp, QERR_INVALID_PARAMETER, name);
Beware, @name is null when visiting list members. The test still passes
for me, since g_strdup_vprintf() formats a null argument to %s as
"(null)".
For what it's worth, the qobject input visitor uses
QERR_MISSING_PARAMETER with a made-up name. Computing the name is
pretty elaborate, see full_name_nth(). I'd rather not duplicate that
here.
Suggest something like
error_setg(errp, "Fewer list elements than expected");
The error message fails to mention the name of the list. Bad, but the
error is a corner case; we don't normally visit beyond the end of the
list. For a better message, we'd have to have start_list() store its
@name in struct OptsVisitor. I'm not asking you to do that now.
> + return NULL;
> + }
> assert(ov->list_mode == LM_IN_PROGRESS);
> return g_queue_peek_head(ov->repeated_opts);
> }
I checked the remaining uses of ->list_mode, and I think they are okay.
next prev parent reply other threads:[~2019-08-02 11:35 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-01 18:42 [Qemu-devel] [PATCH v2] make check-unit: use after free in test-opts-visitor Andrey Shinkevich
2019-08-02 11:34 ` Markus Armbruster [this message]
2019-08-05 15:09 ` Andrey Shinkevich
2019-08-06 5:25 ` Markus Armbruster
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87imrfkdgm.fsf@dusky.pond.sub.org \
--to=armbru@redhat.com \
--cc=andrey.shinkevich@virtuozzo.com \
--cc=den@openvz.org \
--cc=mdroth@linux.vnet.ibm.com \
--cc=qemu-devel@nongnu.org \
--cc=vsementsov@virtuozzo.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.