Merge pull request #1123 from alltilla/filterx-fix-indirect-object-sync

FilterX: fix borrowed/indirect objects getting overwritten by scope syncs

Author: MrAnno

lib/filterx/expr-template.c

@@ -53,7 +53,7 @@ _eval_template(FilterXExpr *s)
    * expressions, thus the FilterXObject is shorter lived than the scratch
    * buffer.  */
 
-  return filterx_message_value_new_borrowed(value->str, value->len, t);
+  return filterx_message_value_new_indirect(value->str, value->len, t);
 }
 
 static void

lib/filterx/filterx-object.h

@@ -204,7 +204,7 @@ struct _FilterXObject
    *
    */
   guint weak_referenced:1, is_dirty:1, allocator_used:1, floating_ref:1, early_allocation:1, early_allocation_checked:1,
-        flags:5;
+        is_nvtable_backed:1, flags:5;
   volatile guint32 hash;
   FilterXType *type;
 };
@@ -756,6 +756,12 @@ filterx_object_dup(FilterXObject *self)
   return self->type->clone(self);
 }
 
+static inline gboolean
+filterx_object_is_nvtable_backed(FilterXObject *self)
+{
+  return self->is_nvtable_backed;
+}
+
 static inline FilterXObject *
 filterx_object_copy(FilterXObject *self)
 {

lib/filterx/filterx-scope.c

@@ -228,6 +228,36 @@ filterx_scope_foreach_variable_readonly(FilterXScope *self, FilterXScopeForeachF
   return result;
 }
 
+static void
+_filterx_scope_make_object_direct(FilterXObject **object)
+{
+  if (!(*object) || !filterx_object_is_nvtable_backed(*object))
+    return;
+
+  FilterXObject *direct = filterx_object_dup(*object);
+  filterx_object_unref(*object);
+  *object = direct;
+}
+
+static void
+_filterx_scope_make_nvtable_backed_variables_direct(FilterXScope *self)
+{
+  for (FilterXScope *scope = self;
+       scope && filterx_scope_is_dirty(scope) && !filterx_scope_is_fork_point(scope);
+       scope = scope->parent_scope)
+    {
+      for (guint32 i = 0; i < scope->variables_size; i++)
+        {
+          FilterXVariable *v = &scope->variables[i];
+          if (!v->variable_type)
+            continue;
+
+          if (filterx_variable_is_message_tied(v) || filterx_variable_is_declared(v))
+            _filterx_scope_make_object_direct(&v->value);
+        }
+    }
+}
+
 /*
  * 1) sync objects to message
  * 2) drop undeclared objects
@@ -250,6 +280,13 @@ filterx_scope_sync(FilterXScope *self, LogMessage **pmsg, const LogPathOptions *
       return;
     }
 
+  /*
+   * This has to run before the sync below writes anything back to the message:
+   * a write may overwrite the NVTable entry that a later, not-yet-duplicated
+   * variable still points into.
+   */
+  _filterx_scope_make_nvtable_backed_variables_direct(self);
+
   GString *buffer = NULL;
 
   gint msg_generation = self->msg->generation;

lib/filterx/object-message-value.c

@@ -399,7 +399,7 @@ _len(FilterXObject *s, guint64 *len)
 
 /* NOTE: the caller must ensure that repr lives as long as the constructed object, avoids copying */
 FilterXObject *
-filterx_message_value_new_borrowed(const gchar *repr, gssize repr_len, LogMessageValueType type)
+filterx_message_value_new_indirect(const gchar *repr, gssize repr_len, LogMessageValueType type)
 {
   FilterXMessageValue *self = filterx_new_object(FilterXMessageValue);
 
@@ -420,7 +420,7 @@ filterx_message_value_new(const gchar *repr, gssize repr_len, LogMessageValueTyp
   gchar *buf = g_malloc(len + 1);
   memcpy(buf, repr, len);
   buf[len] = 0;
-  FilterXMessageValue *self = (FilterXMessageValue *) filterx_message_value_new_borrowed(buf, len, type);
+  FilterXMessageValue *self = (FilterXMessageValue *) filterx_message_value_new_indirect(buf, len, type);
   self->buf = buf;
   return &self->super;
 }
@@ -429,7 +429,7 @@ filterx_message_value_new(const gchar *repr, gssize repr_len, LogMessageValueTyp
 FilterXObject *
 filterx_message_value_new_ref(gchar *repr, gssize repr_len, LogMessageValueType type)
 {
-  FilterXMessageValue *self = (FilterXMessageValue *) filterx_message_value_new_borrowed(repr, repr_len, type);
+  FilterXMessageValue *self = (FilterXMessageValue *) filterx_message_value_new_indirect(repr, repr_len, type);
   self->buf = repr;
   return &self->super;
 }
@@ -472,13 +472,20 @@ filterx_extract_object_from_logmsg(LogMessage *msg, NVHandle handle)
   const gchar *value = log_msg_get_value_if_set_with_type(msg, handle, &value_len, &t);
   if (!value)
     return NULL;
+
+  FilterXObject *self;
   switch (t)
     {
     case LM_VT_STRING:
-      return filterx_string_new_borrowed(value, value_len);
+      self = filterx_string_new_indirect(value, value_len);
+      break;
     case LM_VT_NULL:
       return filterx_null_new();
     default:
-      return filterx_message_value_new_borrowed(value, value_len, t);
+      self = filterx_message_value_new_indirect(value, value_len, t);
+      break;
     }
+
+  self->is_nvtable_backed = TRUE;
+  return self;
 }

lib/filterx/object-message-value.h

@@ -37,7 +37,7 @@ typedef struct _FilterXMessageValue
 
 FILTERX_DECLARE_TYPE(message_value);
 
-FilterXObject *filterx_message_value_new_borrowed(const gchar *repr, gssize repr_len, LogMessageValueType type);
+FilterXObject *filterx_message_value_new_indirect(const gchar *repr, gssize repr_len, LogMessageValueType type);
 FilterXObject *filterx_message_value_new_ref(gchar *repr, gssize repr_len, LogMessageValueType type);
 FilterXObject *filterx_message_value_new(const gchar *repr, gssize repr_len, LogMessageValueType type);
 

lib/filterx/object-string.c

@@ -93,8 +93,8 @@ _free(FilterXObject *s)
   FilterXString *self = (FilterXString *) s;
   if (self->super.flags & FILTERX_STRING_FLAG_STR_ALLOCATED)
     g_free((gchar *) self->str);
-  else if (self->super.flags & FILTERX_STRING_FLAG_STR_BORROWED_SLICE)
-    filterx_object_unref(self->storage.slice);
+  else if (self->super.flags & FILTERX_STRING_FLAG_STR_INDIRECT)
+    filterx_object_unref(self->storage.indirect);
   filterx_object_free_method(s);
 }
 
@@ -393,15 +393,16 @@ filterx_string_new_from_json_literal(const gchar *str, gssize str_len)
 }
 
 FilterXObject *
