[PATCH] telemetry: fix autotest failures on Alpine

[PATCH] telemetry: fix autotest failures on Alpine

[Bruce Richardson]

On Alpine linux, the telemetry_data_autotest was failing for the
test where we had dictionaries embedded in other dictionaries up
to three levels deep. Indications are that this issue is due to
excess data being stored on the stack, so replace stack-allocated
buffer data with dynamically allocated data in the case where we
are doing recursive processing of telemetry data structures into
json.

Bugzilla ID: 1177
Fixes: c933bb5177ca ("telemetry: support array values in data object")
Fixes: d2671e642a8e ("telemetry: support dict of dicts")
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/telemetry/telemetry.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
index 7bceadcee7..34d371ab8a 100644
--- a/lib/telemetry/telemetry.c
+++ b/lib/telemetry/telemetry.c
@@ -208,7 +208,9 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
 				break;
 			case RTE_TEL_CONTAINER:
 			{
-				char temp[buf_len];
+				char *temp = malloc(buf_len);
+				if (temp == NULL)
+					break;
 				const struct container *cont =
 						&v->value.container;
 				if (container_to_json(cont->data,
@@ -219,6 +221,7 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
 							v->name, temp);
 				if (!cont->keep)
 					rte_tel_data_free(cont->data);
+				free(temp);
 				break;
 			}
 			}
@@ -275,7 +278,9 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
 				break;
 			case RTE_TEL_CONTAINER:
 			{
-				char temp[buf_len];
+				char *temp = malloc(buf_len);
+				if (temp == NULL)
+					break;
 				const struct container *cont =
 						&v->value.container;
 				if (container_to_json(cont->data,
@@ -286,6 +291,7 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
 							v->name, temp);
 				if (!cont->keep)
 					rte_tel_data_free(cont->data);
+				free(temp);
 			}
 			}
 		}
@@ -311,7 +317,9 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
 						buf_len, used,
 						d->data.array[i].uval);
 			else if (d->type == TEL_ARRAY_CONTAINER) {
-				char temp[buf_len];
+				char *temp = malloc(buf_len);
+				if (temp == NULL)
+					break;
 				const struct container *rec_data =
 						&d->data.array[i].container;
 				if (container_to_json(rec_data->data,
@@ -321,6 +329,7 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
 							buf_len, used, temp);
 				if (!rec_data->keep)
 					rte_tel_data_free(rec_data->data);
+				free(temp);
 			}
 		break;
 	}
-- 
2.37.2

Re: [PATCH] telemetry: fix autotest failures on Alpine

[Stephen Hemminger]

On Fri, 10 Mar 2023 18:18:36 +0000
Bruce Richardson <bruce.richardson@intel.com> wrote:

> On Alpine linux, the telemetry_data_autotest was failing for the
> test where we had dictionaries embedded in other dictionaries up
> to three levels deep. Indications are that this issue is due to
> excess data being stored on the stack, so replace stack-allocated
> buffer data with dynamically allocated data in the case where we
> are doing recursive processing of telemetry data structures into
> json.
> 
> Bugzilla ID: 1177
> Fixes: c933bb5177ca ("telemetry: support array values in data object")
> Fixes: d2671e642a8e ("telemetry: support dict of dicts")
> Cc: stable@dpdk.org

Looking at the telemetry code:

- why so many temporary buffers, could this be streamed or redesigned
  so that an allocated buffer is returned.

- why is rte_tel_json_XXX all inline?  These should just be internal
  functions and not in a .h file.

FYI - if this library reused existing json writer it would have
been much simpler.  
https://github.com/shemminger/iproute2/blob/main/lib/json_writer.c

Re: [PATCH] telemetry: fix autotest failures on Alpine

[Bruce Richardson]

On Fri, Mar 10, 2023 at 11:08:32AM -0800, Stephen Hemminger wrote:
> On Fri, 10 Mar 2023 18:18:36 +0000
> Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
> > On Alpine linux, the telemetry_data_autotest was failing for the
> > test where we had dictionaries embedded in other dictionaries up
> > to three levels deep. Indications are that this issue is due to
> > excess data being stored on the stack, so replace stack-allocated
> > buffer data with dynamically allocated data in the case where we
> > are doing recursive processing of telemetry data structures into
> > json.
> > 
> > Bugzilla ID: 1177
> > Fixes: c933bb5177ca ("telemetry: support array values in data object")
> > Fixes: d2671e642a8e ("telemetry: support dict of dicts")
> > Cc: stable@dpdk.org
> 
> Looking at the telemetry code:
> 
> - why so many temporary buffers, could this be streamed or redesigned
>   so that an allocated buffer is returned.
> 

This is largely what the fix patch does. The temporary buffers are used to
ensure we never end up with a partially written buffer from any truncated
snprintf, since that could cause us to emit invalid json. I think as a
scheme it works well enough for what it was designed for, but this
particular test case has more levels of recursion than was expected, so the
limit of the design are showing. The downside of using memory allocation
using malloc as here, is just more failure cases.

> - why is rte_tel_json_XXX all inline?  These should just be internal
>   functions and not in a .h file.
>

Sure. Since these are all just internal functions used by the library, I'm
not sure it really matters. 

> FYI - if this library reused existing json writer it would have
> been much simpler.  
> https://github.com/shemminger/iproute2/blob/main/lib/json_writer.c

Yep. Looked at that previously, but it's at a lower level than what we
have. IMHO, the complicated bit of producing json output is not the
formatting characters for each data-type but ensuring correct termination
of the json string even in case of errors, so each function in our
telemetry json code always includes correct terminators at all points, i.e.
we have no separate functions to be called for terminating objects, arrays
etc. - they are always included in the output of the items. This is why so
many temporary buffers are used - the input string to a function is
well-formed json, and we only actually append to that buffer by copying
from the temporary buffer once we are sure that the output will also be
similarly well-formed.

That said, if you want to replace the current json implementation with a
better one, I'm not opposed to it. [though I'd definitely rather no
external dependencies which would mean we could no longer rely on it always
being available in default builds]

/Bruce

[PATCH v2 0/5] telemetry: remove variable length arrays

[Bruce Richardson]

This patchset introduces a series of changes to remove variable-length
arrays from the telemetry code. The first patch replaces a VLA with
malloc memory for the serialization of the json objects contained within
the main response object, which fixes a crash observed on alpine linux.

