Merge pull request 'Fix memory management issues in Pgsql_Params leave sources of moves in a valid state.' (#91) from fix-Pgsql-Params-issues into main

Reviewed-on: #91

pgsql/Pgsql_Params.cpp

@@ -11,77 +11,72 @@ Params::Params(const Params& rhs)
 	, m_paramLengths(rhs.m_paramLengths)
 	, m_paramFormats(rhs.m_paramFormats)
 {
-	//std::vector<const char *> m_paramValues;
-    appendValues(rhs.m_paramValues);
+	appendValues(rhs);
 }
 
 Params& Params::operator=(const Params& rhs)
 {
 	if (&rhs != this) {
-		m_paramTypes = rhs.m_paramTypes;
-		m_paramLengths = rhs.m_paramLengths;
-		m_paramFormats = rhs.m_paramFormats;
-        appendValues(rhs.m_paramValues);
+		Params tmp(rhs);
+		*this = std::move(tmp);
 	}
 	return *this;
 }
 
 Params::Params(Params&& rhs)
 	: m_paramTypes(std::move(rhs.m_paramTypes))
+	, m_paramData(std::move(rhs.m_paramData))
 	, m_paramValues(std::move(rhs.m_paramValues))
 	, m_paramLengths(std::move(rhs.m_paramLengths))
 	, m_paramFormats(std::move(rhs.m_paramFormats))
 {
-	rhs.m_paramValues.clear(); // just in case it was copied instead of moved
+	rhs.clear();
 }
 
 Params& Params::operator=(Params&& rhs)
 {
 	if (&rhs != this) {
 		m_paramTypes   = std::move(rhs.m_paramTypes);
+		m_paramData    = std::move(rhs.m_paramData); // releases the buffers this object held before
 		m_paramValues  = std::move(rhs.m_paramValues);
-		rhs.m_paramValues.clear(); // just in case it was copied instead of moved
 		m_paramLengths = std::move(rhs.m_paramLengths);
 		m_paramFormats = std::move(rhs.m_paramFormats);
+		rhs.clear();
 	}
 	return *this;
 }
 
-Params::~Params()
-{
-	deleteValues();
-}
+Params::~Params() = default;
 
-Param Params::addText(const char *data, Oid oid)
+Param Params::addText(std::unique_ptr<char[]> data, Oid oid)
 {
 	m_paramTypes.push_back(oid);
-	m_paramValues.push_back(data);
-    m_paramLengths.push_back(data ? static_cast<int>(strlen(data)) + 1 : 0);
+	m_paramLengths.push_back(data ? static_cast<int>(std::strlen(data.get())) + 1 : 0);
 	m_paramFormats.push_back(0);
-    return Param(*this, static_cast<int>(m_paramValues.size()) - 1);
+	pushValue(std::move(data));
+	return Param(*this, static_cast<int>(m_paramValues.size()) - 1);
 }
 
 Param Params::add(const QString &s, Oid oid)
 {
 	auto ba = s.toUtf8();
 	const int len = ba.size();
-	char * p = new char[len+1];
-	std::memcpy(p, ba.data(), len);
+	auto p = std::make_unique<char[]>(len + 1);
+	std::memcpy(p.get(), ba.data(), len);
 	p[len] = 0;
-	return addText(p, oid);
+	return addText(std::move(p), oid);
 }
 
 Param Params::add(const char *data, Oid oid)
 {
-	char * p = nullptr;
-	int len = 0;
+	std::unique_ptr<char[]> p;
 	if (data) {
-        len = static_cast<int>(std::strlen(data));
-		p = new char[len+1];
-		std::memcpy(p, data, len);
+		const size_t len = std::strlen(data);
+		p = std::make_unique<char[]>(len + 1);
+		std::memcpy(p.get(), data, len);
 		p[len] = 0;
 	}
-	return addText(p, oid);
+	return addText(std::move(p), oid);
 }
 
 Param Params::add(int v, Oid oid)
@@ -101,32 +96,35 @@ void Params::addParams(const Params &params)
     concatContainers(m_paramTypes, params.m_paramTypes);
     concatContainers(m_paramLengths, params.m_paramLengths);
     concatContainers(m_paramFormats, params.m_paramFormats);
-    appendValues(params.m_paramValues);
+    appendValues(params);
 }
 
 void Params::clear()
 {
 	m_paramTypes.clear();
-	deleteValues();
+	m_paramData.clear();
 	m_paramValues.clear();
 	m_paramLengths.clear();
 	m_paramFormats.clear();
 }
 
