Use memmove instead of strcat

strcat does not allow overlapping ranges; we didn't have a test for this but now we do. As an added bonus, this also means we only compute the length of each fragment once now.
Zoynels · Sep 6, 2023 · 30adb17 · 30adb17
1 parent 58616a2
commit 30adb17
Show file tree

Hide file tree

Showing 2 changed files with 15 additions and 13 deletions.
diff --git a/src/pugixml.cpp b/src/pugixml.cpp
@@ -270,18 +270,6 @@ PUGI_IMPL_NS_BEGIN
 		return static_cast<size_t>(end - s);
 	#endif
 	}
-
-	// Append one string to another
-	PUGI_IMPL_FN void strconcat(char_t* dst, const char_t* src)
-	{
-		assert(dst && src);
-
-	#ifdef PUGIXML_WCHAR_MODE
-		wcscat(dst, src);
-	#else
-		strcat(dst, src);
-	#endif
-	}
 PUGI_IMPL_NS_END
 
 // auto_ptr-like object for exception recovery
@@ -3498,8 +3486,14 @@ PUGI_IMPL_NS_BEGIN
 						{
 							assert(merged >= cursor->first_child->prev_sibling_c->value);
 
+							// Catch up to the end of last parsed value; only needed for the first fragment.
 							merged += strlength(merged);
-							strconcat(merged, parsed_pcdata); // Append PCDATA to the previous one.
+
+							size_t length = strlength(parsed_pcdata);
+
+							// Must use memmove instead of memcpy as this move may overlap
+							memmove(merged, parsed_pcdata, (length + 1) * sizeof(char_t));
+							merged += length;
 						}
 						else
 						{

diff --git a/tests/test_parse.cpp b/tests/test_parse.cpp
@@ -1350,6 +1350,14 @@ TEST(parse_merge_pcdata_append)
 	CHECK_STRING(doc.child(STR("node")).first_child().value(), STR("hello world"));
 }
 
+TEST(parse_merge_pcdata_overlap)
+{
+	xml_document doc;
+	xml_parse_result res = doc.load_string(STR("<node>short <!-- --> this string is very long so long that copying it will overlap itself</node>"), parse_merge_pcdata);
+	CHECK(res);
+	CHECK_STRING(doc.child_value(STR("node")), STR("short  this string is very long so long that copying it will overlap itself"));
+}
+
 TEST(parse_encoding_detect)
 {
 	char test[] = "<?xml version='1.0' encoding='utf-8'?><n/>";