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

[Daniel Kiper]

On Mon, Jun 06, 2022 at 07:28:56AM +0200, Patrick Steinhardt 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 | 101 ++++++++++++++++++++++++++++++++++++++
>  grub-core/lib/json/json.h |  12 +++++
>  2 files changed, 113 insertions(+)
>
> diff --git a/grub-core/lib/json/json.c b/grub-core/lib/json/json.c
> index 1c20c75ea..adb4747a4 100644
> --- a/grub-core/lib/json/json.c
> +++ b/grub-core/lib/json/json.c
> @@ -262,3 +262,104 @@ 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, grub_size_t *outlen, const char *in, 
> grub_size_t inlen)
> +{
> +  grub_err_t ret = GRUB_ERR_NONE;
> +  grub_size_t inpos, resultpos;
> +  char *result;
> +
> +  if (!out || !outlen)

I prefer "if (out == NULL || outlen == NULL)"

Please fix similar issues below.

> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "Output parameters are not 
> set");

grub_error (GRUB_ERR_BAD_ARGUMENT, N_("output parameters are not set"))

Two things are important here. First, you should use N_() in
grub_error() calls. It enables i18n for the GRUB. Second, all
grub_error() messages should start with lowercase except e.g.
abbreviations.

Please fix similar things below.

> +
> +  result = grub_calloc (1, inlen + 1);
> +  if (!result)
> +    return GRUB_ERR_OUT_OF_MEMORY;
> +
> +  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;

Please split this line into two...

result[resultpos++] = '"';
break;

> +           case '/':
> +             result[resultpos++] = '/'; break;

result[resultpos++] = '/';
break;

> +           case '\\':
> +             result[resultpos++] = '\\'; break;

Ditto and below please...

> +           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};

Why values is unsigned char if c is char. I think values should be char too.

> +               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++)

"i < ARRAY_SIZE(values)" instead of "i < 4"?

> +                 {
> +                   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;
> +             }

Please add empty line here and above after every break. OK, the first
one above does not need an additional empty line...

> +           default:
> +             ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Unrecognized escaped 
> character '%c'", in[inpos]);
> +             goto err;
> +         }
> +     }
> +      else
> +       result[resultpos++] = in[inpos];
> +    }
> +
> +  *out = result;
> +  *outlen = resultpos;
> +
> +err:

Please add a space before "err" label.

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

Daniel