Re: [PATCH v3 1/2] json: Add function to unescape JSON-encoded strings

[Glenn Washburn]

On Mon, 30 May 2022 18:01:01 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> JSON strings require certain characters to be encoded, either by using a
> single reverse solidus character "\" for a set of popular characters, or
> by using a Unicode representation of "\uXXXXX". The jsmn library doesn't
> handle unescaping for us, so we must implement this functionality for
> ourselves.
> 
> Add a new function `grub_json_unescape ()` that takes a potentially
> escaped JSON string as input and returns a new unescaped string.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  grub-core/lib/json/json.c | 98 +++++++++++++++++++++++++++++++++++++++
>  grub-core/lib/json/json.h | 12 +++++
>  2 files changed, 110 insertions(+)
> 
> diff --git a/grub-core/lib/json/json.c b/grub-core/lib/json/json.c
> index 1c20c75ea..3e5e25f79 100644
> --- a/grub-core/lib/json/json.c
> +++ b/grub-core/lib/json/json.c
> @@ -262,3 +262,101 @@ grub_json_getint64 (grub_int64_t *out, const 
> grub_json_t *parent, const char *ke
>  
>    return GRUB_ERR_NONE;
>  }
> +
> +grub_err_t
> +grub_json_unescape (char **out, size_t *outlen, const char *in, size_t inlen)

Why not grub_size_t instead of size_t?

> +{
> +  grub_err_t ret = GRUB_ERR_NONE;
> +  size_t inpos, resultpos;

Same here.

> +  char *result;
> +
> +  result = grub_calloc (1, inlen + 1);

Should check for grub_calloc failure.

I like the idea of being able to do an inplace conversion if in ==
*out, which would save on memory and be safe as the out string length
should always be less than or equal to the length of the in string.
I'll admit its not really necessary for the current use-case.

> +
> +  for (inpos = resultpos = 0; inpos < inlen; inpos++)
> +    {
> +      if (in[inpos] == '\\')
> +     {
> +       inpos++;
> +       if (inpos >= inlen)
> +         {
> +           ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Expected escaped 
> character");
> +           goto err;
> +         }
> +
> +       switch (in[inpos])
> +         {
> +           case '"':
> +             result[resultpos++] = '"'; break;
> +           case '/':
> +             result[resultpos++] = '/'; break;
> +           case '\\':
> +             result[resultpos++] = '\\'; break;
> +           case 'b':
> +             result[resultpos++] = '\b'; break;
> +           case 'f':
> +             result[resultpos++] = '\f'; break;
> +           case 'r':
> +             result[resultpos++] = '\r'; break;
> +           case 'n':
> +             result[resultpos++] = '\n'; break;
> +           case 't':
> +             result[resultpos++] = '\t'; break;
> +           case 'u':
> +             {
> +               unsigned char values[4] = {0};
> +               int i;
> +
> +               inpos++;
> +               if (inpos + ARRAY_SIZE(values) > inlen)
> +                 {
> +                   ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Unicode 
> sequence too short");
> +                   goto err;
> +                 }
> +
> +               for (i = 0; i < 4; i++)
> +                 {
> +                   char c = in[inpos++];
> +
> +                   if (c >= '0' && c <= '9')
> +                     values[i] = c - '0';
> +                   else if (c >= 'A' && c <= 'F')
> +                     values[i] = c - 'A' + 10;
> +                   else if (c >= 'a' && c <= 'f')
> +                     values[i] = c - 'a' + 10;
> +                   else
> +                     {
> +                       ret = grub_error (GRUB_ERR_BAD_ARGUMENT,
> +                                         "Unicode sequence with invalid 
> character '%c'", c);
> +                       goto err;
> +                     }
> +                 }
> +
> +               if (values[0] != 0 || values[1] != 0)
> +                 result[resultpos++] = values[0] << 4 | values[1];
> +               result[resultpos++] = values[2] << 4 | values[3];
> +
> +               /* Offset the increment that's coming in via the loop 
> increment. */
> +               inpos--;
> +
> +               break;
> +             }
> +           default:
> +             ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Unrecognized escaped 
> character '%c'", in[inpos]);
> +             goto err;
> +         }
> +     }
> +      else
> +     {
> +       result[resultpos++] = in[inpos];
> +     }

Iirc, Daniel has questioned the need for brackets when the block is
one line.

> +    }
> +
> +  *out = result;
> +  *outlen = resultpos;

Might be good to check these, out and outlen, pointers for NULL before
using.

> +
> +err:
> +  if (ret != GRUB_ERR_NONE)
> +    grub_free (result);
> +
> +  return GRUB_ERR_NONE;

This should be "return ret;", otherwise this function always returns
success, even when it sets grub_errno.

> +}
> diff --git a/grub-core/lib/json/json.h b/grub-core/lib/json/json.h
> index 4ea2a22d8..37a5a5f57 100644
> --- a/grub-core/lib/json/json.h
> +++ b/grub-core/lib/json/json.h
> @@ -125,4 +125,16 @@ extern grub_err_t EXPORT_FUNC(grub_json_getint64) 
> (grub_int64_t *out,
>                                                  const grub_json_t *parent,
>                                                  const char *key);
>  
> +/*
> + * Unescape escaped characters and Unicode sequences in the
> + * given JSON-encoded string. Returns a newly allocated string
> + * passed back via the `out` parameter that has a length of
> + * `*outlen`.
> + *
> + * See https://datatracker.ietf.org/doc/html/rfc8259#section-7 for more
> + * information on escaping in JSON.
> + */
> +extern grub_err_t EXPORT_FUNC(grub_json_unescape) (char **out, size_t 
> *outlen,
> +                                                const char *in, size_t 
> inlen);
> +
>  #endif

Glenn