Fix mtev_dyn_buffer_add_vprintf Bug (#919)

There was a bug in mtev_dyn_buffer_add_vprintf where we could call vsnprintf with a va_list twice without a copy. This has undefined behavior, as the call to vsnprintf is destructive. Fixed it so that it does a copy now. However, that copy could have performance impacts, so also added a new function, mtev_dyn_buffer_maybe_add_vprintf, that will not try to increase the buffer size and requires the caller to handle it in order to avoid the extra copy.

ChangeLog.md

@@ -4,16 +4,24 @@
 
 ## 2.6
 
+ * Added new function, `mtev_dyn_buffer_maybe_add_vprintf`, that will not try to
+   expand the buffer and will return how many more bytes are needed.
+ * Fix bug in `mtev_dyn_buffer_add_vprintf` where the va_list argument could be used
+   after being modified. This change involves making a copy of the argument list
+   every time so it could impact performance; recommend using
+   `mtev_dyn_buffer_maybe_add_vprintf` and handling the buffer manually on failure
+   to avoid the extra copy if you plan on using it in a hot path.
+
 ## 2.6.0
 
-* Add various hooks into `mtev_clustering` to allow applications to use custom fields.
+ * Add various hooks into `mtev_clustering` to allow applications to use custom fields.
 
 ## 2.5
 
 ## 2.5.4
 
- * Fix issue in mtev_net_heartbeat where there would be attempts to use invalid messages
- * Improve logging in mtev_net_heartbeat
+ * Fix issue in `mtev_net_heartbeat` where there would be attempts to use invalid messages
+ * Improve logging in `mtev_net_heartbeat`
 
 ### 2.5.3
 

src/utils/mtev_dyn_buffer.c

@@ -1,4 +1,5 @@
 #include "mtev_dyn_buffer.h"
+#include "mtev_log.h"
 
 #include <stdio.h>
 #include <stdarg.h>
@@ -41,22 +42,47 @@ inline void
 mtev_dyn_buffer_add_vprintf(mtev_dyn_buffer_t *buf, const char *format, va_list args)
 {
   int needed, available;
+  va_list arg_copy;
 
+  // vsnprirntf can be called twice in this function and calling it alters the va_list
+  // argument in a destructive manner, so we need to make a copy
+  va_copy(arg_copy, args);
   available = mtev_dyn_buffer_size(buf) - mtev_dyn_buffer_used(buf);
   needed = vsnprintf((char *)buf->pos, available, format, args);
   if (needed > (available - 1)) {
     mtev_dyn_buffer_ensure(buf, needed + 1); /* ensure we have space for the trailing NUL too */
-    needed = vsnprintf((char *)buf->pos, needed + 1, format, args);
+    needed = vsnprintf((char *)buf->pos, needed + 1, format, arg_copy);
   }
   buf->pos += needed;
+  va_end(arg_copy);
+}
+
+inline int
+mtev_dyn_buffer_maybe_add_vprintf(mtev_dyn_buffer_t *buf, const char *format, va_list args) {
+  int available = mtev_dyn_buffer_size(buf) - mtev_dyn_buffer_used(buf);
+  int needed = vsnprintf((char *)buf->pos, available, format, args);
+  if (needed > (available - 1)) {
+    if (available > 0) {
+      *(buf->pos) = 0;
+    }
+    return needed + 1;
+  }
+  buf->pos += needed;
+  return 0;
 }
 
 inline void
 mtev_dyn_buffer_add_printf(mtev_dyn_buffer_t *buf, const char *format, ...)
 {
   va_list args;
   va_start(args, format);
-  mtev_dyn_buffer_add_vprintf(buf, format, args);
+  int needed = mtev_dyn_buffer_maybe_add_vprintf(buf, format, args);
+  if (needed) {
+    mtev_dyn_buffer_ensure(buf, needed); /* ensure we have space for the trailing NUL too */
+    va_end(args);
+    va_start(args, format);
+    mtevAssert(mtev_dyn_buffer_maybe_add_vprintf(buf, format, args) == 0);
+  }
   va_end(args);
 }
 

src/utils/mtev_dyn_buffer.h

@@ -86,7 +86,7 @@ API_EXPORT(void)
   mtev_dyn_buffer_add_json_string(mtev_dyn_buffer_t *buf, uint8_t *data, size_t len, int sol);
 
 /*! \fn void mtev_dyn_buffer_add_printf(mtev_dyn_buffer_t *buf, const char *format, ...)
-    \brief add data to the dyn_buffer using printf semantics.
+    \brief add data to the dyn_buffer using printf semantics
     \param buf the buffer to add to.
     \param format the printf style format string
     \param args printf arguments
@@ -100,20 +100,39 @@ API_EXPORT(void)
   mtev_dyn_buffer_add_printf(mtev_dyn_buffer_t *buf, const char *format, ...);
 
 
-/*! \fn void mtev_dyn_buffer_add_printf(mtev_dyn_buffer_t *buf, const char *format, ...)
-    \brief add data to the dyn_buffer using printf semantics.
+/*! \fn void mtev_dyn_buffer_add_vprintf(mtev_dyn_buffer_t *buf, const char *format, va_list arg)
+    \brief add data to the dyn_buffer using printf semantics and expand the buffer if necessary
     \param buf the buffer to add to.
     \param format the printf style format string
-    \param args printf arguments
+    \param arg the variadic printf arguments
     
     This does NUL terminate the format string but does not advance the write_pointer past
     the NUL.  Basically, the last mtev_dyn_buffer_add_printf will leave the resultant
     data NUL terminated.
-    
+
+    Due to taking a va_list as an argument and possibly needing to use the list twice,
+    this function has to make a copy of the argument. In hot paths, this could have
+    performance impacts. It is recommended to use mtev_dyn_buffer_maybe_add_vprintf and
+    handle the buffer yourself if performance is a factor to avoid this copy.
  */
 API_EXPORT(void)
   mtev_dyn_buffer_add_vprintf(mtev_dyn_buffer_t *buf, const char *format, va_list arg);
 
+/*! \fn void mtev_dyn_buffer_maybe_add_vprintf(mtev_dyn_buffer_t *buf, const char *format, va_list arg)
+    \brief add data to the dyn_buffer using printf semantics if there's enough space
+    \param buf the buffer to add to.
+    \param format the printf style format string
+    \param arg the variadic printf arguments
+    \return 0 on success, the number of bytes the buffer needs to expand by on failure
+
+    This does NUL terminate the format string but does not advance the write_pointer past
+    the NUL.  Basically, the last mtev_dyn_buffer_add_printf will leave the resultant
+    data NUL terminated.
+    If the data could not be written, the pointer at `pos` will be set to NULL.
+ */
+API_EXPORT(int)
+  mtev_dyn_buffer_maybe_add_vprintf(mtev_dyn_buffer_t *buf, const char *format, va_list args);
+
 /*! \fn void mtev_dyn_buffer_ensure(mtev_dyn_buffer_t *buf, size_t len)
     \brief possibly grow the dyn_buffer so it can fit len bytes
     \param buf the buffer to ensure

src/utils/mtev_log.c

@@ -2135,7 +2135,13 @@ add_to_jsonf(int nelem, mtev_dyn_buffer_t *buff,
   mtev_dyn_buffer_init(&scratch);
   va_list args;
   va_start(args, fmt);
-  mtev_dyn_buffer_add_vprintf(&scratch, fmt, args);
+  int needed = mtev_dyn_buffer_maybe_add_vprintf(&scratch, fmt, args);
+  if (needed) {
+    mtev_dyn_buffer_ensure(&scratch, needed);
+    va_end(args);
+    va_start(args, fmt);
+    mtevAssert(mtev_dyn_buffer_maybe_add_vprintf(&scratch, fmt, args) == 0);
+  }
   va_end(args);
   int rv = add_to_json(nelem, buff, key, str, 
                        (const char *)mtev_dyn_buffer_data(&scratch));