-void Params::appendValues(const t_paramValues &r)
+void Params::pushValue(std::unique_ptr<char[]> data)
 {
-    const int n = static_cast<int>(r.size());
-    const size_t ofs = m_paramValues.size();
-    m_paramValues.reserve(ofs + n);
-	for (int i = 0; i < n; ++i) {
-		if (r[i]) {
-            const int len = m_paramLengths[ofs + i];
-			char * p = new char[len+1];
-			std::memcpy(p, r[i], len+1);
-			m_paramValues.push_back(p);
+	m_paramValues.push_back(data.get());
+	m_paramData.push_back(std::move(data));
+}
+
+void Params::appendValues(const Params &src)
+{
+	for (const auto &e : src.m_paramData) {
+		if (e) {
+			const size_t len = std::strlen(e.get());
+			auto p = std::make_unique<char[]>(len + 1);
+			std::memcpy(p.get(), e.get(), len + 1);
+			pushValue(std::move(p));
 		}
 		else {
-			m_paramValues.push_back(nullptr);
+			pushValue(nullptr);
 		}
 	}
 }

pgsql/Pgsql_Params.h

@@ -1,6 +1,7 @@
 #ifndef PGSQL_PARAMS_H
 #define PGSQL_PARAMS_H
 
+#include <memory>
 #include <vector>
 #include <QString>
 #include <libpq-fe.h>
@@ -56,27 +57,26 @@ namespace Pgsql {
 		const int* formats() const { return m_paramFormats.data(); }
 
 	private:
+		using t_paramData = std::vector<std::unique_ptr<char[]>>;
 		using t_paramValues = std::vector<const char *>;
 
-		void deleteValues()
-		{
-			for (auto e : m_paramValues)
-				delete[] e;
-		}
+		/** \brief Deep copies the values of src, the other lists must already have been updated */
+        void appendValues(const Params &src);
 
-		/* Assumes other lists already have been copied */
-        void appendValues(const t_paramValues &r);
+		/** \brief Takes ownership of data and makes it visible through values() */
+		void pushValue(std::unique_ptr<char[]> data);
 
 		std::vector<Oid> m_paramTypes;
-		t_paramValues m_paramValues;
+		t_paramData m_paramData; ///< owns the text buffers, nullptr for NULL values
+		t_paramValues m_paramValues; ///< points into m_paramData, handed to libpq
 		std::vector<int> m_paramLengths; ///< postgresql ignores lengths for text parameters but we will it anyway for efficient copying
 		std::vector<int> m_paramFormats;
 
 		/** \brief Add a parameter to the list.
 		 *
-		 * The class takes ownership of data and will try to delete[] it.
+		 * The class takes ownership of data.
 		 */
-		Param addText(const char *data, Oid oid=varchar_oid);
+		Param addText(std::unique_ptr<char[]> data, Oid oid=varchar_oid);
 	};
 
 } // end namespace Pgsql

tests/PgsqlTests/CMakeLists.txt

@@ -3,6 +3,7 @@ add_executable(PgsqlTests
     tst_Value.cpp
     tst_ArrayParser.cpp
     tst_Pgsql_oids.cpp
+    tst_Pgsql_Params.cpp
     )
 
 target_link_libraries(PgsqlTests PRIVATE

tests/PgsqlTests/tst_Pgsql_Params.cpp

@@ -0,0 +1,85 @@
+#include <gtest/gtest.h>
+#include <gmock/gmock-matchers.h>
+#include <string>
+#include <utility>
+#include <vector>
+#include "Pgsql_Params.h"
+#include "PrintTo_Qt.h"
+
+using namespace testing;
+using namespace Pgsql;
+
+namespace {
+	void expectValues(const Params &p, const std::vector<std::string> &expected)
+	{
+		ASSERT_EQ(p.size(), static_cast<int>(expected.size()));
+		for (int i = 0; i < p.size(); ++i) {
+			ASSERT_NE(p.values()[i], nullptr);
+			EXPECT_STREQ(p.values()[i], expected[i].c_str());
+		}
+	}
+}
+
+TEST(Pgsql_Params, CopyAssignIntoNonEmpty)
+{
+	Params src;
+	src.add("a");
+	src.add("bb");
+
+	Params dst;
+	dst.add("x");
+	dst.add("y");
+	dst.add("z");
+
+	dst = src;
+	expectValues(dst, {"a", "bb"});
+	expectValues(src, {"a", "bb"});
+}
+
+TEST(Pgsql_Params, CopyAssignIntoEmpty)
+{
+	Params src;
+	src.add("a");
+	src.add("bb");
+
+	Params dst;
+	dst = src;
+	expectValues(dst, {"a", "bb"});
+}
+
+TEST(Pgsql_Params, MoveAssignIntoNonEmpty)
+{
+	Params src;
+	src.add("a");
+	src.add("bb");
+
+	Params dst;
+	dst.add("x");
+	dst.add("y");
+	dst.add("z");
+
+	dst = std::move(src);
+	expectValues(dst, {"a", "bb"});
+}
+
+TEST(Pgsql_Params, CopyConstructorDoesNotAliasSource)
+{
+	Params src;
+	src.add("abc");
+
+	Params dst(src);
+	src.clear();
+	expectValues(dst, {"abc"});
+}
+
+TEST(Pgsql_Params, CopyPreservesNull)
+{
+	Params src;
+	src.add(std::optional<std::string>{});
+	src.add("v");
+
+	Params dst(src);
+	ASSERT_EQ(dst.size(), 2);
+	EXPECT_EQ(dst.values()[0], nullptr);
+	EXPECT_STREQ(dst.values()[1], "v");
+}