Address JSON thread safety issues.

In tracking down some unit tests failures, I ended up reading the fine
print[1] regarding Jansson's thread safety.

In short:
 1. Ref-counting is non-atomic.
 2. json_dumps() and friends are not thread safe.

This patch adds locking where necessary to our ast_json_* wrapper API,
with documentation in json.h describing the thread safety limitations of
the API.

 [1]: http://www.digip.org/jansson/doc/2.4/portability.html#thread-safety

Review: https://reviewboard.asterisk.org/r/2716/


git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@396119 65c4cc65-6c06-0410-ace0-fbb531ad65f3

6 files changed, 236 insertions, 17 deletions

diff --git a/include/asterisk/json.h b/include/asterisk/json.h
index a735fd36b..15e1108da 100644
--- a/include/asterisk/json.h
+++ b/include/asterisk/json.h
@@ -43,6 +43,37 @@
  * wrap them with json_ref() when passing them to other \c ast_json_*()
  * functions.
  *
+ * \par Thread Safety
+ *
+ * Jansson (as of 2.4) provides fairly weak thread safety guarantees. The
+ * Asterisk wrapper improves upon that slightly. The remaining refcounting
+ * problems are issues when slicing/sharing/mixing instances between JSON
+ * objects and arrays, which we avoid.
+ *
+ * The \c ast_json_dump_* functions are thread safe for multiple concurrent
+ * dumps of the same object, so long as the concurrent dumps start from the same
+ * \c root object. But if an object is shared by other JSON objects/arrays, then
+ * concurrent dumps of the outer objects/arrays are not thread safe. This can be
+ * avoided by using ast_json_deep_copy() when sharing JSON instances between
+ * objects.
+ *
+ * The ast_json_ref() and ast_json_unref() functions are thread safe. Since the
+ * Asterisk wrapper exclusively uses the reference stealing API, Jansson won't
+ * be performing many refcount modifications behind our backs. There are a few
+ * exceptions.
+ *
+ * The first is the transitive json_decref() that occurs when \ref
+ * AST_JSON_OBJECT and \ref AST_JSON_ARRAY instances are deleted. This can be
+ * avoided by using ast_json_deep_copy() when sharing JSON instances between
+ * objects.
+ *
+ * The second is when using the reference borrowing specifier in
+ * ast_json_pack() (capital \c O). This can be avoided by using the reference
+ * stealing specifier (lowercase \c o) and wrapping the JSON object parameter
+ * with ast_json_ref() for an explicit ref-bump.
+ *
+ * \par Example code
+ *
  * \code
  *	// Example of how to use the Asterisk JSON API
  *	static struct ast_json *foo(void) {
@@ -107,6 +138,20 @@ void ast_json_set_alloc_funcs(void *(*malloc_fn)(size_t), void (*free_fn)(void*)
 void ast_json_reset_alloc_funcs(void);
 
 /*!
+ * \brief Asterisk's custom JSON allocator. Exposed for use by unit tests.
+ * \since 12.0.0
+ * \internal
+ */
+void *ast_json_malloc(size_t size);
+
+/*!
+ * \brief Asterisk's custom JSON allocator. Exposed for use by unit tests.
+ * \since 12.0.0
+ * \internal
+ */
+void ast_json_free(void *p);
+
+/*!
  * \struct ast_json
  * \brief Abstract JSON element (object, array, string, int, ...).
  * \since 12.0.0
@@ -683,13 +728,23 @@ enum ast_json_encoding_format
 	AST_JSON_PRETTY,
 };
 
+/*!
+ * \brief Encode a JSON value to a compact string.
+ * \since 12.0.0
+ *
+ * Returned string must be freed by calling ast_json_free().
+ *
+ * \param root JSON value.
+ * \return String encoding of \a root.
+ * \return \c NULL on error.
+ */
 #define ast_json_dump_string(root) ast_json_dump_string_format(root, AST_JSON_COMPACT)
 
 /*!
  * \brief Encode a JSON value to a string.
  * \since 12.0.0
  *
- * Returned string must be freed by calling ast_free().
+ * Returned string must be freed by calling ast_json_free().
  *
  * \param root JSON value.
  * \param format encoding format type.
diff --git a/main/cel.c b/main/cel.c
index befb2b257..ae42a443d 100644
--- a/main/cel.c
+++ b/main/cel.c
@@ -638,7 +638,7 @@ struct ast_event *ast_cel_create_event(struct ast_channel_snapshot *snapshot,
 		struct ast_json *extra, const char *peer_name)
 {
 	struct timeval eventtime = ast_tvnow();
-	RAII_VAR(char *, extra_txt, NULL, ast_free);
+	RAII_VAR(char *, extra_txt, NULL, ast_json_free);
 	if (extra) {
 		extra_txt = ast_json_dump_string(extra);
 	}
diff --git a/main/json.c b/main/json.c
index 1b0e41275..8658502b3 100644
--- a/main/json.c
+++ b/main/json.c
@@ -20,8 +20,8 @@
  *
  * \brief JSON abstraction layer.
  *
- * This is a very thin wrapper around the Jansson API. For more details on it, see its
- * docs at http://www.digip.org/jansson/doc/2.4/apiref.html.
+ * This is a very thin wrapper around the Jansson API. For more details on it,
+ * see its docs at http://www.digip.org/jansson/doc/2.4/apiref.html.
  *
  * \author David M. Lee, II <dlee@digium.com>
  */
@@ -46,20 +46,148 @@ ASTERISK_FILE_VERSION(__FILE__, "$Revision$")
 #include <jansson.h>
 #include <time.h>
 
+/*! \brief Magic number, for safety checks. */
+#define JSON_MAGIC 0x1541992
+
+/*! \brief Internal structure for allocated memory blocks */
+struct json_mem {
+	/*! Magic number, for safety checks */
+	uint32_t magic;
+	/*! Mutext for locking this memory block */
+	ast_mutex_t mutex;
+	/*! Linked list pointer for the free list */
+	AST_LIST_ENTRY(json_mem) list;
+	/*! Data section of the allocation; void pointer for proper alignment */
+	void *data[];
+};
+
+/*! \brief Free a \ref json_mem block. */
+static void json_mem_free(struct json_mem *mem)
+{
+	mem->magic = 0;
+	ast_mutex_destroy(&mem->mutex);
+	ast_free(mem);
+}
+
+/*!
+ * \brief Get the \ref json_mem block for a pointer allocated via
+ * ast_json_malloc().
+ *
+ * This function properly handles Jansson singletons (null, true, false), and
+ * \c NULL.
+ *
+ * \param p Pointer, usually to a \c json_t or \ref ast_json.
+ * \return \ref json_mem object with extra allocation info.
+ */
+static inline struct json_mem *to_json_mem(void *p)
+{
+	struct json_mem *mem;
+	/* Avoid ref'ing the singleton values */
+	if (p == NULL || p == json_null() || p == json_true() ||
+		p == json_false()) {
+		return NULL;
+	}
+	mem = (struct json_mem *)((char *) (p) - sizeof(*mem));
+	ast_assert(mem->magic == JSON_MAGIC);
+	return mem;
+}
+
+/*!
+ * \brief Lock an \ref ast_json instance.
+ *
+ * If \a json is an immutable singleton (null, true, false), this function
+ * safely ignores it and returns \c NULL. Otherwise, \a json must have been
+ * allocates using ast_json_malloc().
+ *
+ * \param json JSON instance to lock.
+ * \return \ref Corresponding \ref json_mem block.
+ * \return \c NULL if \a json was not allocated.
+ */
+static struct json_mem *json_mem_lock(struct ast_json *json)
+{
+	struct json_mem *mem = to_json_mem(json);
+	if (!mem) {
+		return NULL;
+	}
+	ast_mutex_lock(&mem->mutex);
+	return mem;
+}
+
+/*!
+ * \brief Unlock a \ref json_mem instance.
+ *
+ * \param mem \ref json_mem, usually returned from json_mem_lock().
+ */
+static void json_mem_unlock(struct json_mem *mem)
+{
+	if (!mem) {
+		return;
+	}
+	ast_mutex_unlock(&mem->mutex);
+}
+
 /*!
- * \brief Function wrapper around ast_malloc macro.
+ * \brief Scoped lock for a \ref ast_json instance.
+ *
+ * \param json JSON instance to lock.
  */
-static void *json_malloc(size_t size)
+#define SCOPED_JSON_LOCK(json)				\
+	RAII_VAR(struct json_mem *, __mem_ ## __LINE__, \
+		json_mem_lock(json), json_mem_unlock)
+
+void *ast_json_malloc(size_t size)
 {
-	return ast_malloc(size);
+	struct json_mem *mem = ast_malloc(size + sizeof(*mem));
+	if (!mem) {
+		return NULL;
+	}
+	mem->magic = JSON_MAGIC;
+	ast_mutex_init(&mem->mutex);
+	return mem->data;
 }
 
+AST_THREADSTORAGE(json_free_list_ts);
+
+/*!
+ * \brief Struct for a linked list of \ref json_mem.
+ */
+AST_LIST_HEAD_NOLOCK(json_mem_list, json_mem);
+
 /*!
- * \brief Function wrapper around ast_free macro.
+ * \brief Thread local list of \ref json_mem blocks to free at the end of an
+ * unref.
  */
-static void json_free(void *p)
+static struct json_mem_list *json_free_list(void)
+{
+	return ast_threadstorage_get(&json_free_list_ts,
+		sizeof(struct json_mem_list));
+}
+
+void ast_json_free(void *p)
 {
-	ast_free(p);
+	struct json_mem *mem;
+	struct json_mem_list *free_list;
+	mem = to_json_mem(p);
+
+	if (!mem) {
+		return;
+	}
+
+	/* Since the unref is holding a lock in mem, we can't free it
+	 * immediately. Store it off on a thread local list to be freed by
+	 * ast_json_unref().
+	 */
+	free_list = json_free_list();
+	if (!free_list) {
+		ast_log(LOG_ERROR, "Error allocating free list\n");
+		ast_assert(0);
+		/* It's not ideal to free the memory immediately, but that's the
+		 * best we can do if the threadlocal allocation fails */
+		json_mem_free(mem);
+		return;
+	}
+
+	AST_LIST_INSERT_HEAD(free_list, mem, list);
 }
 
 void ast_json_set_alloc_funcs(void *(*malloc_fn)(size_t), void (*free_fn)(void*))
@@ -69,21 +197,41 @@ void ast_json_set_alloc_funcs(void *(*malloc_fn)(size_t), void (*free_fn)(void*)
 
 void ast_json_reset_alloc_funcs(void)
 {
-	json_set_alloc_funcs(json_malloc, json_free);
+	json_set_alloc_funcs(ast_json_malloc, ast_json_free);
 }
 
 struct ast_json *ast_json_ref(struct ast_json *json)
 {
+	/* Jansson refcounting is non-atomic; lock it. */
+	SCOPED_JSON_LOCK(json);
 	json_incref((json_t *)json);
 	return json;
 }
 
 void ast_json_unref(struct ast_json *json)
 {
-	if (!json) {
+	struct json_mem_list *free_list;
+	struct json_mem *mem;
+
+	/* Jansson refcounting is non-atomic; lock it. */
+	{
+		SCOPED_JSON_LOCK(json);
+		if (!json) {
+			return;
+		}
+		json_decref((json_t *)json);
+	}
+
+	/* Now free any objects that were ast_json_free()'s while the lock was
+	 * held */
+	free_list = json_free_list();
+	if (!free_list) {
 		return;
 	}
-	json_decref((json_t *)json);
+
+	while ((mem = AST_LIST_REMOVE_HEAD(free_list, list))) {
+		json_mem_free(mem);
+	}
 }
 
 enum ast_json_type ast_json_typeof(const struct ast_json *json)
@@ -383,6 +531,10 @@ static size_t dump_flags(enum ast_json_encoding_format format)
 
 char *ast_json_dump_string_format(struct ast_json *root, enum ast_json_encoding_format format)
 {
+	/* Jansson's json_dump*, even though it's a read operation, isn't
+	 * thread safe for concurrent reads. Locking is necessary.
+	 * See http://www.digip.org/jansson/doc/2.4/portability.html#thread-safety. */
+	SCOPED_JSON_LOCK(root);
 	return json_dumps((json_t *)root, dump_flags(format));
 }
 
@@ -419,12 +571,20 @@ static int write_to_ast_str(const char *buffer, size_t size, void *data)
 
 int ast_json_dump_str_format(struct ast_json *root, struct ast_str **dst, enum ast_json_encoding_format format)
 {
+	/* Jansson's json_dump*, even though it's a read operation, isn't
+	 * thread safe for concurrent reads. Locking is necessary.
+	 * See http://www.digip.org/jansson/doc/2.4/portability.html#thread-safety. */
+	SCOPED_JSON_LOCK(root);
 	return json_dump_callback((json_t *)root, write_to_ast_str, dst, dump_flags(format));
 }
 
 
 int ast_json_dump_file_format(struct ast_json *root, FILE *output, enum ast_json_encoding_format format)
 {
+	/* Jansson's json_dump*, even though it's a read operation, isn't
+	 * thread safe for concurrent reads. Locking is necessary.
+	 * See http://www.digip.org/jansson/doc/2.4/portability.html#thread-safety. */
+	SCOPED_JSON_LOCK(root);
 	if (!root || !output) {
 		return -1;
 	}
@@ -432,6 +592,10 @@ int ast_json_dump_file_format(struct ast_json *root, FILE *output, enum ast_json
 }
 int ast_json_dump_new_file_format(struct ast_json *root, const char *path, enum ast_json_encoding_format format)
 {
+	/* Jansson's json_dump*, even though it's a read operation, isn't
+	 * thread safe for concurrent reads. Locking is necessary.
+	 * See http://www.digip.org/jansson/doc/2.4/portability.html#thread-safety. */
+	SCOPED_JSON_LOCK(root);
 	if (!root || !path) {
 		return -1;
 	}
diff --git a/res/ari/ari_websockets.c b/res/ari/ari_websockets.c
index 13650c293..a34e0f691 100644
--- a/res/ari/ari_websockets.c
+++ b/res/ari/ari_websockets.c
@@ -142,7 +142,7 @@ struct ast_json *ast_ari_websocket_session_read(
 int ast_ari_websocket_session_write(struct ast_ari_websocket_session *session,
 	struct ast_json *message)
 {
-	RAII_VAR(char *, str, NULL, ast_free);
+	RAII_VAR(char *, str, NULL, ast_json_free);
 
 #ifdef AST_DEVMODE
 	if (!session->validator(message)) {
diff --git a/res/res_sorcery_astdb.c b/res/res_sorcery_astdb.c
index 65a6a504e..ecea885d0 100644
--- a/res/res_sorcery_astdb.c
+++ b/res/res_sorcery_astdb.c
@@ -126,7 +126,7 @@ static int sorcery_json_equal(struct ast_json *object, struct ast_json *criteria
 static int sorcery_astdb_create(const struct ast_sorcery *sorcery, void *data, void *object)
 {
 	RAII_VAR(struct ast_json *, objset, ast_sorcery_objectset_json_create(sorcery, object), ast_json_unref);
-	RAII_VAR(char *, value, NULL, ast_free_ptr);
+	RAII_VAR(char *, value, NULL, ast_json_free);
 	const char *prefix = data;
 	char family[strlen(prefix) + strlen(ast_sorcery_object_get_type(object)) + 2];
 
diff --git a/tests/test_json.c b/tests/test_json.c
index 08837de06..d6bc7a2c7 100644
--- a/tests/test_json.c
+++ b/tests/test_json.c
@@ -55,7 +55,7 @@ static size_t alloc_count;
  */
 static void *json_debug_malloc(size_t size)
 {
-	void *p = ast_malloc(size);
+	void *p = ast_json_malloc(size);
 	if (p) {
 		++alloc_count;
 	}
@@ -67,7 +67,7 @@ static void json_debug_free(void *p)
 	if (p) {
 		--alloc_count;
 	}
-	ast_free(p);
+	ast_json_free(p);
 }
 
 static int json_test_init(struct ast_test_info *info, struct ast_test *test)