ovsdb: Fix race for datum JSON string reference counter.

Compaction thread supposed to not change anything in the database
it is working on, since the same data can be accessed by the main
thread at the same time.  However, while converting database rows
to JSON objects, strings in the datum will be cloned using
json_clone(), which is a shallow copy, and that will change the
reference counter for the JSON string object.  If both the main
thread and the compaction thread will clone/destroy the same object
at the same time we may end up with a broken reference counter
leading to a memory leak or use-after free.

Adding a new argument to the database to JSON conversion to prevent
use of shallow copies from the compaction thread.  This way all
the database operations will be truly read-only avoiding the race.

'ovsdb_atom_to_json' and 'ovsdb_datum_to_json' are more widely used,
so creating separate variant for these functions instead of adding
a new argument, to avoid changing a lot of existing code.

Other solution might be to use atomic reference counters, but that
will require API/ABI break, because counter is exposed in public
headers.  Also, we can not easily expose atomic functions, so we'll
need to un-inline reference counting with the associated performance
cost.

Fixes: 3cd2cbd684e0 ("ovsdb: Prepare snapshot JSON in a separate thread.")
Reported-at: https://bugzilla.redhat.com/2133431
Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

5 files changed, 38 insertions, 13 deletions

diff --git a/ovsdb/file.c b/ovsdb/file.c
index ca80c2823..fdc289ad1 100644
--- a/ovsdb/file.c
+++ b/ovsdb/file.c
@@ -52,7 +52,8 @@ static void ovsdb_file_txn_init(struct ovsdb_file_txn *);
 static void ovsdb_file_txn_add_row(struct ovsdb_file_txn *,
                                    const struct ovsdb_row *old,
                                    const struct ovsdb_row *new,
-                                   const unsigned long int *changed);
+                                   const unsigned long int *changed,
+                                   bool allow_shallow_copies);
 
 /* If set to 'true', file transactions will contain difference between
  * datums of old and new rows and not the whole new datum for the column. */
@@ -361,12 +362,19 @@ ovsdb_file_change_cb(const struct ovsdb_row *old,
                      void *ftxn_)
 {
     struct ovsdb_file_txn *ftxn = ftxn_;
-    ovsdb_file_txn_add_row(ftxn, old, new, changed);
+    ovsdb_file_txn_add_row(ftxn, old, new, changed, true);
     return true;
 }
 
+/* Converts the database into transaction JSON representation.
+ * If 'allow_shallow_copies' is false, makes sure that all the JSON
+ * objects in the resulted transaction JSON are separately allocated
+ * objects and not shallow clones of JSON objects already existing
+ * in the database.  Useful when multiple threads are working on the
+ * same database object. */
 struct json *
-ovsdb_to_txn_json(const struct ovsdb *db, const char *comment)
+ovsdb_to_txn_json(const struct ovsdb *db, const char *comment,
+                  bool allow_shallow_copies)
 {
     struct ovsdb_file_txn ftxn;
 
@@ -378,7 +386,8 @@ ovsdb_to_txn_json(const struct ovsdb *db, const char *comment)
         const struct ovsdb_row *row;
 
         HMAP_FOR_EACH (row, hmap_node, &table->rows) {
-            ovsdb_file_txn_add_row(&ftxn, NULL, row, NULL);
+            ovsdb_file_txn_add_row(&ftxn, NULL, row, NULL,
+                                   allow_shallow_copies);
         }
     }
 
