Re: [Qemu-devel] [PATCH v2 59/60] json: Improve safety of qobject_from_jsonf_nofail() & friends

[Eric Blake]

On 08/17/2018 10:05 AM, Markus Armbruster wrote:
> The JSON parser optionally supports interpolation.  This is used to
> build QObjects by parsing string templates.  The templates are C
> literals, so parse errors (such as invalid interpolation
> specifications) are actually programming errors.  Consequently, the
> functions providing parsing with interpolation
> (qobject_from_jsonf_nofail(), qobject_from_vjsonf_nofail(),
> qdict_from_jsonf_nofail(), qdict_from_vjsonf_nofail()) pass
> &error_abort to the parser.

> Avoid undefined behavior by catching the programming error at run
> time: have the parser recognize and reject '%' in JSON strings.
>
> Signed-off-by: Markus Armbruster <address@hidden>
> ---

> @@ -205,7 +206,14 @@ static QString *parse_string(JSONParserContext *ctxt, 
> JSONToken *token)
>                   parse_error(ctxt, token, "invalid escape sequence in string");
>                   goto out;
>               }
> -        } else {
> +            break;
> +        case '%':
> +            if (ctxt->ap) {
> +                parse_error(ctxt, token, "can't interpolate into string");

Do we want to abort() here, to match the fact that all our other 
interpolation functions abort on programmer errors?

> +++ b/tests/check-qjson.c
> @@ -1037,16 +1037,13 @@ static void interpolation_unknown(void)
>   
>   static void interpolation_string(void)
>   {
> -    QLitObject decoded = QLIT_QLIST(((QLitObject[]){
> -            QLIT_QSTR("%s"),
> -            QLIT_QSTR("eins"),
> -            {}}));
> -    QObject *qobj;
> -
> -    /* Dangerous misfeature: % is silently ignored in strings */
> -    qobj = qobject_from_jsonf_nofail("['%s', %s]", "eins", "zwei");
> -    g_assert(qlit_equal_qobject(&decoded, qobj));
> -    qobject_unref(qobj);
> +    if (g_test_subprocess()) {
> +        qobject_from_jsonf_nofail("['%s', %s]", "eins", "zwei");
> +    }
> +    g_test_trap_subprocess(NULL, 0, 0);
> +    g_test_trap_assert_failed();
> +    g_test_trap_assert_stderr("*Unexpected error*"
> +                              "can't interpolate into string*");

Oh, we still abort, but later on in the caller function.

Okay, looks like reasonable handling.
Reviewed-by: Eric Blake <address@hidden>

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