[v3,2/5] telemetry: remove variable length array in printf fn
Checks
Commit Message
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(-)
Comments
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>
@@ -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 */
}