[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.richard...@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