From 8dea806cd961df49b5af60644bd3dff454448335 Mon Sep 17 00:00:00 2001 From: eelke Date: Sun, 18 Oct 2026 13:38:41 +0200 Subject: [PATCH] Fix memory management issues in Pgsql_Params leave sources of moves in a valid state. --- pgsql/Pgsql_Params.cpp | 74 ++++++++++++----------- pgsql/Pgsql_Params.h | 20 +++---- tests/PgsqlTests/CMakeLists.txt | 1 + tests/PgsqlTests/tst_Pgsql_Params.cpp | 85 +++++++++++++++++++++++++++ 4 files changed, 132 insertions(+), 48 deletions(-) create mode 100644 tests/PgsqlTests/tst_Pgsql_Params.cpp diff --git a/pgsql/Pgsql_Params.cpp b/pgsql/Pgsql_Params.cpp index d59c55b..0832e92 100644 --- a/pgsql/Pgsql_Params.cpp +++ b/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 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 data, Oid oid) { m_paramTypes.push_back(oid); - m_paramValues.push_back(data); - m_paramLengths.push_back(data ? static_cast(strlen(data)) + 1 : 0); + m_paramLengths.push_back(data ? static_cast(std::strlen(data.get())) + 1 : 0); m_paramFormats.push_back(0); - return Param(*this, static_cast(m_paramValues.size()) - 1); + pushValue(std::move(data)); + return Param(*this, static_cast(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(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 p; if (data) { - len = static_cast(std::strlen(data)); - p = new char[len+1]; - std::memcpy(p, data, len); + const size_t len = std::strlen(data); + p = std::make_unique(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 ¶ms) 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 data) { - const int n = static_cast(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(len + 1); + std::memcpy(p.get(), e.get(), len + 1); + pushValue(std::move(p)); } else { - m_paramValues.push_back(nullptr); + pushValue(nullptr); } } } diff --git a/pgsql/Pgsql_Params.h b/pgsql/Pgsql_Params.h index 83d349c..f5a59c3 100644 --- a/pgsql/Pgsql_Params.h +++ b/pgsql/Pgsql_Params.h @@ -1,6 +1,7 @@ #ifndef PGSQL_PARAMS_H #define PGSQL_PARAMS_H +#include #include #include #include @@ -56,27 +57,26 @@ namespace Pgsql { const int* formats() const { return m_paramFormats.data(); } private: + using t_paramData = std::vector>; using t_paramValues = std::vector; - 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 data); std::vector 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 m_paramLengths; ///< postgresql ignores lengths for text parameters but we will it anyway for efficient copying std::vector 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 data, Oid oid=varchar_oid); }; } // end namespace Pgsql diff --git a/tests/PgsqlTests/CMakeLists.txt b/tests/PgsqlTests/CMakeLists.txt index 34071a1..ce9e9c7 100644 --- a/tests/PgsqlTests/CMakeLists.txt +++ b/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 diff --git a/tests/PgsqlTests/tst_Pgsql_Params.cpp b/tests/PgsqlTests/tst_Pgsql_Params.cpp new file mode 100644 index 0000000..6ef2fce --- /dev/null +++ b/tests/PgsqlTests/tst_Pgsql_Params.cpp @@ -0,0 +1,85 @@ +#include +#include +#include +#include +#include +#include "Pgsql_Params.h" +#include "PrintTo_Qt.h" + +using namespace testing; +using namespace Pgsql; + +namespace { + void expectValues(const Params &p, const std::vector &expected) + { + ASSERT_EQ(p.size(), static_cast(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{}); + src.add("v"); + + Params dst(src); + ASSERT_EQ(dst.size(), 2); + EXPECT_EQ(dst.values()[0], nullptr); + EXPECT_STREQ(dst.values()[1], "v"); +} -- 2.47.2