-_filterx_string_new_slice_from_borrowed_str_and_len(FilterXObject *object, const gchar *str, gsize str_len)
+_filterx_string_new_indirect_from_str_and_len(FilterXObject *object, const gchar *str, gsize str_len)
 {
   FilterXString *self = filterx_new_object(FilterXString);
   filterx_object_init_instance(&self->super, &FILTERX_TYPE_NAME(string));
 
-  self->super.flags |= FILTERX_STRING_FLAG_STR_BORROWED_SLICE;
+  self->super.flags |= FILTERX_STRING_FLAG_STR_INDIRECT;
+  self->super.is_nvtable_backed = object && filterx_object_is_nvtable_backed(object);
   self->str = str;
   self->str_len = str_len;
-  self->storage.slice = filterx_object_ref(object);
+  self->storage.indirect = filterx_object_ref(object);
   return &self->super;
 }
 
@@ -421,7 +422,7 @@ _filterx_string_new_slice_from_non_string(FilterXObject *object, gsize start, gs
   if (cached)
     return cached;
 
-  return _filterx_string_new_slice_from_borrowed_str_and_len(object, str, str_len);
+  return _filterx_string_new_indirect_from_str_and_len(object, str, str_len);
 }
 
 FilterXObject *

lib/filterx/object-string.h

@@ -31,8 +31,9 @@
 /* the string holds a value that does not need escaping when generating a JSON string literal */
 #define FILTERX_STRING_FLAG_NO_JSON_ESCAPE_NEEDED  0x02
 
-/* string is borrowing its payload from another string */
-#define FILTERX_STRING_FLAG_STR_BORROWED_SLICE     0x04
+/* the string borrows its payload from another object (storage.indirect) instead of owning it,
+ * similar to NVEntry's indirect in lib/logmsg/nvtable.h */
+#define FILTERX_STRING_FLAG_STR_INDIRECT           0x04
 
 /* cache indices */
 enum
@@ -61,7 +62,7 @@ struct _FilterXString
   guint32 str_len;
   union
   {
-    FilterXObject *slice;
+    FilterXObject *indirect;
     gchar bytes[0];
   } storage;
 };
@@ -225,7 +226,7 @@ filterx_string_new(const gchar *str, gssize str_len)
 }
 
 /* slow paths */
-FilterXObject *_filterx_string_new_slice_from_borrowed_str_and_len(FilterXObject *object, const gchar *str,
+FilterXObject *_filterx_string_new_indirect_from_str_and_len(FilterXObject *object, const gchar *str,
     gsize str_len);
 FilterXObject *_filterx_string_new_slice_from_non_string(FilterXObject *object, gsize start, gsize end);
 
@@ -249,16 +250,16 @@ filterx_string_new_slice(FilterXObject *object, gsize start, gsize end)
         return cached;
       if (slice_len < FILTERX_STRING_SHORT_SLICE_LIMIT)
         return _filterx_string_new(slice_start, slice_len);
-      return _filterx_string_new_slice_from_borrowed_str_and_len(object, slice_start, slice_len);
+      return _filterx_string_new_indirect_from_str_and_len(object, slice_start, slice_len);
     }
 
   return _filterx_string_new_slice_from_non_string(object, start, end);
 }
 
 static inline FilterXObject *
