[v2,59/60] json: Improve safety of qobject_from_jsonf_nofail() & friends

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.

However, there's another, more dangerous kind of programming error:
since we use va_arg() to get the value to interpolate, behavior is
undefined when the variable argument isn't consistent with the
interpolation specification.

The same problem exists with printf()-like functions, and the solution
is to have the compiler check consistency.  This is what
GCC_FMT_ATTR() is about.

To enable this type checking for interpolation as well, we carefully
chose our interpolation specifications to match printf conversion
specifications, and decorate functions parsing templates with
GCC_FMT_ATTR().

Note that this only protects against undefined behavior due to type
errors.  It can't protect against use of invalid interpolation
specifications that happen to be valid printf conversion
specifications.

However, there's still a gaping hole in the type checking: GCC
recognizes '%' as start of printf conversion specification anywhere in
the template, but the parser recognizes it only outside JSON strings.
For instance, if someone were to pass a "{ '%s': %d }" template, GCC
would require a char * and an int argument, but the parser would
va_arg() only an int argument, resulting in undefined behavior.

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 <armbru@redhat.com>
---
 qobject/json-parser.c | 12 ++++++++++--
 tests/check-qjson.c   | 17 +++++++----------
 2 files changed, 17 insertions(+), 12 deletions(-)

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 <armbru@redhat.com>
> ---

> @@ -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 <eblake@redhat.com>