Subsequent patches rework the json printing code to avoid the use of
temporary buffers where possible, or use malloc-allocated memory where
not.

Based off testing with the unit tests for telemetry, json serialization
for the telemetry callbacks should always use the path where a temporary
buffer is *not* used, but the allocation-case is kept to ensure that any
unexpected edge-cases are covered too.

Bruce Richardson (5):
  telemetry: fix autotest failures on Alpine
  telemetry: remove variable length array in printf fn
  telemetry: split out body of json string format fn
  telemetry: rename local variables
  telemetry: remove VLA in json string format function

 app/test/test_telemetry_json.c |   2 +-
 lib/telemetry/telemetry.c      |  21 ++++++-
 lib/telemetry/telemetry_json.h | 107 +++++++++++++++++++++++++--------
 3 files changed, 100 insertions(+), 30 deletions(-)

--
2.37.2

[PATCH v2 1/5] telemetry: fix autotest failures on Alpine

[Bruce Richardson]

On Alpine linux, the telemetry_data_autotest was failing for the
test where we had dictionaries embedded in other dictionaries up
to three levels deep. Indications are that this issue is due to
excess data being stored on the stack, so replace stack-allocated
buffer data with dynamically allocated data in the case where we
are doing recursive processing of telemetry data structures into
json.

Bugzilla ID: 1177
Fixes: c933bb5177ca ("telemetry: support array values in data object")
Fixes: d2671e642a8e ("telemetry: support dict of dicts")
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

---
V2:
  set '\0' in newly malloc'ed buffer to ensure it always has valid
  string data.
---
 lib/telemetry/telemetry.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
index 7bceadcee7..728a0bffd4 100644
--- a/lib/telemetry/telemetry.c
+++ b/lib/telemetry/telemetry.c
@@ -208,7 +208,11 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
 				break;
 			case RTE_TEL_CONTAINER:
 			{
-				char temp[buf_len];
+				char *temp = malloc(buf_len);
+				if (temp == NULL)
+					break;
+				*temp = '\0';  /* ensure valid string */
+
 				const struct container *cont =
 						&v->value.container;
 				if (container_to_json(cont->data,
@@ -219,6 +223,7 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
 							v->name, temp);
 				if (!cont->keep)
 					rte_tel_data_free(cont->data);
+				free(temp);
 				break;
 			}
 			}
@@ -275,7 +280,11 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
 				break;
 			case RTE_TEL_CONTAINER:
 			{
-				char temp[buf_len];
+				char *temp = malloc(buf_len);
+				if (temp == NULL)
+					break;
+				*temp = '\0';  /* ensure valid string */
+
 				const struct container *cont =
 						&v->value.container;
 				if (container_to_json(cont->data,
@@ -286,6 +295,7 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
 							v->name, temp);
 				if (!cont->keep)
 					rte_tel_data_free(cont->data);
+				free(temp);
 			}
 			}
 		}
@@ -311,7 +321,11 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
 						buf_len, used,
 						d->data.array[i].uval);
 			else if (d->type == TEL_ARRAY_CONTAINER) {
-				char temp[buf_len];
+				char *temp = malloc(buf_len);
+				if (temp == NULL)
+					break;
+				*temp = '\0';  /* ensure valid string */
+
 				const struct container *rec_data =
 						&d->data.array[i].container;
 				if (container_to_json(rec_data->data,
@@ -321,6 +335,7 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
 							buf_len, used, temp);
 				if (!rec_data->keep)
 					rte_tel_data_free(rec_data->data);
+				free(temp);
 			}
 		break;
 	}
-- 
2.37.2

[PATCH v2 2/5] telemetry: remove variable length array in printf fn

[Bruce Richardson]

The json_snprintf function, used to add json characters on to a buffer,
leaving the buffer unmodified in case of error, used a variable length
array to store the data temporarily while checking for overflow. VLAs
can be unsafe, and are unsupported by some compilers, so remove use of
the VLA.

For the normal case where there is only a small amount of existing text
in the buffer (<4 chars) to be preserved, save that off temporarily to a
local array, and restore on error. To handle cases where there is more
than a few characters in the buffer, we use the existing logic of doing
the print to a temporary buffer initially and then copying. In this
case, though we use malloc-allocated buffer rather than VLA.

Within the unit tests, the "telemetry_data_autotests" test cases - which
mimic real telemetry use - all exercise the first path. The
telemetry_json_autotest cases work directly with generating json, and
use uninitialized buffers so also test the second, malloc-allocated
buffer, cases.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/telemetry/telemetry_json.h | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
index 744bbfe053..cb18453449 100644
--- a/lib/telemetry/telemetry_json.h
+++ b/lib/telemetry/telemetry_json.h
@@ -8,6 +8,7 @@
 #include <inttypes.h>
 #include <stdarg.h>
 #include <stdio.h>
+#include <stdlib.h>
 #include <rte_common.h>
 #include <rte_telemetry.h>
 
