From 97559bb1b2f6c916092d91d2255acfce6e2849a4 Mon Sep 17 00:00:00 2001 From: Niels Lohmann Date: Thu, 8 Mar 2018 07:36:56 +0100 Subject: [PATCH 1/3] :hammer: trying to fix the leak Part 1: properly use forwarding --- include/nlohmann/detail/input/lexer.hpp | 2 +- include/nlohmann/json.hpp | 6 +++--- single_include/nlohmann/json.hpp | 8 ++++---- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/include/nlohmann/detail/input/lexer.hpp b/include/nlohmann/detail/input/lexer.hpp index 75001652..ea116093 100644 --- a/include/nlohmann/detail/input/lexer.hpp +++ b/include/nlohmann/detail/input/lexer.hpp @@ -1130,7 +1130,7 @@ scan_number_done: } /// return current string value (implicitly resets the token; useful only once) - std::string move_string() + std::string&& move_string() { return std::move(token_buffer); } diff --git a/include/nlohmann/json.hpp b/include/nlohmann/json.hpp index 91de782f..70f26747 100644 --- a/include/nlohmann/json.hpp +++ b/include/nlohmann/json.hpp @@ -953,7 +953,7 @@ class basic_json /// constructor for rvalue strings json_value(string_t&& value) { - string = create(std::move(value)); + string = create(std::forward < string_t&& > (value)); } /// constructor for objects @@ -965,7 +965,7 @@ class basic_json /// constructor for rvalue objects json_value(object_t&& value) { - object = create(std::move(value)); + object = create(std::forward < object_t&& > (value)); } /// constructor for arrays @@ -977,7 +977,7 @@ class basic_json /// constructor for rvalue arrays json_value(array_t&& value) { - array = create(std::move(value)); + array = create(std::forward < array_t&& > (value)); } void destroy(value_t t) noexcept diff --git a/single_include/nlohmann/json.hpp b/single_include/nlohmann/json.hpp index 5ef88282..afbfa076 100644 --- a/single_include/nlohmann/json.hpp +++ b/single_include/nlohmann/json.hpp @@ -2969,7 +2969,7 @@ scan_number_done: } /// return current string value (implicitly resets the token; useful only once) - std::string move_string() + std::string&& move_string() { return std::move(token_buffer); } @@ -10561,7 +10561,7 @@ class basic_json /// constructor for rvalue strings json_value(string_t&& value) { - string = create(std::move(value)); + string = create(std::forward < string_t&& > (value)); } /// constructor for objects @@ -10573,7 +10573,7 @@ class basic_json /// constructor for rvalue objects json_value(object_t&& value) { - object = create(std::move(value)); + object = create(std::forward < object_t&& > (value)); } /// constructor for arrays @@ -10585,7 +10585,7 @@ class basic_json /// constructor for rvalue arrays json_value(array_t&& value) { - array = create(std::move(value)); + array = create(std::forward < array_t&& > (value)); } void destroy(value_t t) noexcept From aa8fc2a41cec098c6f6de58f40bc7e8ff1fda16f Mon Sep 17 00:00:00 2001 From: Niels Lohmann Date: Thu, 8 Mar 2018 17:11:15 +0100 Subject: [PATCH 2/3] :ambulance: hopefully fixed the memory leak --- include/nlohmann/detail/input/parser.hpp | 1 + single_include/nlohmann/json.hpp | 1 + 2 files changed, 2 insertions(+) diff --git a/include/nlohmann/detail/input/parser.hpp b/include/nlohmann/detail/input/parser.hpp index 63e8541f..58d42bbe 100644 --- a/include/nlohmann/detail/input/parser.hpp +++ b/include/nlohmann/detail/input/parser.hpp @@ -403,6 +403,7 @@ class parser if (keep and callback and not callback(depth, parse_event_t::value, result)) { + result.m_value.destroy(result.m_type); result.m_type = value_t::discarded; } } diff --git a/single_include/nlohmann/json.hpp b/single_include/nlohmann/json.hpp index afbfa076..8726ccd8 100644 --- a/single_include/nlohmann/json.hpp +++ b/single_include/nlohmann/json.hpp @@ -3526,6 +3526,7 @@ class parser if (keep and callback and not callback(depth, parse_event_t::value, result)) { + result.m_value.destroy(result.m_type); result.m_type = value_t::discarded; } } From 9918523077311b89d6d2ccb2e036503e5ffea437 Mon Sep 17 00:00:00 2001 From: Niels Lohmann Date: Fri, 9 Mar 2018 21:31:46 +0100 Subject: [PATCH 3/3] :memo: cleanup after #1001 --- README.md | 1 + test/src/unit-class_parser.cpp | 85 ---------------------------------- test/src/unit-regression.cpp | 83 +++++++++++++++++++++++++++++++++ 3 files changed, 84 insertions(+), 85 deletions(-) diff --git a/README.md b/README.md index 2cb8603b..1d065407 100644 --- a/README.md +++ b/README.md @@ -974,6 +974,7 @@ I deeply appreciate the help of the following people. - [johnfb](https://github.com/johnfb) found a bug in the implementation of CBOR's indefinite length strings. - [Paul Fultz II](https://github.com/pfultz2) added a note on the cget package manager. - [Wilson Lin](https://github.com/wla80) made the integration section of the README more concise. +- [RalfBielig](https://github.com/ralfbielig) detected and fixed a memory leak in the parser callback. Thanks a lot for helping out! Please [let me know](mailto:mail@nlohmann.me) if I forgot someone. diff --git a/test/src/unit-class_parser.cpp b/test/src/unit-class_parser.cpp index 2efc98d1..3e309469 100644 --- a/test/src/unit-class_parser.cpp +++ b/test/src/unit-class_parser.cpp @@ -1415,91 +1415,6 @@ TEST_CASE("parser class") }); CHECK(j_empty_array == json()); } - - /* - SECTION("skip in GeoJSON") - { - auto geojsonExample = R"( - { "type": "FeatureCollection", - "features": [ - { "type": "Feature", - "geometry": {"type": "Point", "coordinates": [102.0, 0.5]}, - "properties": {"prop0": "value0"} - }, - { "type": "Feature", - "geometry": { - "type": "LineString", - "coordinates": [ - [102.0, 0.0], [103.0, 1.0], [104.0, 0.0], [105.0, 1.0] - ] - }, - "properties": { - "prop0": "value0", - "prop1": 0.0 - } - }, - { "type": "Feature", - "geometry": { - "type": "Polygon", - "coordinates": [ - [ [100.0, 0.0], [101.0, 0.0], [101.0, 1.0], - [100.0, 1.0], [100.0, 0.0] ] - ] - }, - "properties": { - "prop0": "value0", - "prop1": {"this": "that"} - } - } - ] - })"; - - json::parser_callback_t cb = [&](int, json::parse_event_t event, json & parsed) - { - // skip uninteresting events - if (event == json::parse_event_t::value and !parsed.is_primitive()) - { - return false; - } - - switch (event) - { - case json::parse_event_t::key: - { - return true; - } - case json::parse_event_t::value: - { - return false; - } - case json::parse_event_t::object_start: - { - return true; - } - case json::parse_event_t::object_end: - { - return false; - } - case json::parse_event_t::array_start: - { - return true; - } - case json::parse_event_t::array_end: - { - return false; - } - - default: - { - return true; - } - } - }; - - auto j = json::parse(geojsonExample, cb, true); - CHECK(j == json()); - } - */ } SECTION("constructing from contiguous containers") diff --git a/test/src/unit-regression.cpp b/test/src/unit-regression.cpp index c84c2750..604def6c 100644 --- a/test/src/unit-regression.cpp +++ b/test/src/unit-regression.cpp @@ -1514,4 +1514,87 @@ TEST_CASE("regression tests") CHECK(ff.x == 3); nlohmann::json nj = lj; // This line works as expected } + + SECTION("issue #1001 - Fix memory leak during parser callback") + { + auto geojsonExample = R"( + { "type": "FeatureCollection", + "features": [ + { "type": "Feature", + "geometry": {"type": "Point", "coordinates": [102.0, 0.5]}, + "properties": {"prop0": "value0"} + }, + { "type": "Feature", + "geometry": { + "type": "LineString", + "coordinates": [ + [102.0, 0.0], [103.0, 1.0], [104.0, 0.0], [105.0, 1.0] + ] + }, + "properties": { + "prop0": "value0", + "prop1": 0.0 + } + }, + { "type": "Feature", + "geometry": { + "type": "Polygon", + "coordinates": [ + [ [100.0, 0.0], [101.0, 0.0], [101.0, 1.0], + [100.0, 1.0], [100.0, 0.0] ] + ] + }, + "properties": { + "prop0": "value0", + "prop1": {"this": "that"} + } + } + ] + })"; + + json::parser_callback_t cb = [&](int, json::parse_event_t event, json & parsed) + { + // skip uninteresting events + if (event == json::parse_event_t::value and !parsed.is_primitive()) + { + return false; + } + + switch (event) + { + case json::parse_event_t::key: + { + return true; + } + case json::parse_event_t::value: + { + return false; + } + case json::parse_event_t::object_start: + { + return true; + } + case json::parse_event_t::object_end: + { + return false; + } + case json::parse_event_t::array_start: + { + return true; + } + case json::parse_event_t::array_end: + { + return false; + } + + default: + { + return true; + } + } + }; + + auto j = json::parse(geojsonExample, cb, true); + CHECK(j == json()); + } }