From e4d8dc02e87614854f82f2d960c1e38a10511519 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Art=C3=B6m=20Bakri=20Al-Sarmini?= <3sz3tt+git@gmail.com> Date: Thu, 2 Apr 2020 15:20:25 +0300 Subject: [PATCH 1/5] Fixes #1971 (memory leak in basic_json::push_back) --- include/nlohmann/json.hpp | 4 +-- single_include/nlohmann/json.hpp | 4 +-- test/src/unit-allocator.cpp | 48 ++++++++++++++++++++++++++++++++ 3 files changed, 50 insertions(+), 6 deletions(-) diff --git a/include/nlohmann/json.hpp b/include/nlohmann/json.hpp index b72410d7..da3d57b1 100644 --- a/include/nlohmann/json.hpp +++ b/include/nlohmann/json.hpp @@ -4878,9 +4878,7 @@ class basic_json // add element to array (move semantics) m_value.array->push_back(std::move(val)); - // invalidate object: mark it null so we do not call the destructor - // cppcheck-suppress accessMoved - val.m_type = value_t::null; + // if val is moved from, basic_json move constructor marks it null so we do not call the destructor } /*! diff --git a/single_include/nlohmann/json.hpp b/single_include/nlohmann/json.hpp index 77bd1739..4d0b84f4 100644 --- a/single_include/nlohmann/json.hpp +++ b/single_include/nlohmann/json.hpp @@ -19421,9 +19421,7 @@ class basic_json // add element to array (move semantics) m_value.array->push_back(std::move(val)); - // invalidate object: mark it null so we do not call the destructor - // cppcheck-suppress accessMoved - val.m_type = value_t::null; + // if val is moved from, basic_json move constructor marks it null so we do not call the destructor } /*! diff --git a/test/src/unit-allocator.cpp b/test/src/unit-allocator.cpp index 3518c4ae..c751d3c5 100644 --- a/test/src/unit-allocator.cpp +++ b/test/src/unit-allocator.cpp @@ -234,3 +234,51 @@ TEST_CASE("controlled bad_alloc") } } } + +namespace +{ +template +struct allocator_no_forward +{ + typedef std::remove_const_t value_type; + template + struct rebind + { + typedef allocator_no_forward other; + }; + + T* allocate(size_t sz) + { + return static_cast(malloc(sz * sizeof(T))); + } + + void deallocate(T* p, size_t) + { + free(p); + } + + void construct(T* p, const T& arg) + { + ::new (static_cast(p)) T(arg); + } +}; +} + +TEST_CASE("bad my_allocator::construct") +{ + SECTION("my_allocator::construct doesn't forward") + { + using bad_alloc_json = nlohmann::basic_json; + + bad_alloc_json json; + json["test"] = bad_alloc_json::array_t(); + json["test"].push_back("should not leak"); + } +} From a74a031bba2106c846a97d294813919674ba25e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Art=C3=B6m=20Bakri=20Al-Sarmini?= <3sz3tt+git@gmail.com> Date: Thu, 2 Apr 2020 15:47:08 +0300 Subject: [PATCH 2/5] Fix build error --- test/src/unit-allocator.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/src/unit-allocator.cpp b/test/src/unit-allocator.cpp index c751d3c5..f817897c 100644 --- a/test/src/unit-allocator.cpp +++ b/test/src/unit-allocator.cpp @@ -240,7 +240,7 @@ namespace template struct allocator_no_forward { - typedef std::remove_const_t value_type; + typedef T value_type; template struct rebind { From 4ce31695f1f05095549350ad966b16994ce828d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Art=C3=B6m=20Bakri=20Al-Sarmini?= <3sz3tt+git@gmail.com> Date: Wed, 8 Apr 2020 00:26:43 +0300 Subject: [PATCH 3/5] Fixed formatting, trying to fix msvc build error in appveyor --- test/src/unit-allocator.cpp | 41 ++++++++++++++----------------------- 1 file changed, 15 insertions(+), 26 deletions(-) diff --git a/test/src/unit-allocator.cpp b/test/src/unit-allocator.cpp index f817897c..4cac5872 100644 --- a/test/src/unit-allocator.cpp +++ b/test/src/unit-allocator.cpp @@ -238,28 +238,17 @@ TEST_CASE("controlled bad_alloc") namespace { template -struct allocator_no_forward +struct allocator_no_forward : std::allocator { - typedef T value_type; template - struct rebind - { + struct rebind { typedef allocator_no_forward other; }; - T* allocate(size_t sz) + template + void construct(T* p, const Args&... args) { - return static_cast(malloc(sz * sizeof(T))); - } - - void deallocate(T* p, size_t) - { - free(p); - } - - void construct(T* p, const T& arg) - { - ::new (static_cast(p)) T(arg); + ::new (static_cast(p)) T(args...); } }; } @@ -269,16 +258,16 @@ TEST_CASE("bad my_allocator::construct") SECTION("my_allocator::construct doesn't forward") { using bad_alloc_json = nlohmann::basic_json; + std::vector, + std::string, + bool, + std::int64_t, + std::uint64_t, + double, + allocator_no_forward>; - bad_alloc_json json; - json["test"] = bad_alloc_json::array_t(); - json["test"].push_back("should not leak"); + bad_alloc_json json; + json["test"] = bad_alloc_json::array_t(); + json["test"].push_back("should not leak"); } } From fec0bdd93b7197b3069af787957f0d5d9383b73f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Art=C3=B6m=20Bakri=20Al-Sarmini?= <3sz3tt+git@gmail.com> Date: Wed, 8 Apr 2020 00:42:03 +0300 Subject: [PATCH 4/5] still fixing --- test/src/unit-allocator.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/test/src/unit-allocator.cpp b/test/src/unit-allocator.cpp index 4cac5872..67281a82 100644 --- a/test/src/unit-allocator.cpp +++ b/test/src/unit-allocator.cpp @@ -240,9 +240,14 @@ namespace template struct allocator_no_forward : std::allocator { - template + using std::allocator::allocator; + + template + allocator_no_forward(allocator_no_forward) {} + + template struct rebind { - typedef allocator_no_forward other; + using other = allocator_no_forward; }; template From 8db02bcc55db03db1a0b240391472e71289c938e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Art=C3=B6m=20Bakri=20Al-Sarmini?= <3sz3tt+git@gmail.com> Date: Wed, 8 Apr 2020 15:53:14 +0300 Subject: [PATCH 5/5] Fix for gcc --- test/src/unit-allocator.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/src/unit-allocator.cpp b/test/src/unit-allocator.cpp index 67281a82..5d48b2ea 100644 --- a/test/src/unit-allocator.cpp +++ b/test/src/unit-allocator.cpp @@ -240,8 +240,7 @@ namespace template struct allocator_no_forward : std::allocator { - using std::allocator::allocator; - + allocator_no_forward() {} template allocator_no_forward(allocator_no_forward) {} @@ -253,6 +252,7 @@ struct allocator_no_forward : std::allocator template void construct(T* p, const Args&... args) { + // force copy even if move is available ::new (static_cast(p)) T(args...); } };