@@ -30,17 +31,40 @@ __rte_format_printf(3, 4)
 static inline int
 __json_snprintf(char *buf, const int len, const char *format, ...)
 {
-	char tmp[len];
 	va_list ap;
+	char tmp[4];
+	char *newbuf;
 	int ret;
 
+	if (len == 0)
+		return 0;
+
+	/* to ensure unmodified if we overflow, we save off any values currently in buf
+	 * before we printf, if they are short enough. We restore them on error.
+	 */
+	if (strnlen(buf, sizeof(tmp)) < sizeof(tmp)) {
+		strcpy(tmp, buf);  /* strcpy is safe as we know the length */
+		va_start(ap, format);
+		ret = vsnprintf(buf, len, format, ap);
+		va_end(ap);
+		if (ret > 0 && ret < len)
+			return ret;
+		strcpy(buf, tmp);  /* restore on error */
+		return 0;
+	}
+
+	/* in normal operations should never hit this, but can do if buffer is
+	 * incorrectly initialized e.g. in unit test cases
+	 */
 	va_start(ap, format);
-	ret = vsnprintf(tmp, sizeof(tmp), format, ap);
+	ret = vasprintf(&newbuf, format, ap);
 	va_end(ap);
-	if (ret > 0 && ret < (int)sizeof(tmp) && ret < len) {
-		strcpy(buf, tmp);
+	if (ret > 0 && ret < len) {
+		strcpy(buf, newbuf);
+		free(newbuf);
 		return ret;
 	}
+	free(newbuf);
 	return 0; /* nothing written or modified */
 }
 
-- 
2.37.2

[PATCH v2 3/5] telemetry: split out body of json string format fn

[Bruce Richardson]

To enable further rework to (efficiently) avoid using variable-length
arrays, we first separate out the body of the __json_format_str
function. This means that the actual VLA buffer is in the wrapper
function, and means we can reuse the actual writing code in multiple
code paths without duplication.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/telemetry/telemetry_json.h | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
index cb18453449..4c1941ad15 100644
--- a/lib/telemetry/telemetry_json.h
+++ b/lib/telemetry/telemetry_json.h
@@ -76,15 +76,13 @@ static const char control_chars[0x20] = {
 
 /**
  * @internal
- * This function acts the same as __json_snprintf(buf, len, "%s%s%s", prefix, str, suffix)
- * except that it does proper escaping of "str" as necessary. Prefix and suffix should be compile-
- * time constants, or values not needing escaping.
- * Drops any invalid characters we don't support
+ * Function that does the actual printing, used by __json_format_str. Modifies buffer
+ * directly, but returns 0 on overflow. Otherwise returns number of chars written to buffer.
  */
 static inline int
-__json_format_str(char *buf, const int len, const char *prefix, const char *str, const char *suffix)
+__json_format_str_to_buf(char *tmp, const int len,
+		const char *prefix, const char *str, const char *suffix)
 {
-	char tmp[len];
 	int tmpidx = 0;
 
 	while (*prefix != '\0' && tmpidx < len)
@@ -119,11 +117,29 @@ __json_format_str(char *buf, const int len, const char *prefix, const char *str,
 		return 0;
 
 	tmp[tmpidx] = '\0';
-
-	strcpy(buf, tmp);
 	return tmpidx;
 }
 
+/**
+ * @internal
+ * This function acts the same as __json_snprintf(buf, len, "%s%s%s", prefix, str, suffix)
+ * except that it does proper escaping of "str" as necessary. Prefix and suffix should be compile-
+ * time constants, or values not needing escaping.
+ * Drops any invalid characters we don't support
+ */
+static inline int
+__json_format_str(char *buf, const int len, const char *prefix, const char *str, const char *suffix)
+{
+	char tmp[len];
+	int ret;
+
+	ret = __json_format_str_to_buf(tmp, len, prefix, str, suffix);
+	if (ret > 0)
+		strcpy(buf, tmp);
+
+	return ret;
+}
+
 /* Copies an empty array into the provided buffer. */
 static inline int
 rte_tel_json_empty_array(char *buf, const int len, const int used)
-- 
2.37.2

[PATCH v2 4/5] telemetry: rename local variables

[Bruce Richardson]

In the newly separated out function, rename "tmp" to "buf" to have more
meaningful variable names.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

---

When committing, this patch can be merged with the previous. I've kept
them separate for now, as it makes reviewing a lot easier.
---
 lib/telemetry/telemetry_json.h | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
index 4c1941ad15..4d725d938b 100644
--- a/lib/telemetry/telemetry_json.h
+++ b/lib/telemetry/telemetry_json.h
@@ -80,44 +80,44 @@ static const char control_chars[0x20] = {
  * directly, but returns 0 on overflow. Otherwise returns number of chars written to buffer.
  */
 static inline int
-__json_format_str_to_buf(char *tmp, const int len,
+__json_format_str_to_buf(char *buf, const int len,
 		const char *prefix, const char *str, const char *suffix)
 {
-	int tmpidx = 0;
+	int bufidx = 0;
 
-	while (*prefix != '\0' && tmpidx < len)
-		tmp[tmpidx++] = *prefix++;
-	if (tmpidx >= len)
+	while (*prefix != '\0' && bufidx < len)
+		buf[bufidx++] = *prefix++;
+	if (bufidx >= len)
 		return 0;
 
 	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];
+				buf[bufidx++] = '\\';
+				buf[bufidx++] = control_chars[idx];
 			}
 		} else if (*str == '"' || *str == '\\') {
-			tmp[tmpidx++] = '\\';
-			tmp[tmpidx++] = *str;
+			buf[bufidx++] = '\\';
+			buf[bufidx++] = *str;
 		} else
-			tmp[tmpidx++] = *str;
+			buf[bufidx++] = *str;
 		/* we always need space for (at minimum) 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 (bufidx > len - 2)
 			return 0;
 		str++;
 	}
 
-	while (*suffix != '\0' && tmpidx < len)
-		tmp[tmpidx++] = *suffix++;
-	if (tmpidx >= len)
+	while (*suffix != '\0' && bufidx < len)
+		buf[bufidx++] = *suffix++;
+	if (bufidx >= len)
 		return 0;
 
-	tmp[tmpidx] = '\0';
-	return tmpidx;
+	buf[bufidx] = '\0';
+	return bufidx;
 }
 
 /**
-- 
2.37.2

[PATCH v2 5/5] telemetry: remove VLA in json string format function

[Bruce Richardson]

Since variable length arrays (VLAs) are potentially unsecure and
unsupported by some compilers, rework the code to remove their use. As
with previous changes to remove VLAs in the telemetry code, this
function uses two methods to avoid modifying the buffer when adding to
it fails:
* if there are only a few characters in the buffer, save them off to
  restore on failure, then use the buffer as-is,
* otherwise use malloc rather than a VLA to allocate a temporary buffer
  and copy from that on success only.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/test_telemetry_json.c |  2 +-
 lib/telemetry/telemetry_json.h | 19 +++++++++++++++++--
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/app/test/test_telemetry_json.c b/app/test/test_telemetry_json.c
index e81e3a8a98..5617eac540 100644
--- a/app/test/test_telemetry_json.c
+++ b/app/test/test_telemetry_json.c
@@ -129,7 +129,7 @@ test_string_char_escaping(void)
 {
 	static const char str[] = "A string across\ntwo lines and \"with quotes\"!";
 	const char *expected = "\"A string across\\ntwo lines and \\\"with quotes\\\"!\"";
-	char buf[sizeof(str) + 10];
+	char buf[sizeof(str) + 10] = "";
 	int used = 0;
 
 	used = rte_tel_json_str(buf, sizeof(buf), used, str);
diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
index 4d725d938b..fceff91842 100644
--- a/lib/telemetry/telemetry_json.h
+++ b/lib/telemetry/telemetry_json.h
@@ -130,13 +130,28 @@ __json_format_str_to_buf(char *buf, const int len,
 static inline int
 __json_format_str(char *buf, const int len, const char *prefix, const char *str, const char *suffix)
 {
-	char tmp[len];
 	int ret;
+	char saved[4] = "";
+	char *tmp;
+
+	if (strnlen(buf, sizeof(saved)) < sizeof(saved)) {
+		/* we have only a few bytes in buffer, so save them off to restore on error*/
+		strcpy(saved, buf);
+		ret = __json_format_str_to_buf(buf, len, prefix, str, suffix);
+		if (ret == 0)
+			strcpy(buf, saved); /* restore */
+		return ret;
+	}
+
+	tmp = malloc(len);
+	if (tmp == NULL)
+		return 0;
 
 	ret = __json_format_str_to_buf(tmp, len, prefix, str, suffix);
 	if (ret > 0)
-		strcpy(buf, tmp);
+		strcpy(buf, saved);
 
+	free(tmp);
 	return ret;
 }
 
-- 
2.37.2

[PATCH v3 0/5] telemetry: remove variable length arrays

[Bruce Richardson]

This patchset introduces a series of changes to remove variable-length
arrays from the telemetry code. The first patch replaces a VLA with
malloc memory for the serialization of the json objects contained within
the main response object, which fixes a crash observed on alpine linux.

Subsequent patches rework the json printing code to avoid the use of
temporary buffers where possible, or use malloc-allocated memory where
not.

Based off testing with the unit tests for telemetry, json serialization
for the telemetry callbacks should always use the path where a temporary
buffer is *not* used, but the allocation-case is kept to ensure that any
unexpected edge-cases are covered too.

V3: remove use of non-standard asprintf function in patch 2.

V2: expand from single fix for Alpine, to general cleanup to remove VLAs

Bruce Richardson (5):
  telemetry: fix autotest failures on Alpine
  telemetry: remove variable length array in printf fn
  telemetry: split out body of json string format fn
  telemetry: rename local variables
  telemetry: remove VLA in json string format function

 app/test/test_telemetry_json.c |   2 +-
 lib/telemetry/telemetry.c      |  21 ++++++-
 lib/telemetry/telemetry_json.h | 111 +++++++++++++++++++++++++--------
 3 files changed, 104 insertions(+), 30 deletions(-)

--
2.37.2

[PATCH v3 1/5] telemetry: fix autotest failures on Alpine

[Bruce Richardson]

On Alpine linux, the telemetry_data_autotest was failing for the
test where we had dictionaries embedded in other dictionaries up
to three levels deep. Indications are that this issue is due to
excess data being stored on the stack, so replace stack-allocated
buffer data with dynamically allocated data in the case where we
are doing recursive processing of telemetry data structures into
json.

Bugzilla ID: 1177
Fixes: c933bb5177ca ("telemetry: support array values in data object")
Fixes: d2671e642a8e ("telemetry: support dict of dicts")
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

---
V2:
  set '\0' in newly malloc'ed buffer to ensure it always has valid
  string data.
---
 lib/telemetry/telemetry.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
index 7bceadcee7..728a0bffd4 100644
--- a/lib/telemetry/telemetry.c
+++ b/lib/telemetry/telemetry.c
@@ -208,7 +208,11 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
 				break;
 			case RTE_TEL_CONTAINER:
 			{
-				char temp[buf_len];
+				char *temp = malloc(buf_len);
+				if (temp == NULL)
+					break;
+				*temp = '\0';  /* ensure valid string */
+
 				const struct container *cont =
 						&v->value.container;
 				if (container_to_json(cont->data,
@@ -219,6 +223,7 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
 							v->name, temp);
 				if (!cont->keep)
 					rte_tel_data_free(cont->data);
+				free(temp);
 				break;
 			}
 			}
@@ -275,7 +280,11 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
 				break;
 			case RTE_TEL_CONTAINER:
 			{
-				char temp[buf_len];
+				char *temp = malloc(buf_len);
+				if (temp == NULL)
+					break;
+				*temp = '\0';  /* ensure valid string */
+
 				const struct container *cont =
 						&v->value.container;
 				if (container_to_json(cont->data,
@@ -286,6 +295,7 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
 							v->name, temp);
 				if (!cont->keep)
 					rte_tel_data_free(cont->data);
+				free(temp);
 			}
 			}
 		}
