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

[Glenn Washburn]

On Mon, 6 Jun 2022 07:18:28 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> On Sun, Jun 05, 2022 at 02:00:44PM -0500, Glenn Washburn wrote:
> > 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.
> [snip]
> 
> Yeah, I thought about this, and in fact my first iteration did exactly
> that. But it complicates the interface somewhat given that you now have
> to pass in the caller-allocated size, return the end result's size and
> also have to somehow handle NUL-terminating the result. So altogether it
> added complexity and felt a lot more fragile compared to just allocating
> the result array.

My thought was to have the caller pass in outlen pointing to the length
of the already allocated output array. No need for extra parameters.
Then outlen would be written to with the number of bytes written to the
output buffer. As for NULL terminating the result, no need to either,
just return an error indicating that the buffer was too small. I was
also thinking of keeping the current functionality, eg. output buffer
allocation, when *out == NULL. But yeah, this can be done some other
day when it would actually be useful.

Glenn