From dc3b313e91b573f393d8717c1c9e8c11a51f7ab1 Mon Sep 17 00:00:00 2001 From: Corey Farrell Date: Mon, 22 Jan 2018 14:50:37 -0500 Subject: [PATCH 1/4] Use thread-safe reference counting if supported by the compiler. This makes use of __atomic or __sync builtin compiler functions to make json_decref and json_incref thread-safe. Issue #387 --- CMakeLists.txt | 4 ++-- configure.ac | 12 ++++++++++-- src/jansson.h | 23 ++++++++++++++++++++--- src/jansson_config.h.in | 8 ++++++++ 4 files changed, 40 insertions(+), 7 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 68cb35b..7c1082b 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -303,8 +303,8 @@ else() set (JSON_INLINE) endif() -check_c_source_compiles ("int main() { unsigned long val; __sync_bool_compare_and_swap(&val, 0, 1); return 0; } " HAVE_SYNC_BUILTINS) -check_c_source_compiles ("int main() { char l; unsigned long v; __atomic_test_and_set(&l, __ATOMIC_RELAXED); __atomic_store_n(&v, 1, __ATOMIC_RELEASE); __atomic_load_n(&v, __ATOMIC_ACQUIRE); return 0; }" HAVE_ATOMIC_BUILTINS) +check_c_source_compiles ("int main() { unsigned long val; __sync_bool_compare_and_swap(&val, 0, 1); __sync_add_and_fetch(&val, 1); __sync_sub_and_fetch(&val, 1); return 0; } " HAVE_SYNC_BUILTINS) +check_c_source_compiles ("int main() { char l; unsigned long v; __atomic_test_and_set(&l, __ATOMIC_RELAXED); __atomic_store_n(&v, 1, __ATOMIC_RELEASE); __atomic_load_n(&v, __ATOMIC_ACQUIRE); __atomic_add_fetch(&v, 1, __ATOMIC_ACQUIRE); __atomic_sub_fetch(&v, 1, __ATOMIC_RELEASE); return 0; }" HAVE_ATOMIC_BUILTINS) set (JANSSON_INITIAL_HASHTABLE_ORDER 3 CACHE STRING "Number of buckets new object hashtables contain is 2 raised to this power. The default is 3, so empty hashtables contain 2^3 = 8 buckets.") diff --git a/configure.ac b/configure.ac index d1c4faf..fa0f005 100644 --- a/configure.ac +++ b/configure.ac @@ -38,25 +38,33 @@ AC_CHECK_FUNCS([close getpid gettimeofday localeconv open read sched_yield strto AC_MSG_CHECKING([for gcc __sync builtins]) have_sync_builtins=no AC_TRY_LINK( - [], [unsigned long val; __sync_bool_compare_and_swap(&val, 0, 1);], + [], [unsigned long val; __sync_bool_compare_and_swap(&val, 0, 1); __sync_add_and_fetch(&val, 1); __sync_sub_and_fetch(&val, 1);], [have_sync_builtins=yes], ) if test "x$have_sync_builtins" = "xyes"; then AC_DEFINE([HAVE_SYNC_BUILTINS], [1], [Define to 1 if gcc's __sync builtins are available]) + json_have_sync_builtins=1 +else + json_have_sync_builtins=0 fi +AC_SUBST([json_have_sync_builtins]) AC_MSG_RESULT([$have_sync_builtins]) AC_MSG_CHECKING([for gcc __atomic builtins]) have_atomic_builtins=no AC_TRY_LINK( - [], [char l; unsigned long v; __atomic_test_and_set(&l, __ATOMIC_RELAXED); __atomic_store_n(&v, 1, __ATOMIC_RELEASE); __atomic_load_n(&v, __ATOMIC_ACQUIRE);], + [], [char l; unsigned long v; __atomic_test_and_set(&l, __ATOMIC_RELAXED); __atomic_store_n(&v, 1, __ATOMIC_RELEASE); __atomic_load_n(&v, __ATOMIC_ACQUIRE); __atomic_add_fetch(&v, 1, __ATOMIC_ACQUIRE); __atomic_sub_fetch(&v, 1, __ATOMIC_RELEASE);], [have_atomic_builtins=yes], ) if test "x$have_atomic_builtins" = "xyes"; then AC_DEFINE([HAVE_ATOMIC_BUILTINS], [1], [Define to 1 if gcc's __atomic builtins are available]) + json_have_atomic_builtins=1 +else + json_have_atomic_builtins=0 fi +AC_SUBST([json_have_atomic_builtins]) AC_MSG_RESULT([$have_atomic_builtins]) case "$ac_cv_type_long_long_int$ac_cv_func_strtoll" in diff --git a/src/jansson.h b/src/jansson.h index ecf0e09..042390c 100644 --- a/src/jansson.h +++ b/src/jansson.h @@ -33,6 +33,11 @@ extern "C" { (JANSSON_MINOR_VERSION << 8) | \ (JANSSON_MICRO_VERSION << 0)) +/* If __atomic or __sync builtins are available the library is thread + * safe for all read-only functions plus reference counting. */ +#if JSON_HAVE_ATOMIC_BUILTINS || JSON_HAVE_SYNC_BUILTINS +#define JANSSON_THREAD_SAFE +#endif /* types */ @@ -49,7 +54,7 @@ typedef enum { typedef struct json_t { json_type type; - size_t refcount; + volatile size_t refcount; } json_t; #ifndef JANSSON_USING_CMAKE /* disabled if using cmake */ @@ -94,11 +99,23 @@ json_t *json_false(void); #define json_boolean(val) ((val) ? json_true() : json_false()) json_t *json_null(void); +/* do not call JSON_INTERNAL_INCREF or JSON_INTERNAL_DECREF directly */ +#if JSON_HAVE_ATOMIC_BUILTINS +#define JSON_INTERNAL_INCREF(json) __atomic_add_fetch(&json->refcount, 1, __ATOMIC_ACQUIRE) +#define JSON_INTERNAL_DECREF(json) __atomic_sub_fetch(&json->refcount, 1, __ATOMIC_RELEASE) +#elif JSON_HAVE_SYNC_BUILTINS +#define JSON_INTERNAL_INCREF(json) __sync_add_and_fetch(&json->refcount, 1) +#define JSON_INTERNAL_DECREF(json) __sync_sub_and_fetch(&json->refcount, 1) +#else +#define JSON_INTERNAL_INCREF(json) (++json->refcount) +#define JSON_INTERNAL_DECREF(json) (--json->refcount) +#endif + static JSON_INLINE json_t *json_incref(json_t *json) { if(json && json->refcount != (size_t)-1) - ++json->refcount; + JSON_INTERNAL_INCREF(json); return json; } @@ -108,7 +125,7 @@ void json_delete(json_t *json); static JSON_INLINE void json_decref(json_t *json) { - if(json && json->refcount != (size_t)-1 && --json->refcount == 0) + if(json && json->refcount != (size_t)-1 && JSON_INTERNAL_DECREF(json) == 0) json_delete(json); } diff --git a/src/jansson_config.h.in b/src/jansson_config.h.in index 5e94762..fe692ab 100644 --- a/src/jansson_config.h.in +++ b/src/jansson_config.h.in @@ -36,6 +36,14 @@ otherwise to 0. */ #define JSON_HAVE_LOCALECONV @json_have_localeconv@ +/* If __atomic builtins are available they will be used to manage + reference counts of json_t. */ +#define JSON_HAVE_ATOMIC_BUILTINS @json_have_atomic_builtins@ + +/* If __atomic builtins are not available we try using __sync builtins + to manage reference counts of json_t. */ +#define JSON_HAVE_SYNC_BUILTINS @json_have_sync_builtins@ + /* Maximum recursion depth for parsing JSON input. This limits the depth of e.g. array-within-array constructions. */ #define JSON_PARSER_MAX_DEPTH 2048 From 37e0ee4d485f8d0821ea357d8bf97d3904145b73 Mon Sep 17 00:00:00 2001 From: Corey Farrell Date: Mon, 22 Jan 2018 15:39:58 -0500 Subject: [PATCH 2/4] json_dump: Fix thread safety issue. Circular reference detection in json_dump was not thread safe. Replace visited flag with a hashtable_t. Issue #387 --- src/dump.c | 90 +++++++++++++++++++++++-------------------- src/jansson_private.h | 2 - src/value.c | 4 -- 3 files changed, 48 insertions(+), 48 deletions(-) diff --git a/src/dump.c b/src/dump.c index 63a9c15..8e725c9 100644 --- a/src/dump.c +++ b/src/dump.c @@ -196,8 +196,17 @@ static int compare_keys(const void *key1, const void *key2) return strcmp(*(const char **)key1, *(const char **)key2); } +static int loop_check(hashtable_t *parents, const json_t *json, char *key, size_t key_size) +{ + snprintf(key, key_size, "%p", json); + if (hashtable_get(parents, key)) + return -1; + + return hashtable_set(parents, key, json_null()); +} + static int do_dump(const json_t *json, size_t flags, int depth, - json_dump_callback_t dump, void *data) + hashtable_t *parents, json_dump_callback_t dump, void *data) { int embed = flags & JSON_EMBED; @@ -251,58 +260,53 @@ static int do_dump(const json_t *json, size_t flags, int depth, { size_t n; size_t i; - - json_array_t *array; + /* Space for "0x", double the sizeof a pointer for the hex and a terminator. */ + char key[2 + (sizeof(json) * 2) + 1]; /* detect circular references */ - array = json_to_array(json); - if(array->visited) - goto array_error; - array->visited = 1; + if (loop_check(parents, json, key, sizeof(key))) + return -1; n = json_array_size(json); if(!embed && dump("[", 1, data)) - goto array_error; + return -1; if(n == 0) { - array->visited = 0; + hashtable_del(parents, key); return embed ? 0 : dump("]", 1, data); } if(dump_indent(flags, depth + 1, 0, dump, data)) - goto array_error; + return -1; for(i = 0; i < n; ++i) { if(do_dump(json_array_get(json, i), flags, depth + 1, - dump, data)) - goto array_error; + parents, dump, data)) + return -1; if(i < n - 1) { if(dump(",", 1, data) || dump_indent(flags, depth + 1, 1, dump, data)) - goto array_error; + return -1; } else { if(dump_indent(flags, depth, 0, dump, data)) - goto array_error; + return -1; } } - array->visited = 0; + hashtable_del(parents, key); return embed ? 0 : dump("]", 1, data); - - array_error: - array->visited = 0; - return -1; } case JSON_OBJECT: { - json_object_t *object; void *iter; const char *separator; int separator_length; + /* Space for "0x", double the sizeof a pointer for the hex and a terminator. */ + char key[2 + (sizeof(json) * 2) + 1]; if(flags & JSON_COMPACT) { separator = ":"; @@ -314,21 +318,19 @@ static int do_dump(const json_t *json, size_t flags, int depth, } /* detect circular references */ - object = json_to_object(json); - if(object->visited) - goto object_error; - object->visited = 1; + if (loop_check(parents, json, key, sizeof(key))) + return -1; iter = json_object_iter((json_t *)json); if(!embed && dump("{", 1, data)) - goto object_error; + return -1; if(!iter) { - object->visited = 0; + hashtable_del(parents, key); return embed ? 0 : dump("}", 1, data); } if(dump_indent(flags, depth + 1, 0, dump, data)) - goto object_error; + return -1; if(flags & JSON_SORT_KEYS) { @@ -338,7 +340,7 @@ static int do_dump(const json_t *json, size_t flags, int depth, size = json_object_size(json); keys = jsonp_malloc(size * sizeof(const char *)); if(!keys) - goto object_error; + return -1; i = 0; while(iter) @@ -362,10 +364,10 @@ static int do_dump(const json_t *json, size_t flags, int depth, dump_string(key, strlen(key), dump, data, flags); if(dump(separator, separator_length, data) || - do_dump(value, flags, depth + 1, dump, data)) + do_dump(value, flags, depth + 1, parents, dump, data)) { jsonp_free(keys); - goto object_error; + return -1; } if(i < size - 1) @@ -374,7 +376,7 @@ static int do_dump(const json_t *json, size_t flags, int depth, dump_indent(flags, depth + 1, 1, dump, data)) { jsonp_free(keys); - goto object_error; + return -1; } } else @@ -382,7 +384,7 @@ static int do_dump(const json_t *json, size_t flags, int depth, if(dump_indent(flags, depth, 0, dump, data)) { jsonp_free(keys); - goto object_error; + return -1; } } } @@ -401,31 +403,27 @@ static int do_dump(const json_t *json, size_t flags, int depth, dump_string(key, strlen(key), dump, data, flags); if(dump(separator, separator_length, data) || do_dump(json_object_iter_value(iter), flags, depth + 1, - dump, data)) - goto object_error; + parents, dump, data)) + return -1; if(next) { if(dump(",", 1, data) || dump_indent(flags, depth + 1, 1, dump, data)) - goto object_error; + return -1; } else { if(dump_indent(flags, depth, 0, dump, data)) - goto object_error; + return -1; } iter = next; } } - object->visited = 0; + hashtable_del(parents, key); return embed ? 0 : dump("}", 1, data); - - object_error: - object->visited = 0; - return -1; } default: @@ -489,10 +487,18 @@ int json_dump_file(const json_t *json, const char *path, size_t flags) int json_dump_callback(const json_t *json, json_dump_callback_t callback, void *data, size_t flags) { + int res; + hashtable_t parents_set; + if(!(flags & JSON_ENCODE_ANY)) { if(!json_is_array(json) && !json_is_object(json)) return -1; } - return do_dump(json, flags, 0, callback, data); + if (hashtable_init(&parents_set)) + return -1; + res = do_dump(json, flags, 0, &parents_set, callback, data); + hashtable_close(&parents_set); + + return res; } diff --git a/src/jansson_private.h b/src/jansson_private.h index 1e1cb3c..d19cf9c 100644 --- a/src/jansson_private.h +++ b/src/jansson_private.h @@ -35,7 +35,6 @@ typedef struct { json_t json; hashtable_t hashtable; - int visited; } json_object_t; typedef struct { @@ -43,7 +42,6 @@ typedef struct { size_t size; size_t entries; json_t **table; - int visited; } json_array_t; typedef struct { diff --git a/src/value.c b/src/value.c index 94869f4..d27bb12 100644 --- a/src/value.c +++ b/src/value.c @@ -67,8 +67,6 @@ json_t *json_object(void) return NULL; } - object->visited = 0; - return &object->json; } @@ -354,8 +352,6 @@ json_t *json_array(void) return NULL; } - array->visited = 0; - return &array->json; } From 3aee856d7bc453d488ff9fa5f2ac70e979dd9869 Mon Sep 17 00:00:00 2001 From: Corey Farrell Date: Mon, 22 Jan 2018 19:37:36 -0500 Subject: [PATCH 3/4] Docs: Update information on thread safety. Fixes #387 --- doc/apiref.rst | 5 +++++ doc/portability.rst | 24 +++++++----------------- 2 files changed, 12 insertions(+), 17 deletions(-) diff --git a/doc/apiref.rst b/doc/apiref.rst index a8cf8d6..6c73d30 100644 --- a/doc/apiref.rst +++ b/doc/apiref.rst @@ -58,6 +58,11 @@ the library: /* Code specific to version 1.3 and above */ #endif +``JANSSON_THREAD_SAFE`` + If this value is defined all read-only operations and reference counting in + Jansson are thread safe. This value is not defined for versions older than + ``2.11`` or when the compiler does not provide built-in atomic functions. + Value Representation ==================== diff --git a/doc/portability.rst b/doc/portability.rst index 272e46a..e1b0c0b 100644 --- a/doc/portability.rst +++ b/doc/portability.rst @@ -13,23 +13,13 @@ see below. There's no locking performed inside Jansson's code, so a multithreaded program must perform its own locking if JSON values are shared by -multiple threads. Jansson's reference counting semantics may make this -a bit harder than it seems, as it's possible to have a reference to a -value that's also stored inside a list or object. Modifying the -container (adding or removing values) may trigger concurrent access to -such values, as containers manage the reference count of their -contained values. Bugs involving concurrent incrementing or -decrementing of deference counts may be hard to track. - -The encoding functions (:func:`json_dumps()` and friends) track -reference loops by modifying the internal state of objects and arrays. -For this reason, encoding functions must not be run on the same JSON -values in two separate threads at the same time. As already noted -above, be especially careful if two arrays or objects share their -contained values with another array or object. - -If you want to make sure that two JSON value hierarchies do not -contain shared values, use :func:`json_deep_copy()` to make copies. +multiple threads. Jansson uses built-in compiler atomic functions to +manage reference counts. If compiler support is not available it may +be very difficult to ensure thread safety of reference counting. +It's possible to have a reference to a value that's also stored inside +a list or object. Modifying the container (adding or removing values) +may trigger concurrent access to such values, as containers manage the +reference count of their contained values. Hash function seed From f44921e1766b4584bb239593ce7e7ea3242c99c3 Mon Sep 17 00:00:00 2001 From: Petri Lehtinen Date: Thu, 8 Feb 2018 12:38:14 +0200 Subject: [PATCH 4/4] Clarify thread safety docs, rename JANSSON_THREAD_SAFE --- doc/apiref.rst | 2 +- doc/portability.rst | 36 ++++++++++++++++++++++++------------ src/jansson.h | 2 +- 3 files changed, 26 insertions(+), 14 deletions(-) diff --git a/doc/apiref.rst b/doc/apiref.rst index 6c73d30..3ad406d 100644 --- a/doc/apiref.rst +++ b/doc/apiref.rst @@ -58,7 +58,7 @@ the library: /* Code specific to version 1.3 and above */ #endif -``JANSSON_THREAD_SAFE`` +``JANSSON_THREAD_SAFE_REFCOUNT`` If this value is defined all read-only operations and reference counting in Jansson are thread safe. This value is not defined for versions older than ``2.11`` or when the compiler does not provide built-in atomic functions. diff --git a/doc/portability.rst b/doc/portability.rst index e1b0c0b..f8d81cd 100644 --- a/doc/portability.rst +++ b/doc/portability.rst @@ -7,19 +7,31 @@ Portability Thread safety ------------- -Jansson is thread safe and has no mutable global state. The only -exceptions are the hash function seed and memory allocation functions, -see below. +Jansson as a library is thread safe and has no mutable global state. +The only exceptions are the hash function seed and memory allocation +functions, see below. -There's no locking performed inside Jansson's code, so a multithreaded -program must perform its own locking if JSON values are shared by -multiple threads. Jansson uses built-in compiler atomic functions to -manage reference counts. If compiler support is not available it may -be very difficult to ensure thread safety of reference counting. -It's possible to have a reference to a value that's also stored inside -a list or object. Modifying the container (adding or removing values) -may trigger concurrent access to such values, as containers manage the -reference count of their contained values. +There's no locking performed inside Jansson's code. **Read-only** +access to JSON values shared by multiple threads is safe, but +**mutating** a JSON value that's shared by multiple threads is not. A +multithreaded program must perform its own locking if JSON values +shared by multiple threads are mutated. + +However, **reference count manipulation** (:func:`json_incref()`, +:func:`json_decref()`) is usually thread-safe, and can be performed on +JSON values that are shared among threads. The thread-safety of +reference counting can be checked with the +``JANSSON_THREAD_SAFE_REFCOUNT`` preprocessor constant. Thread-safe +reference count manipulation is achieved using compiler built-in +atomic functions, which are available in most modern compilers. + +If compiler support is not available (``JANSSON_THREAD_SAFE_REFCOUNT`` +is not defined), it may be very difficult to ensure thread safety of +reference counting. It's possible to have a reference to a value +that's also stored inside an array or object in another thread. +Modifying the container (adding or removing values) may trigger +concurrent access to such values, as containers manage the reference +count of their contained values. Hash function seed diff --git a/src/jansson.h b/src/jansson.h index 042390c..b17e8f4 100644 --- a/src/jansson.h +++ b/src/jansson.h @@ -36,7 +36,7 @@ extern "C" { /* If __atomic or __sync builtins are available the library is thread * safe for all read-only functions plus reference counting. */ #if JSON_HAVE_ATOMIC_BUILTINS || JSON_HAVE_SYNC_BUILTINS -#define JANSSON_THREAD_SAFE +#define JANSSON_THREAD_SAFE_REFCOUNT 1 #endif /* types */