@@ -311,7 +321,11 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
 						buf_len, used,
 						d->data.array[i].uval);
 			else if (d->type == TEL_ARRAY_CONTAINER) {
-				char temp[buf_len];
+				char *temp = malloc(buf_len);
+				if (temp == NULL)
+					break;
+				*temp = '\0';  /* ensure valid string */
+
 				const struct container *rec_data =
 						&d->data.array[i].container;
 				if (container_to_json(rec_data->data,
@@ -321,6 +335,7 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
 							buf_len, used, temp);
 				if (!rec_data->keep)
 					rte_tel_data_free(rec_data->data);
+				free(temp);
 			}
 		break;
 	}
-- 
2.37.2

[PATCH v3 2/5] telemetry: remove variable length array in printf fn

[Bruce Richardson]

The json_snprintf function, used to add json characters on to a buffer,
leaving the buffer unmodified in case of error, used a variable length
array to store the data temporarily while checking for overflow. VLAs
can be unsafe, and are unsupported by some compilers, so remove use of
the VLA.

For the normal case where there is only a small amount of existing text
in the buffer (<4 chars) to be preserved, save that off temporarily to a
local array, and restore on error. To handle cases where there is more
than a few characters in the buffer, we use the existing logic of doing
the print to a temporary buffer initially and then copying. In this
case, though we use malloc-allocated buffer rather than VLA.

Within the unit tests, the "telemetry_data_autotests" test cases - which
mimic real telemetry use - all exercise the first path. The
telemetry_json_autotest cases work directly with generating json, and
use uninitialized buffers so also test the second, malloc-allocated
buffer, cases.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

---
v3: remove use of non-standard vasprintf
---
 lib/telemetry/telemetry_json.h | 36 ++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
