From 33a75c734bc176d92aa9c958ff3df80424c01f78 Mon Sep 17 00:00:00 2001 From: Arseny Kapoulkine Date: Mon, 16 May 2022 19:10:12 -0700 Subject: Fix memory leak during OOM in convert_buffer This is the same fix as #497, but we're using auto_deleter instead because if allocation function throws, we can't rely on an explicit call to deallocate. Comes along with two tests that validate the behavior. --- src/pugixml.cpp | 12 +++++++----- tests/test_document.cpp | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/src/pugixml.cpp b/src/pugixml.cpp index 13781e1..8ff879b 100644 --- a/src/pugixml.cpp +++ b/src/pugixml.cpp @@ -4707,16 +4707,18 @@ PUGI__NS_BEGIN // get actual encoding xml_encoding buffer_encoding = impl::get_buffer_encoding(encoding, contents, size); + // if convert_buffer below throws bad_alloc, we still need to deallocate contents if we own it + auto_deleter contents_guard(own ? contents : NULL, xml_memory::deallocate); + // get private buffer char_t* buffer = 0; size_t length = 0; // coverity[var_deref_model] - if (!impl::convert_buffer(buffer, length, buffer_encoding, contents, size, is_mutable)) - { - if (own && contents) impl::xml_memory::deallocate(contents); - return impl::make_parse_result(status_out_of_memory); - } + if (!impl::convert_buffer(buffer, length, buffer_encoding, contents, size, is_mutable)) return impl::make_parse_result(status_out_of_memory); + + // after this we either deallocate contents (below) or hold on to it via doc->buffer, so we don't need to guard it + contents_guard.data = NULL; // delete original buffer if we performed a conversion if (own && buffer != contents && contents) impl::xml_memory::deallocate(contents); diff --git a/tests/test_document.cpp b/tests/test_document.cpp index 5379092..0e0bad4 100644 --- a/tests/test_document.cpp +++ b/tests/test_document.cpp @@ -1817,3 +1817,39 @@ TEST(document_move_assign_empty) CHECK_NODE(doc, STR("")); } #endif + +TEST(document_load_buffer_convert_out_of_memory) +{ + const char* source = "\xe7"; + size_t size = strlen(source); + + test_runner::_memory_fail_threshold = 1; + + xml_document doc; + + xml_parse_result result; + result.status = status_out_of_memory; + CHECK_ALLOC_FAIL(result = doc.load_buffer(source, size, pugi::parse_default, pugi::encoding_latin1)); + + CHECK(result.status == status_out_of_memory); +} + +TEST(document_load_buffer_own_convert_out_of_memory) +{ + const char* source = "\xe7"; + size_t size = strlen(source); + + void* buffer = pugi::get_memory_allocation_function()(size); + CHECK(buffer); + memcpy(buffer, source, size); + + test_runner::_memory_fail_threshold = 1; + + xml_document doc; + + xml_parse_result result; + result.status = status_out_of_memory; + CHECK_ALLOC_FAIL(result = doc.load_buffer_inplace_own(buffer, size, pugi::parse_default, pugi::encoding_latin1)); + + CHECK(result.status == status_out_of_memory); +} -- cgit v1.1 From 832a4f4914cc7830b1a7ec675b1c84290bdf2808 Mon Sep 17 00:00:00 2001 From: Arseny Kapoulkine Date: Mon, 16 May 2022 19:14:29 -0700 Subject: Use more idiomatic code in this codebase --- src/pugixml.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/pugixml.cpp b/src/pugixml.cpp index 8ff879b..47764c8 100644 --- a/src/pugixml.cpp +++ b/src/pugixml.cpp @@ -4708,7 +4708,7 @@ PUGI__NS_BEGIN xml_encoding buffer_encoding = impl::get_buffer_encoding(encoding, contents, size); // if convert_buffer below throws bad_alloc, we still need to deallocate contents if we own it - auto_deleter contents_guard(own ? contents : NULL, xml_memory::deallocate); + auto_deleter contents_guard(own ? contents : 0, xml_memory::deallocate); // get private buffer char_t* buffer = 0; @@ -4718,7 +4718,7 @@ PUGI__NS_BEGIN if (!impl::convert_buffer(buffer, length, buffer_encoding, contents, size, is_mutable)) return impl::make_parse_result(status_out_of_memory); // after this we either deallocate contents (below) or hold on to it via doc->buffer, so we don't need to guard it - contents_guard.data = NULL; + contents_guard.release(); // delete original buffer if we performed a conversion if (own && buffer != contents && contents) impl::xml_memory::deallocate(contents); -- cgit v1.1