Re: [PATCH v2 02/13] telemetry: fix escaping of invalid json characters

[fengchengwen <fengchengwen@huawei.com>]

On 2022/7/26 0:35, Bruce Richardson wrote:
> 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.
> 
> 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
> 
> Fixes: 6dd571fd07c3 ("telemetry: introduce new functionality")
> Bugzilla ID: 1037
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  lib/telemetry/telemetry.c      | 11 +++++---
>  lib/telemetry/telemetry_json.h | 48 +++++++++++++++++++++++++++++++++-
>  2 files changed, 55 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
> index c6fd03a5ab..7188b1905c 100644
> --- a/lib/telemetry/telemetry.c
> +++ b/lib/telemetry/telemetry.c
> @@ -232,9 +232,14 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
>  				MAX_CMD_LEN, cmd ? cmd : "none");
>  		break;
>  	case RTE_TEL_STRING:
> -		used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":\"%.*s\"}",
> -				MAX_CMD_LEN, cmd,
> -				RTE_TEL_MAX_SINGLE_STRING_LEN, d->data.str);
> +		prefix_used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":",
> +				MAX_CMD_LEN, cmd);

The cmd need also escaped.
But I notice the [PATCH v2 06/13] limit it. Suggest move 06 at the head of patchset.

> +		cb_data_buf = &out_buf[prefix_used];
> +		buf_len = sizeof(out_buf) - prefix_used - 1; /* space for '}' */
> +
> +		used = rte_tel_json_str(cb_data_buf, buf_len, 0, d->data.str);
> +		used += prefix_used;
> +		used += strlcat(out_buf + used, "}", sizeof(out_buf) - used);
>  		break;
>  	case RTE_TEL_DICT:
>  		prefix_used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":",
> 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];

Could reuse buf otherthan tmp

> +	int tmpidx = 0;
> +
> +	tmp[tmpidx++] = '"';
> +	while (*str != '\0') {
> +		if (*str < (int)RTE_DIM(control_chars)) {
> +			int idx = *str;  /* compilers don't like char type as index */
> +			if (control_chars[idx] != 0) {
> +				tmp[tmpidx++] = '\\';
> +				tmp[tmpidx++] = control_chars[idx];

Why not espace all control chars?

> +			}
> +		} 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)
> +			return 0;

Suggest add log here to help find out problem.

> +		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. */
>