index 744bbfe053..1bddd124f9 100644
--- a/lib/telemetry/telemetry_json.h
+++ b/lib/telemetry/telemetry_json.h
@@ -8,6 +8,7 @@
 #include <inttypes.h>
 #include <stdarg.h>
 #include <stdio.h>
+#include <stdlib.h>
 #include <rte_common.h>
 #include <rte_telemetry.h>
 
@@ -30,17 +31,44 @@ __rte_format_printf(3, 4)
 static inline int
 __json_snprintf(char *buf, const int len, const char *format, ...)
 {
-	char tmp[len];
 	va_list ap;
+	char tmp[4];
+	char *newbuf;
 	int ret;
 
+	if (len == 0)
+		return 0;
+
+	/* to ensure unmodified if we overflow, we save off any values currently in buf
+	 * before we printf, if they are short enough. We restore them on error.
+	 */
+	if (strnlen(buf, sizeof(tmp)) < sizeof(tmp)) {
+		strcpy(tmp, buf);  /* strcpy is safe as we know the length */
+		va_start(ap, format);
+		ret = vsnprintf(buf, len, format, ap);
+		va_end(ap);
+		if (ret > 0 && ret < len)
+			return ret;
+		strcpy(buf, tmp);  /* restore on error */
+		return 0;
+	}
+
+	/* in normal operations should never hit this, but can do if buffer is
+	 * incorrectly initialized e.g. in unit test cases
+	 */
+	newbuf = malloc(len);
+	if (newbuf == NULL)
+		return 0;
+
 	va_start(ap, format);
-	ret = vsnprintf(tmp, sizeof(tmp), format, ap);
+	ret = vsnprintf(newbuf, len, format, ap);
 	va_end(ap);
-	if (ret > 0 && ret < (int)sizeof(tmp) && ret < len) {
-		strcpy(buf, tmp);
+	if (ret > 0 && ret < len) {
+		strcpy(buf, newbuf);
+		free(newbuf);
 		return ret;
 	}
+	free(newbuf);
 	return 0; /* nothing written or modified */
 }
 
-- 
2.37.2

[PATCH v3 3/5] telemetry: split out body of json string format fn

[Bruce Richardson]

To enable further rework to (efficiently) avoid using variable-length
arrays, we first separate out the body of the __json_format_str
function. This means that the actual VLA buffer is in the wrapper
function, and means we can reuse the actual writing code in multiple
code paths without duplication.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/telemetry/telemetry_json.h | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
index 1bddd124f9..aada523a27 100644
--- a/lib/telemetry/telemetry_json.h
+++ b/lib/telemetry/telemetry_json.h
@@ -80,15 +80,13 @@ static const char control_chars[0x20] = {
 
 /**
  * @internal
- * This function acts the same as __json_snprintf(buf, len, "%s%s%s", prefix, str, suffix)
- * except that it does proper escaping of "str" as necessary. Prefix and suffix should be compile-
- * time constants, or values not needing escaping.
- * Drops any invalid characters we don't support
+ * Function that does the actual printing, used by __json_format_str. Modifies buffer
+ * directly, but returns 0 on overflow. Otherwise returns number of chars written to buffer.
  */
 static inline int
-__json_format_str(char *buf, const int len, const char *prefix, const char *str, const char *suffix)
+__json_format_str_to_buf(char *tmp, const int len,
+		const char *prefix, const char *str, const char *suffix)
 {
-	char tmp[len];
 	int tmpidx = 0;
 
 	while (*prefix != '\0' && tmpidx < len)
@@ -123,11 +121,29 @@ __json_format_str(char *buf, const int len, const char *prefix, const char *str,
 		return 0;
 
 	tmp[tmpidx] = '\0';
-
-	strcpy(buf, tmp);
 	return tmpidx;
 }
 
+/**
+ * @internal
+ * This function acts the same as __json_snprintf(buf, len, "%s%s%s", prefix, str, suffix)
+ * except that it does proper escaping of "str" as necessary. Prefix and suffix should be compile-
+ * time constants, or values not needing escaping.
+ * Drops any invalid characters we don't support
+ */
+static inline int
+__json_format_str(char *buf, const int len, const char *prefix, const char *str, const char *suffix)
+{
+	char tmp[len];
+	int ret;
+
+	ret = __json_format_str_to_buf(tmp, len, prefix, str, suffix);
+	if (ret > 0)
+		strcpy(buf, tmp);
+
+	return ret;
+}
+
 /* Copies an empty array into the provided buffer. */
 static inline int
 rte_tel_json_empty_array(char *buf, const int len, const int used)
-- 
2.37.2

[PATCH v3 4/5] telemetry: rename local variables

[Bruce Richardson]

In the newly separated out function, rename "tmp" to "buf" to have more
meaningful variable names.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

---

When committing, this patch can be merged with the previous. I've kept
them separate for now, as it makes reviewing a lot easier.
---
 lib/telemetry/telemetry_json.h | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
index aada523a27..c087b833eb 100644
--- a/lib/telemetry/telemetry_json.h
+++ b/lib/telemetry/telemetry_json.h
@@ -84,44 +84,44 @@ static const char control_chars[0x20] = {
  * directly, but returns 0 on overflow. Otherwise returns number of chars written to buffer.
  */
 static inline int
-__json_format_str_to_buf(char *tmp, const int len,
+__json_format_str_to_buf(char *buf, const int len,
 		const char *prefix, const char *str, const char *suffix)
 {
-	int tmpidx = 0;
+	int bufidx = 0;
 
-	while (*prefix != '\0' && tmpidx < len)
-		tmp[tmpidx++] = *prefix++;
-	if (tmpidx >= len)
+	while (*prefix != '\0' && bufidx < len)
+		buf[bufidx++] = *prefix++;
+	if (bufidx >= len)
 		return 0;
 
 	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];
+				buf[bufidx++] = '\\';
+				buf[bufidx++] = control_chars[idx];
 			}
 		} else if (*str == '"' || *str == '\\') {
-			tmp[tmpidx++] = '\\';
-			tmp[tmpidx++] = *str;
+			buf[bufidx++] = '\\';
+			buf[bufidx++] = *str;
 		} else
-			tmp[tmpidx++] = *str;
+			buf[bufidx++] = *str;
 		/* we always need space for (at minimum) 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 (bufidx > len - 2)
 			return 0;
 		str++;
 	}
 
-	while (*suffix != '\0' && tmpidx < len)
-		tmp[tmpidx++] = *suffix++;
-	if (tmpidx >= len)
+	while (*suffix != '\0' && bufidx < len)
+		buf[bufidx++] = *suffix++;
+	if (bufidx >= len)
 		return 0;
 
-	tmp[tmpidx] = '\0';
-	return tmpidx;
+	buf[bufidx] = '\0';
+	return bufidx;
 }
 
 /**
-- 
2.37.2

[PATCH v3 5/5] telemetry: remove VLA in json string format function

[Bruce Richardson]

Since variable length arrays (VLAs) are potentially insecure and
unsupported by some compilers, rework the code to remove their use. As
with previous changes to remove VLAs in the telemetry code, this
function uses two methods to avoid modifying the buffer when adding to
it fails:
* if there are only a few characters in the buffer, save them off to
  restore on failure, then use the buffer as-is,
* otherwise use malloc rather than a VLA to allocate a temporary buffer
  and copy from that on success only.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/test_telemetry_json.c |  2 +-
 lib/telemetry/telemetry_json.h | 19 +++++++++++++++++--
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/app/test/test_telemetry_json.c b/app/test/test_telemetry_json.c
index e81e3a8a98..5617eac540 100644
--- a/app/test/test_telemetry_json.c
+++ b/app/test/test_telemetry_json.c
@@ -129,7 +129,7 @@ test_string_char_escaping(void)
 {
 	static const char str[] = "A string across\ntwo lines and \"with quotes\"!";
 	const char *expected = "\"A string across\\ntwo lines and \\\"with quotes\\\"!\"";
-	char buf[sizeof(str) + 10];
+	char buf[sizeof(str) + 10] = "";
 	int used = 0;
 
 	used = rte_tel_json_str(buf, sizeof(buf), used, str);
diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
index c087b833eb..7999535848 100644
--- a/lib/telemetry/telemetry_json.h
+++ b/lib/telemetry/telemetry_json.h
@@ -134,13 +134,28 @@ __json_format_str_to_buf(char *buf, const int len,
 static inline int
 __json_format_str(char *buf, const int len, const char *prefix, const char *str, const char *suffix)
 {
-	char tmp[len];
 	int ret;
+	char saved[4] = "";
+	char *tmp;
+
+	if (strnlen(buf, sizeof(saved)) < sizeof(saved)) {
+		/* we have only a few bytes in buffer, so save them off to restore on error*/
+		strcpy(saved, buf);
+		ret = __json_format_str_to_buf(buf, len, prefix, str, suffix);
+		if (ret == 0)
+			strcpy(buf, saved); /* restore */
+		return ret;
+	}
+
+	tmp = malloc(len);
+	if (tmp == NULL)
+		return 0;
 
 	ret = __json_format_str_to_buf(tmp, len, prefix, str, suffix);
 	if (ret > 0)
-		strcpy(buf, tmp);
+		strcpy(buf, saved);
 
+	free(tmp);
 	return ret;
 }
 
-- 
2.37.2

Re: [PATCH v2 1/5] telemetry: fix autotest failures on Alpine

[Tyler Retzlaff]

On Wed, Apr 05, 2023 at 04:44:10PM +0100, Bruce Richardson wrote:
> On Alpine linux, the telemetry_data_autotest was failing for the
> test where we had dictionaries embedded in other dictionaries up
> to three levels deep. Indications are that this issue is due to
> excess data being stored on the stack, so replace stack-allocated
> buffer data with dynamically allocated data in the case where we
> are doing recursive processing of telemetry data structures into
> json.
> 
> Bugzilla ID: 1177
> Fixes: c933bb5177ca ("telemetry: support array values in data object")
> Fixes: d2671e642a8e ("telemetry: support dict of dicts")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> ---
Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

(one observation below)

> V2:
>   set '\0' in newly malloc'ed buffer to ensure it always has valid
>   string data.
> ---
>  lib/telemetry/telemetry.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
> index 7bceadcee7..728a0bffd4 100644
> --- a/lib/telemetry/telemetry.c
> +++ b/lib/telemetry/telemetry.c
> @@ -208,7 +208,11 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
>  				break;
>  			case RTE_TEL_CONTAINER:
>  			{
> -				char temp[buf_len];
> +				char *temp = malloc(buf_len);
> +				if (temp == NULL)
> +					break;
> +				*temp = '\0';  /* ensure valid string */
> +
>  				const struct container *cont =
>  						&v->value.container;
>  				if (container_to_json(cont->data,
> @@ -219,6 +223,7 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
>  							v->name, temp);
>  				if (!cont->keep)
>  					rte_tel_data_free(cont->data);
> +				free(temp);
>  				break;
>  			}
>  			}
> @@ -275,7 +280,11 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
>  				break;
>  			case RTE_TEL_CONTAINER:
>  			{
> -				char temp[buf_len];
> +				char *temp = malloc(buf_len);
> +				if (temp == NULL)
> +					break;
> +				*temp = '\0';  /* ensure valid string */
> +
>  				const struct container *cont =
>  						&v->value.container;
>  				if (container_to_json(cont->data,
> @@ -286,6 +295,7 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
>  							v->name, temp);
>  				if (!cont->keep)
>  					rte_tel_data_free(cont->data);
> +				free(temp);

not expressing a preference just noticing that when
RTE_TEL_CONTAINER cases are the last case in the switch sometimes there
is an explicit break; and sometimes not.

>  			}
>  			}
>  		}
> @@ -311,7 +321,11 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
>  						buf_len, used,
>  						d->data.array[i].uval);
>  			else if (d->type == TEL_ARRAY_CONTAINER) {
> -				char temp[buf_len];
> +				char *temp = malloc(buf_len);
> +				if (temp == NULL)
> +					break;
> +				*temp = '\0';  /* ensure valid string */
> +
>  				const struct container *rec_data =
>  						&d->data.array[i].container;
>  				if (container_to_json(rec_data->data,
> @@ -321,6 +335,7 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
>  							buf_len, used, temp);
>  				if (!rec_data->keep)
>  					rte_tel_data_free(rec_data->data);
> +				free(temp);
>  			}
>  		break;
>  	}
> -- 
> 2.37.2