-filterx_string_new_borrowed(const gchar *str, gssize str_len)
+filterx_string_new_indirect(const gchar *str, gssize str_len)
 {
-  return _filterx_string_new_slice_from_borrowed_str_and_len(NULL, str, str_len);
+  return _filterx_string_new_indirect_from_str_and_len(NULL, str, str_len);
 }
 
 void filterx_string_global_init(void);

lib/filterx/tests/test_object_message.c

@@ -42,7 +42,7 @@ Test(filterx_message, test_filterx_object_message_marshals_to_the_stored_values)
 
   gchar borrowed_value[] = "string";
 
-  fobj = filterx_message_value_new_borrowed(borrowed_value, -1, LM_VT_STRING);
+  fobj = filterx_message_value_new_indirect(borrowed_value, -1, LM_VT_STRING);
   assert_marshaled_object(fobj, "string", LM_VT_STRING);
   borrowed_value[0]++;
   assert_marshaled_object(fobj, "ttring", LM_VT_STRING);
@@ -65,7 +65,7 @@ Test(filterx_message, test_filterx_object_value_maps_to_the_right_json_value)
 
   gchar borrowed_value[] = "string";
 
-  fobj = filterx_message_value_new_borrowed(borrowed_value, -1, LM_VT_STRING);
+  fobj = filterx_message_value_new_indirect(borrowed_value, -1, LM_VT_STRING);
   assert_object_json_equals(fobj, "\"string\"");
   borrowed_value[0]++;
   assert_object_json_equals(fobj, "\"ttring\"");

lib/filterx/tests/test_object_string.c

@@ -80,7 +80,7 @@ Test(filterx_string, test_string_taking_a_short_slice_of_another_string)
   FilterXObject *str2 = filterx_string_new_slice(str, 1, 3);
 
   assert_object_json_equals(str2, "\"bc\"");
-  cr_assert((str2->flags & FILTERX_STRING_FLAG_STR_BORROWED_SLICE) == 0);
+  cr_assert((str2->flags & FILTERX_STRING_FLAG_STR_INDIRECT) == 0);
   filterx_object_unref(str2);
   filterx_object_unref(str);
 }
@@ -91,7 +91,7 @@ Test(filterx_string, test_string_taking_a_long_slice_of_another_string)
   FilterXObject *str2 = filterx_string_new_slice(str, 1, 15);
 
   assert_object_json_equals(str2, "\"123456789abcde\"");
-  cr_assert((str2->flags & FILTERX_STRING_FLAG_STR_BORROWED_SLICE) != 0);
+  cr_assert((str2->flags & FILTERX_STRING_FLAG_STR_INDIRECT) != 0);
   filterx_object_unref(str2);
   filterx_object_unref(str);
 }
@@ -107,6 +107,30 @@ Test(filterx_string, test_string_taking_an_cached_string_slice_results_in_a_cach
   filterx_object_unref(str);
 }
 
+Test(filterx_string, test_string_slice_propagates_nvtable_backed_marking)
+{
+  /* simulate a value pulled from the message NVTable (the marking is normally
+   * done by filterx_extract_object_from_logmsg(), not by the constructor) */
+  FilterXObject *nvtable_backed = filterx_string_new_indirect("0123456789abcdef", 16);
+  nvtable_backed->is_nvtable_backed = TRUE;
+
+  /* a slice of an NVTable-backed string is itself NVTable-backed */
+  FilterXObject *slice = filterx_string_new_slice(nvtable_backed, 1, 15);
+  cr_assert(filterx_object_is_nvtable_backed(slice));
+
+  /* a slice of a self-owned string is not NVTable-backed, even though it
+   * borrows the payload */
+  FilterXObject *owned = filterx_string_new_take(g_strdup("0123456789abcdef"), 16);
+  FilterXObject *owned_slice = filterx_string_new_slice(owned, 1, 15);
+  cr_assert(!filterx_object_is_nvtable_backed(owned_slice));
+  cr_assert((owned_slice->flags & FILTERX_STRING_FLAG_STR_INDIRECT) != 0);
+
+  filterx_object_unref(slice);
+  filterx_object_unref(nvtable_backed);
+  filterx_object_unref(owned_slice);
+  filterx_object_unref(owned);
+}
+
 static void
 _translate_to_incremented(gchar *target, const gchar *source, gsize *len)
 {

news/bugfix-1123.md

@@ -0,0 +1 @@
+`filterx`: Fixed a bug where a value read from a log message field could be corrupted when that field was overwritten later in the same flow.