diff --git a/include/nlohmann/detail/conversions/to_json.hpp b/include/nlohmann/detail/conversions/to_json.hpp index 9d7f55fc9..08462a4bb 100644 --- a/include/nlohmann/detail/conversions/to_json.hpp +++ b/include/nlohmann/detail/conversions/to_json.hpp @@ -22,6 +22,13 @@ namespace detail // constructors // ////////////////// +/* + * Note all external_constructor<>::construct functions need to call + * j.m_value.destroy(j.m_type) to avoid a memory leak in case j contains an + * allocated value (e.g., a string). See bug issue + * https://github.com/nlohmann/json/issues/2865 for more information. + */ + template struct external_constructor; template<> @@ -30,6 +37,7 @@ struct external_constructor template static void construct(BasicJsonType& j, typename BasicJsonType::boolean_t b) noexcept { + j.m_value.destroy(j.m_type); j.m_type = value_t::boolean; j.m_value = b; j.assert_invariant(); @@ -42,6 +50,7 @@ struct external_constructor template static void construct(BasicJsonType& j, const typename BasicJsonType::string_t& s) { + j.m_value.destroy(j.m_type); j.m_type = value_t::string; j.m_value = s; j.assert_invariant(); @@ -50,6 +59,7 @@ struct external_constructor template static void construct(BasicJsonType& j, typename BasicJsonType::string_t&& s) { + j.m_value.destroy(j.m_type); j.m_type = value_t::string; j.m_value = std::move(s); j.assert_invariant(); @@ -60,6 +70,7 @@ struct external_constructor int > = 0 > static void construct(BasicJsonType& j, const CompatibleStringType& str) { + j.m_value.destroy(j.m_type); j.m_type = value_t::string; j.m_value.string = j.template create(str); j.assert_invariant(); @@ -72,6 +83,7 @@ struct external_constructor template static void construct(BasicJsonType& j, const typename BasicJsonType::binary_t& b) { + j.m_value.destroy(j.m_type); j.m_type = value_t::binary; j.m_value = typename BasicJsonType::binary_t(b); j.assert_invariant(); @@ -80,6 +92,7 @@ struct external_constructor template static void construct(BasicJsonType& j, typename BasicJsonType::binary_t&& b) { + j.m_value.destroy(j.m_type); j.m_type = value_t::binary; j.m_value = typename BasicJsonType::binary_t(std::move(b));; j.assert_invariant(); @@ -92,6 +105,7 @@ struct external_constructor template static void construct(BasicJsonType& j, typename BasicJsonType::number_float_t val) noexcept { + j.m_value.destroy(j.m_type); j.m_type = value_t::number_float; j.m_value = val; j.assert_invariant(); @@ -104,6 +118,7 @@ struct external_constructor template static void construct(BasicJsonType& j, typename BasicJsonType::number_unsigned_t val) noexcept { + j.m_value.destroy(j.m_type); j.m_type = value_t::number_unsigned; j.m_value = val; j.assert_invariant(); @@ -116,6 +131,7 @@ struct external_constructor template static void construct(BasicJsonType& j, typename BasicJsonType::number_integer_t val) noexcept { + j.m_value.destroy(j.m_type); j.m_type = value_t::number_integer; j.m_value = val; j.assert_invariant(); @@ -128,6 +144,7 @@ struct external_constructor template static void construct(BasicJsonType& j, const typename BasicJsonType::array_t& arr) { + j.m_value.destroy(j.m_type); j.m_type = value_t::array; j.m_value = arr; j.set_parents(); @@ -137,6 +154,7 @@ struct external_constructor template static void construct(BasicJsonType& j, typename BasicJsonType::array_t&& arr) { + j.m_value.destroy(j.m_type); j.m_type = value_t::array; j.m_value = std::move(arr); j.set_parents(); @@ -150,6 +168,8 @@ struct external_constructor { using std::begin; using std::end; + + j.m_value.destroy(j.m_type); j.m_type = value_t::array; j.m_value.array = j.template create(begin(arr), end(arr)); j.set_parents(); @@ -159,6 +179,7 @@ struct external_constructor template static void construct(BasicJsonType& j, const std::vector& arr) { + j.m_value.destroy(j.m_type); j.m_type = value_t::array; j.m_value = value_t::array; j.m_value.array->reserve(arr.size()); @@ -174,6 +195,7 @@ struct external_constructor enable_if_t::value, int> = 0> static void construct(BasicJsonType& j, const std::valarray& arr) { + j.m_value.destroy(j.m_type); j.m_type = value_t::array; j.m_value = value_t::array; j.m_value.array->resize(arr.size()); @@ -192,6 +214,7 @@ struct external_constructor template static void construct(BasicJsonType& j, const typename BasicJsonType::object_t& obj) { + j.m_value.destroy(j.m_type); j.m_type = value_t::object; j.m_value = obj; j.set_parents(); @@ -201,6 +224,7 @@ struct external_constructor template static void construct(BasicJsonType& j, typename BasicJsonType::object_t&& obj) { + j.m_value.destroy(j.m_type); j.m_type = value_t::object; j.m_value = std::move(obj); j.set_parents(); @@ -214,6 +238,7 @@ struct external_constructor using std::begin; using std::end; + j.m_value.destroy(j.m_type); j.m_type = value_t::object; j.m_value.object = j.template create(begin(obj), end(obj)); j.set_parents(); diff --git a/include/nlohmann/json.hpp b/include/nlohmann/json.hpp index a337c1c69..94c004227 100644 --- a/include/nlohmann/json.hpp +++ b/include/nlohmann/json.hpp @@ -1131,53 +1131,55 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec binary = create(std::move(value)); } - void destroy(value_t t) noexcept + void destroy(value_t t) { - // flatten the current json_value to a heap-allocated stack - std::vector stack; + if (t == value_t::array || t == value_t::object) + { + // flatten the current json_value to a heap-allocated stack + std::vector stack; - // move the top-level items to stack - if (t == value_t::array) - { - stack.reserve(array->size()); - std::move(array->begin(), array->end(), std::back_inserter(stack)); - } - else if (t == value_t::object) - { - stack.reserve(object->size()); - for (auto&& it : *object) + // move the top-level items to stack + if (t == value_t::array) { - stack.push_back(std::move(it.second)); + stack.reserve(array->size()); + std::move(array->begin(), array->end(), std::back_inserter(stack)); } - } - - while (!stack.empty()) - { - // move the last item to local variable to be processed - basic_json current_item(std::move(stack.back())); - stack.pop_back(); - - // if current_item is array/object, move - // its children to the stack to be processed later - if (current_item.is_array()) + else { - std::move(current_item.m_value.array->begin(), current_item.m_value.array->end(), - std::back_inserter(stack)); - - current_item.m_value.array->clear(); - } - else if (current_item.is_object()) - { - for (auto&& it : *current_item.m_value.object) + stack.reserve(object->size()); + for (auto&& it : *object) { stack.push_back(std::move(it.second)); } - - current_item.m_value.object->clear(); } - // it's now safe that current_item get destructed - // since it doesn't have any children + while (!stack.empty()) + { + // move the last item to local variable to be processed + basic_json current_item(std::move(stack.back())); + stack.pop_back(); + + // if current_item is array/object, move + // its children to the stack to be processed later + if (current_item.is_array()) + { + std::move(current_item.m_value.array->begin(), current_item.m_value.array->end(), std::back_inserter(stack)); + + current_item.m_value.array->clear(); + } + else if (current_item.is_object()) + { + for (auto&& it : *current_item.m_value.object) + { + stack.push_back(std::move(it.second)); + } + + current_item.m_value.object->clear(); + } + + // it's now safe that current_item get destructed + // since it doesn't have any children + } } switch (t) diff --git a/single_include/nlohmann/json.hpp b/single_include/nlohmann/json.hpp index 429964dd7..8130c6aa8 100644 --- a/single_include/nlohmann/json.hpp +++ b/single_include/nlohmann/json.hpp @@ -4480,6 +4480,13 @@ namespace detail // constructors // ////////////////// +/* + * Note all external_constructor<>::construct functions need to call + * j.m_value.destroy(j.m_type) to avoid a memory leak in case j contains an + * allocated value (e.g., a string). See bug issue + * https://github.com/nlohmann/json/issues/2865 for more information. + */ + template struct external_constructor; template<> @@ -4488,6 +4495,7 @@ struct external_constructor template static void construct(BasicJsonType& j, typename BasicJsonType::boolean_t b) noexcept { + j.m_value.destroy(j.m_type); j.m_type = value_t::boolean; j.m_value = b; j.assert_invariant(); @@ -4500,6 +4508,7 @@ struct external_constructor template static void construct(BasicJsonType& j, const typename BasicJsonType::string_t& s) { + j.m_value.destroy(j.m_type); j.m_type = value_t::string; j.m_value = s; j.assert_invariant(); @@ -4508,6 +4517,7 @@ struct external_constructor template static void construct(BasicJsonType& j, typename BasicJsonType::string_t&& s) { + j.m_value.destroy(j.m_type); j.m_type = value_t::string; j.m_value = std::move(s); j.assert_invariant(); @@ -4518,6 +4528,7 @@ struct external_constructor int > = 0 > static void construct(BasicJsonType& j, const CompatibleStringType& str) { + j.m_value.destroy(j.m_type); j.m_type = value_t::string; j.m_value.string = j.template create(str); j.assert_invariant(); @@ -4530,6 +4541,7 @@ struct external_constructor template static void construct(BasicJsonType& j, const typename BasicJsonType::binary_t& b) { + j.m_value.destroy(j.m_type); j.m_type = value_t::binary; j.m_value = typename BasicJsonType::binary_t(b); j.assert_invariant(); @@ -4538,6 +4550,7 @@ struct external_constructor template static void construct(BasicJsonType& j, typename BasicJsonType::binary_t&& b) { + j.m_value.destroy(j.m_type); j.m_type = value_t::binary; j.m_value = typename BasicJsonType::binary_t(std::move(b));; j.assert_invariant(); @@ -4550,6 +4563,7 @@ struct external_constructor template static void construct(BasicJsonType& j, typename BasicJsonType::number_float_t val) noexcept { + j.m_value.destroy(j.m_type); j.m_type = value_t::number_float; j.m_value = val; j.assert_invariant(); @@ -4562,6 +4576,7 @@ struct external_constructor template static void construct(BasicJsonType& j, typename BasicJsonType::number_unsigned_t val) noexcept { + j.m_value.destroy(j.m_type); j.m_type = value_t::number_unsigned; j.m_value = val; j.assert_invariant(); @@ -4574,6 +4589,7 @@ struct external_constructor template static void construct(BasicJsonType& j, typename BasicJsonType::number_integer_t val) noexcept { + j.m_value.destroy(j.m_type); j.m_type = value_t::number_integer; j.m_value = val; j.assert_invariant(); @@ -4586,6 +4602,7 @@ struct external_constructor template static void construct(BasicJsonType& j, const typename BasicJsonType::array_t& arr) { + j.m_value.destroy(j.m_type); j.m_type = value_t::array; j.m_value = arr; j.set_parents(); @@ -4595,6 +4612,7 @@ struct external_constructor template static void construct(BasicJsonType& j, typename BasicJsonType::array_t&& arr) { + j.m_value.destroy(j.m_type); j.m_type = value_t::array; j.m_value = std::move(arr); j.set_parents(); @@ -4608,6 +4626,8 @@ struct external_constructor { using std::begin; using std::end; + + j.m_value.destroy(j.m_type); j.m_type = value_t::array; j.m_value.array = j.template create(begin(arr), end(arr)); j.set_parents(); @@ -4617,6 +4637,7 @@ struct external_constructor template static void construct(BasicJsonType& j, const std::vector& arr) { + j.m_value.destroy(j.m_type); j.m_type = value_t::array; j.m_value = value_t::array; j.m_value.array->reserve(arr.size()); @@ -4632,6 +4653,7 @@ struct external_constructor enable_if_t::value, int> = 0> static void construct(BasicJsonType& j, const std::valarray& arr) { + j.m_value.destroy(j.m_type); j.m_type = value_t::array; j.m_value = value_t::array; j.m_value.array->resize(arr.size()); @@ -4650,6 +4672,7 @@ struct external_constructor template static void construct(BasicJsonType& j, const typename BasicJsonType::object_t& obj) { + j.m_value.destroy(j.m_type); j.m_type = value_t::object; j.m_value = obj; j.set_parents(); @@ -4659,6 +4682,7 @@ struct external_constructor template static void construct(BasicJsonType& j, typename BasicJsonType::object_t&& obj) { + j.m_value.destroy(j.m_type); j.m_type = value_t::object; j.m_value = std::move(obj); j.set_parents(); @@ -4672,6 +4696,7 @@ struct external_constructor using std::begin; using std::end; + j.m_value.destroy(j.m_type); j.m_type = value_t::object; j.m_value.object = j.template create(begin(obj), end(obj)); j.set_parents(); @@ -18166,53 +18191,55 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec binary = create(std::move(value)); } - void destroy(value_t t) noexcept + void destroy(value_t t) { - // flatten the current json_value to a heap-allocated stack - std::vector stack; + if (t == value_t::array || t == value_t::object) + { + // flatten the current json_value to a heap-allocated stack + std::vector stack; - // move the top-level items to stack - if (t == value_t::array) - { - stack.reserve(array->size()); - std::move(array->begin(), array->end(), std::back_inserter(stack)); - } - else if (t == value_t::object) - { - stack.reserve(object->size()); - for (auto&& it : *object) + // move the top-level items to stack + if (t == value_t::array) { - stack.push_back(std::move(it.second)); + stack.reserve(array->size()); + std::move(array->begin(), array->end(), std::back_inserter(stack)); } - } - - while (!stack.empty()) - { - // move the last item to local variable to be processed - basic_json current_item(std::move(stack.back())); - stack.pop_back(); - - // if current_item is array/object, move - // its children to the stack to be processed later - if (current_item.is_array()) + else { - std::move(current_item.m_value.array->begin(), current_item.m_value.array->end(), - std::back_inserter(stack)); - - current_item.m_value.array->clear(); - } - else if (current_item.is_object()) - { - for (auto&& it : *current_item.m_value.object) + stack.reserve(object->size()); + for (auto&& it : *object) { stack.push_back(std::move(it.second)); } - - current_item.m_value.object->clear(); } - // it's now safe that current_item get destructed - // since it doesn't have any children + while (!stack.empty()) + { + // move the last item to local variable to be processed + basic_json current_item(std::move(stack.back())); + stack.pop_back(); + + // if current_item is array/object, move + // its children to the stack to be processed later + if (current_item.is_array()) + { + std::move(current_item.m_value.array->begin(), current_item.m_value.array->end(), std::back_inserter(stack)); + + current_item.m_value.array->clear(); + } + else if (current_item.is_object()) + { + for (auto&& it : *current_item.m_value.object) + { + stack.push_back(std::move(it.second)); + } + + current_item.m_value.object->clear(); + } + + // it's now safe that current_item get destructed + // since it doesn't have any children + } } switch (t) diff --git a/test/src/unit-regression2.cpp b/test/src/unit-regression2.cpp index 4d48e4765..ef9d7ee54 100644 --- a/test/src/unit-regression2.cpp +++ b/test/src/unit-regression2.cpp @@ -594,4 +594,30 @@ TEST_CASE("regression tests 2") } } } + + SECTION("issue #2865 - ASAN detects memory leaks") + { + // the code below is expected to not leak memory + { + nlohmann::json o; + std::string s = "bar"; + + nlohmann::to_json(o["foo"], s); + + nlohmann::json p = o; + + // call to_json with a non-null JSON value + nlohmann::to_json(p["foo"], s); + } + + { + nlohmann::json o; + std::string s = "bar"; + + nlohmann::to_json(o["foo"], s); + + // call to_json with a non-null JSON value + nlohmann::to_json(o["foo"], s); + } + } }