Re: [PATCH v3 1/5] telemetry: fix autotest failures on Alpine

[Tyler Retzlaff]

On Wed, Apr 05, 2023 at 05:03:22PM +0100, Bruce Richardson wrote:
> On Alpine linux, the telemetry_data_autotest was failing for the
> test where we had dictionaries embedded in other dictionaries up
> to three levels deep. Indications are that this issue is due to
> excess data being stored on the stack, so replace stack-allocated
> buffer data with dynamically allocated data in the case where we
> are doing recursive processing of telemetry data structures into
> json.
> 
> Bugzilla ID: 1177
> Fixes: c933bb5177ca ("telemetry: support array values in data object")
> Fixes: d2671e642a8e ("telemetry: support dict of dicts")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> ---
Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

(ack on latest version)

Re: [PATCH v3 2/5] telemetry: remove variable length array in printf fn

[Tyler Retzlaff]

On Wed, Apr 05, 2023 at 05:03:23PM +0100, Bruce Richardson wrote:
> The json_snprintf function, used to add json characters on to a buffer,
> leaving the buffer unmodified in case of error, used a variable length
> array to store the data temporarily while checking for overflow. VLAs
> can be unsafe, and are unsupported by some compilers, so remove use of
> the VLA.
> 
> For the normal case where there is only a small amount of existing text
> in the buffer (<4 chars) to be preserved, save that off temporarily to a
> local array, and restore on error. To handle cases where there is more
> than a few characters in the buffer, we use the existing logic of doing
> the print to a temporary buffer initially and then copying. In this
> case, though we use malloc-allocated buffer rather than VLA.
> 
> Within the unit tests, the "telemetry_data_autotests" test cases - which
> mimic real telemetry use - all exercise the first path. The
> telemetry_json_autotest cases work directly with generating json, and
> use uninitialized buffers so also test the second, malloc-allocated
> buffer, cases.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> ---
Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