@@ -426,7 +435,8 @@ static void
 ovsdb_file_txn_add_row(struct ovsdb_file_txn *ftxn,
                        const struct ovsdb_row *old,
                        const struct ovsdb_row *new,
-                       const unsigned long int *changed)
+                       const unsigned long int *changed,
+                       bool allow_shallow_copies)
 {
     struct json *row;
 
@@ -451,10 +461,20 @@ ovsdb_file_txn_add_row(struct ovsdb_file_txn *ftxn,
                 if (old && use_column_diff) {
                     ovsdb_datum_diff(&datum, &old->fields[idx],
                                      &new->fields[idx], type);
-                    column_json = ovsdb_datum_to_json(&datum, type);
+                    if (allow_shallow_copies) {
+                        column_json = ovsdb_datum_to_json(&datum, type);
+                    } else {
+                        column_json = ovsdb_datum_to_json_deep(&datum, type);
+                    }
                     ovsdb_datum_destroy(&datum, type);
                 } else {
-                    column_json = ovsdb_datum_to_json(&new->fields[idx], type);
+                    if (allow_shallow_copies) {
+                        column_json = ovsdb_datum_to_json(
+                                            &new->fields[idx], type);
+                    } else {
+                        column_json = ovsdb_datum_to_json_deep(
+                                            &new->fields[idx], type);
+                    }
                 }
                 if (!row) {
                     row = json_object_create();
diff --git a/ovsdb/file.h b/ovsdb/file.h
index be4f6ad27..ae90d4fe1 100644
--- a/ovsdb/file.h
+++ b/ovsdb/file.h
@@ -25,7 +25,8 @@ struct ovsdb_txn;
 
 void ovsdb_file_column_diff_disable(void);
 
-struct json *ovsdb_to_txn_json(const struct ovsdb *, const char *comment);
+struct json *ovsdb_to_txn_json(const struct ovsdb *, const char *comment,
+                               bool allow_shallow_copies);
 struct json *ovsdb_file_txn_to_json(const struct ovsdb_txn *);
 struct json *ovsdb_file_txn_annotate(struct json *, const char *comment);
 struct ovsdb_error *ovsdb_file_txn_from_json(struct ovsdb *,
diff --git a/ovsdb/ovsdb-tool.c b/ovsdb/ovsdb-tool.c
index df2e373c3..60f353197 100644
--- a/ovsdb/ovsdb-tool.c
+++ b/ovsdb/ovsdb-tool.c
@@ -304,7 +304,7 @@ do_create_cluster(struct ovs_cmdl_context *ctx)
 
         struct ovsdb *ovsdb = ovsdb_file_read(src_file_name, false);
         char *comment = xasprintf("created from %s", src_file_name);
-        data = ovsdb_to_txn_json(ovsdb, comment);
+        data = ovsdb_to_txn_json(ovsdb, comment, true);
         free(comment);
         schema = ovsdb_schema_clone(ovsdb->schema);
         ovsdb_destroy(ovsdb);
@@ -359,7 +359,8 @@ write_standalone_db(const char *file_name, const char *comment,
 
     error = ovsdb_log_write_and_free(log, ovsdb_schema_to_json(db->schema));
     if (!error) {
-        error = ovsdb_log_write_and_free(log, ovsdb_to_txn_json(db, comment));
+        error = ovsdb_log_write_and_free(log,
+                                         ovsdb_to_txn_json(db, comment, true));
     }
     ovsdb_log_close(log);
 
diff --git a/ovsdb/ovsdb.c b/ovsdb/ovsdb.c
index 8cbefbe3d..1c011fab0 100644
--- a/ovsdb/ovsdb.c
+++ b/ovsdb/ovsdb.c
@@ -585,7 +585,9 @@ compaction_thread(void *aux)
     struct json *data;
 
     VLOG_DBG("%s: Compaction thread started.", state->db->name);
-    data = ovsdb_to_txn_json(state->db, "compacting database online");
+    data = ovsdb_to_txn_json(state->db, "compacting database online",
+                             /* Do not allow shallow copies to avoid races. */
+                             false);
     state->data = json_serialized_object_create(data);
     json_destroy(data);
 
@@ -633,7 +635,8 @@ ovsdb_snapshot(struct ovsdb *db, bool trim_memory OVS_UNUSED)
     if (!applied_index) {
         /* Parallel compaction is not supported for standalone databases. */
         state = xzalloc(sizeof *state);
-        state->data = ovsdb_to_txn_json(db, "compacting database online");
+        state->data = ovsdb_to_txn_json(db,
+                                        "compacting database online", true);
         state->schema = ovsdb_schema_to_json(db->schema);
     } else if (ovsdb_snapshot_ready(db)) {
         xpthread_join(db->snap_state->thread, NULL);
diff --git a/ovsdb/trigger.c b/ovsdb/trigger.c
index 7d3003bca..01bb80e28 100644
--- a/ovsdb/trigger.c
+++ b/ovsdb/trigger.c
@@ -282,7 +282,7 @@ ovsdb_trigger_try(struct ovsdb_trigger *t, long long int now)
 
             /* Make the new copy into a transaction log record. */
             struct json *txn_json = ovsdb_to_txn_json(
-                newdb, "converted by ovsdb-server");
+                newdb, "converted by ovsdb-server", true);
 
             /* Propose the change. */
             t->progress = ovsdb_txn_propose_schema_change(