On Thu, Jun 23, 2022 at 08:34:07PM +0200, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richard...@intel.com]
> > Sent: Thursday, 23 June 2022 18.43
> >
> > For string values returned from telemetry, escape any values that
> > cannot
> > normally appear in a json string. According to the json spec[1], the
> > characters than need to be handled are control chars (char value <
> > 0x20)
> > and '"' and '\' characters.
>
> Correct. Other chars are optional to escape.
>
> >
> > To handle this, we replace the snprintf call with a separate string
> > copying and encapsulation routine which checks each character as it
> > copies it to the final array.
> >
> > [1] https://www.rfc-editor.org/rfc/rfc8259.txt
> >
> > Signed-off-by: Bruce Richardson <bruce.richard...@intel.com>
> > ---
> > lib/telemetry/telemetry_json.h | 48 +++++++++++++++++++++++++++++++++-
> > 1 file changed, 47 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/telemetry/telemetry_json.h
> > b/lib/telemetry/telemetry_json.h
> > index db70690274..13df5d07e3 100644
> > --- a/lib/telemetry/telemetry_json.h
> > +++ b/lib/telemetry/telemetry_json.h
> > @@ -44,6 +44,52 @@ __json_snprintf(char *buf, const int len, const char
> > *format, ...)
> > return 0; /* nothing written or modified */
> > }
> >
> > +static const char control_chars[0x20] = {
> > + ['\n'] = 'n',
> > + ['\r'] = 'r',
> > + ['\t'] = 't',
> > +};
> > +
> > +/**
> > + * @internal
> > + * Does the same as __json_snprintf(buf, len, "\"%s\"", str)
> > + * except that it does proper escaping as necessary.
> > + * Drops any invalid characters we don't support
> > + */
> > +static inline int
> > +__json_format_str(char *buf, const int len, const char *str)
> > +{
> > + char tmp[len];
> > + int tmpidx = 0;
> > +
> > + tmp[tmpidx++] = '"';
> > + while (*str != '\0') {
> > + if (*str < (int)RTE_DIM(control_chars)) {
>
> I would prefer the more explicit 0x20, directly copied from the RFC.
> RTE_DIM(control_chars) hints that it could change.
>
Sure. Just trying to avoid magic constants, but in this case it does make
sense. Alternatively, I considered using space char as the sentinel value,
as first non-control-char allowed.
> > + int idx = *str; /* compilers don't like char type as
> > index */
> > + if (control_chars[idx] != 0) {
> > + tmp[tmpidx++] = '\\';
> > + tmp[tmpidx++] = control_chars[idx];
> > + }
>
> Consider support for other control characters:
> + else {
> + tmp[tmpidx++] = '\\';
> + tmp[tmpidx++] = 'u';
> + tmp[tmpidx++] = '0';
> + tmp[tmpidx++] = '0';
> + tmp[tmpidx++] = hexchar(idx >> 4);
> + tmp[tmpidx++] = hexchar(idx & 0xf);
> + }
>
> Or just drop them, as you mention in the function's description.
>
Yeah, I'd appreciate general feedback on that. Adding support is nice, but
just not sure if we really need it or not.
> > + } else if (*str == '"' || *str == '\\') {
> > + tmp[tmpidx++] = '\\';
> > + tmp[tmpidx++] = *str;
> > + } else
> > + tmp[tmpidx++] = *str;
> > + /* we always need space for closing quote and null
> > character.
> > + * Ensuring at least two free characters also means we can
> > always take an
> > + * escaped character like "\n" without overflowing
> > + */
> > + if (tmpidx > len - 2)
>
> If supporting the \u00XX encoding, you need to reserve more than 2 characters
> here and in related code.
>
Yep. I avoided supporting it for simplicity for now.
> > + return 0;
> > + str++;
> > + }
> > + tmp[tmpidx++] = '"';
> > + tmp[tmpidx] = '\0';
> > +
> > + strcpy(buf, tmp);
> > + return tmpidx;
> > +}
> > +
> > /* Copies an empty array into the provided buffer. */
> > static inline int
> > rte_tel_json_empty_array(char *buf, const int len, const int used)
> > @@ -62,7 +108,7 @@ rte_tel_json_empty_obj(char *buf, const int len,
> > const int used)
> > static inline int
> > rte_tel_json_str(char *buf, const int len, const int used, const char
> > *str)
> > {
> > - return used + __json_snprintf(buf + used, len - used, "\"%s\"",
> > str);
> > + return used + __json_format_str(buf + used, len - used, str);
> > }
> >
> > /* Appends a string into the JSON array in the provided buffer. */
> > --
> > 2.34.1
> >
>