Re: [PATCH v3 3/5] telemetry: split out body of json string format fn

[Tyler Retzlaff]

On Wed, Apr 05, 2023 at 05:03:24PM +0100, Bruce Richardson wrote:
> To enable further rework to (efficiently) avoid using variable-length
> arrays, we first separate out the body of the __json_format_str
> function. This means that the actual VLA buffer is in the wrapper
> function, and means we can reuse the actual writing code in multiple
> code paths without duplication.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---

Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

Re: [PATCH v3 4/5] telemetry: rename local variables

[Tyler Retzlaff]

On Wed, Apr 05, 2023 at 05:03:25PM +0100, Bruce Richardson wrote:
> In the newly separated out function, rename "tmp" to "buf" to have more
> meaningful variable names.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> ---
Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

(with suggestions)

> 
> When committing, this patch can be merged with the previous. I've kept
> them separate for now, as it makes reviewing a lot easier.
> ---
>  lib/telemetry/telemetry_json.h | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
> index aada523a27..c087b833eb 100644
> --- a/lib/telemetry/telemetry_json.h
> +++ b/lib/telemetry/telemetry_json.h
> @@ -84,44 +84,44 @@ static const char control_chars[0x20] = {
>   * directly, but returns 0 on overflow. Otherwise returns number of chars written to buffer.
>   */
>  static inline int
> -__json_format_str_to_buf(char *tmp, const int len,
> +__json_format_str_to_buf(char *buf, const int len,
>  		const char *prefix, const char *str, const char *suffix)

does it cascade rubbish into the caller if `len` is made to be type
`size_t` instead of `int`?

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

should be `size_t` instead of `int` type for idx.

>  			if (control_chars[idx] != 0) {
> -				tmp[tmpidx++] = '\\';
> -				tmp[tmpidx++] = control_chars[idx];
> +				buf[bufidx++] = '\\';
> +				buf[bufidx++] = control_chars[idx];
>  			}
>  		} else if (*str == '"' || *str == '\\') {
> -			tmp[tmpidx++] = '\\';
> -			tmp[tmpidx++] = *str;
> +			buf[bufidx++] = '\\';
> +			buf[bufidx++] = *str;
>  		} else
> -			tmp[tmpidx++] = *str;
> +			buf[bufidx++] = *str;
>  		/* we always need space for (at minimum) 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 (bufidx > len - 2)
>  			return 0;
>  		str++;
>  	}
>  
> -	while (*suffix != '\0' && tmpidx < len)
> -		tmp[tmpidx++] = *suffix++;
> -	if (tmpidx >= len)
> +	while (*suffix != '\0' && bufidx < len)
> +		buf[bufidx++] = *suffix++;
> +	if (bufidx >= len)
>  		return 0;
>  
> -	tmp[tmpidx] = '\0';
> -	return tmpidx;
> +	buf[bufidx] = '\0';
> +	return bufidx;
>  }
>  
>  /**
> -- 
> 2.37.2

Re: [PATCH v3 5/5] telemetry: remove VLA in json string format function

[Tyler Retzlaff]

On Wed, Apr 05, 2023 at 05:03:26PM +0100, Bruce Richardson wrote:
> Since variable length arrays (VLAs) are potentially insecure and
> unsupported by some compilers, rework the code to remove their use. As
> with previous changes to remove VLAs in the telemetry code, this
> function uses two methods to avoid modifying the buffer when adding to
> it fails:
> * if there are only a few characters in the buffer, save them off to
>   restore on failure, then use the buffer as-is,
> * otherwise use malloc rather than a VLA to allocate a temporary buffer
>   and copy from that on success only.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---

Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

Re: [PATCH v2 1/5] telemetry: fix autotest failures on Alpine

[Bruce Richardson]

On Fri, Apr 07, 2023 at 12:21:16PM -0700, Tyler Retzlaff wrote:
> On Wed, Apr 05, 2023 at 04:44:10PM +0100, Bruce Richardson wrote:
> > On Alpine linux, the telemetry_data_autotest was failing for the
> > test where we had dictionaries embedded in other dictionaries up
> > to three levels deep. Indications are that this issue is due to
> > excess data being stored on the stack, so replace stack-allocated
> > buffer data with dynamically allocated data in the case where we
> > are doing recursive processing of telemetry data structures into
> > json.
> > 
> > Bugzilla ID: 1177
> > Fixes: c933bb5177ca ("telemetry: support array values in data object")
> > Fixes: d2671e642a8e ("telemetry: support dict of dicts")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > 
> > ---
> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> 
> (one observation below)
> 
> > V2:
> >   set '\0' in newly malloc'ed buffer to ensure it always has valid
> >   string data.
> > ---
<snip>
> > @@ -286,6 +295,7 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
> >  							v->name, temp);
> >  				if (!cont->keep)
> >  					rte_tel_data_free(cont->data);
> > +				free(temp);
> 
> not expressing a preference just noticing that when
> RTE_TEL_CONTAINER cases are the last case in the switch sometimes there
> is an explicit break; and sometimes not.
> 
I won't do a new patch revision just for that, but if I end up doing one
for other reasons I'll try and remember to make it more consistent.

thanks,
/Bruce

Re: [PATCH v3 4/5] telemetry: rename local variables

[Bruce Richardson]

On Fri, Apr 07, 2023 at 12:50:06PM -0700, Tyler Retzlaff wrote:
> On Wed, Apr 05, 2023 at 05:03:25PM +0100, Bruce Richardson wrote:
> > In the newly separated out function, rename "tmp" to "buf" to have more
> > meaningful variable names.
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > 
> > ---
> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> 
> (with suggestions)
> 
> > 
> > When committing, this patch can be merged with the previous. I've kept
> > them separate for now, as it makes reviewing a lot easier.
> > ---
> >  lib/telemetry/telemetry_json.h | 32 ++++++++++++++++----------------
> >  1 file changed, 16 insertions(+), 16 deletions(-)
> > 
> > diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
> > index aada523a27..c087b833eb 100644
> > --- a/lib/telemetry/telemetry_json.h
> > +++ b/lib/telemetry/telemetry_json.h
> > @@ -84,44 +84,44 @@ static const char control_chars[0x20] = {
> >   * directly, but returns 0 on overflow. Otherwise returns number of chars written to buffer.
> >   */
> >  static inline int
> > -__json_format_str_to_buf(char *tmp, const int len,
> > +__json_format_str_to_buf(char *buf, const int len,
> >  		const char *prefix, const char *str, const char *suffix)
> 
> does it cascade rubbish into the caller if `len` is made to be type
> `size_t` instead of `int`?
> 
> >  {
> > -	int tmpidx = 0;
> > +	int bufidx = 0;
> >  
> > -	while (*prefix != '\0' && tmpidx < len)
> > -		tmp[tmpidx++] = *prefix++;
> > -	if (tmpidx >= len)
> > +	while (*prefix != '\0' && bufidx < len)
> > +		buf[bufidx++] = *prefix++;
> > +	if (bufidx >= len)
> >  		return 0;
> >  
> >  	while (*str != '\0') {
> >  		if (*str < (int)RTE_DIM(control_chars)) {
> >  			int idx = *str;  /* compilers don't like char type as index */
> 
> should be `size_t` instead of `int` type for idx.
> 
Agreed. However, trying to keep this as a pure rename only patch, so I
don't plan on fixing that here. If I do a new version of the series, I'll
see if I can work it into the follow-on patch easily.

/Bruce

Re: [PATCH v3 0/5] telemetry: remove variable length arrays

[Thomas Monjalon]

05/04/2023 18:03, Bruce Richardson:
> This patchset introduces a series of changes to remove variable-length
> arrays from the telemetry code. The first patch replaces a VLA with
> malloc memory for the serialization of the json objects contained within
> the main response object, which fixes a crash observed on alpine linux.
> 
> Subsequent patches rework the json printing code to avoid the use of
> temporary buffers where possible, or use malloc-allocated memory where
> not.
> 
> Based off testing with the unit tests for telemetry, json serialization
> for the telemetry callbacks should always use the path where a temporary
> buffer is *not* used, but the allocation-case is kept to ensure that any
> unexpected edge-cases are covered too.
> 
> V3: remove use of non-standard asprintf function in patch 2.
> 
> V2: expand from single fix for Alpine, to general cleanup to remove VLAs
> 
> Bruce Richardson (5):
>   telemetry: fix autotest failures on Alpine
>   telemetry: remove variable length array in printf fn
>   telemetry: split out body of json string format fn
>   telemetry: rename local variables
>   telemetry: remove VLA in json string format function

Applied, thanks.

Re: [PATCH v3 5/5] telemetry: remove VLA in json string format function

[David Marchand]

On Wed, Apr 5, 2023 at 6:05 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> Since variable length arrays (VLAs) are potentially insecure and
> unsupported by some compilers, rework the code to remove their use. As
> with previous changes to remove VLAs in the telemetry code, this
> function uses two methods to avoid modifying the buffer when adding to
> it fails:
> * if there are only a few characters in the buffer, save them off to
>   restore on failure, then use the buffer as-is,
> * otherwise use malloc rather than a VLA to allocate a temporary buffer
>   and copy from that on success only.
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  app/test/test_telemetry_json.c |  2 +-
>  lib/telemetry/telemetry_json.h | 19 +++++++++++++++++--
>  2 files changed, 18 insertions(+), 3 deletions(-)

This change triggers a unit test failure ("interestingly" only with
gcc, I can't reproduce with clang).

$ ninja -C build-gcc && ./build-gcc/app/test/dpdk-test --no-huge -m
2048 --iova=va -- telemetry_json_autotest
ninja: Entering directory `build-gcc'
ninja: no work to do.
EAL: Detected CPU lcores: 16
EAL: Detected NUMA nodes: 1
EAL: Detected shared linkage of DPDK
EAL: Multi-process socket /run/user/114840/dpdk/rte/mp_socket
EAL: Selected IOVA mode 'VA'
APP: HPET is not enabled, using TSC as default timer
RTE>>telemetry_json_autotest
test_basic_array: buf = '["meaning of life",42]', expected =
'["meaning of life",42]'
OK
test_basic_obj: buf = '{"weddings":4,"funerals":1}', expected =
'{"weddings":4,"funerals":1}'
OK
test_overflow_array: buf = '', expected = '["Arsenal","Chelsea"]'
ERROR
Test Failed


I guess we need:

diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
index 7999535848..7a246deacb 100644
--- a/lib/telemetry/telemetry_json.h
+++ b/lib/telemetry/telemetry_json.h
@@ -153,7 +153,7 @@ __json_format_str(char *buf, const int len, const
char *prefix, const char *str,

        ret = __json_format_str_to_buf(tmp, len, prefix, str, suffix);
        if (ret > 0)
-               strcpy(buf, saved);
+               strcpy(buf, tmp);

        free(tmp);
        return ret;


-- 
David Marchand