Skip to content

XML: Use RAII wrappers instead of manual memory management #5767

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Jinming-Hu merged 11 commits into from
Jul 30, 2024
Merged

XML: Use RAII wrappers instead of manual memory management #5767

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
15b47d9
Use RAII wrappers instead of manual memory management
alesapin Jan 10, 2023
54234f8
Remove bug
alesapin Jan 11, 2023
fbfa476
Why am I so stupid
alesapin Jan 11, 2023
3a832a4
make initialization public
CurtizJ Jan 30, 2023
81bb45d
remove global static RAII wrapper
CurtizJ Jan 31, 2023
2b6a01b
Fixups
rschu1ze Jul 7, 2024
e18ad74
Incorporate review feedback
rschu1ze Jul 8, 2024
9469d0f
Fix Windows build (hopefully)
rschu1ze Jul 22, 2024
634e4de
Revert to previous libxml init/deinit
rschu1ze Jul 23, 2024
d59841b
Try to fix build
rschu1ze Jul 23, 2024
01e4ccc
Fix clang-format
rschu1ze Jul 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 9 additions & 16 deletions sdk/storage/azure-storage-common/inc/azure/storage/common/internal/xml_wrapper.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#pragma once

#include <cstdint>
#include <memory>
#include <string>

namespace Azure { namespace Storage { namespace _internal {
Expand Down Expand Up @@ -40,41 +41,33 @@ namespace Azure { namespace Storage { namespace _internal {
explicit XmlReader(const char* data, size_t length);
XmlReader(const XmlReader& other) = delete;
XmlReader& operator=(const XmlReader& other) = delete;
XmlReader(XmlReader&& other) noexcept { *this = std::move(other); }
XmlReader& operator=(XmlReader&& other) noexcept
{
m_context = other.m_context;
other.m_context = nullptr;
return *this;
}
XmlReader(XmlReader&& other) noexcept;
XmlReader& operator=(XmlReader&& other) noexcept;
~XmlReader();

XmlNode Read();

private:
void* m_context = nullptr;
struct XmlReaderContext;
std::unique_ptr<XmlReaderContext> m_context;
};

class XmlWriter final {
public:
explicit XmlWriter();
XmlWriter(const XmlWriter& other) = delete;
XmlWriter& operator=(const XmlWriter& other) = delete;
XmlWriter(XmlWriter&& other) noexcept { *this = std::move(other); }
XmlWriter& operator=(XmlWriter&& other) noexcept
{
m_context = other.m_context;
other.m_context = nullptr;
return *this;
}
XmlWriter(XmlWriter&& other) noexcept;
XmlWriter& operator=(XmlWriter&& other) noexcept;
~XmlWriter();

void Write(XmlNode node);

std::string GetDocument();

private:
void* m_context = nullptr;
struct XmlWriterContext;
std::unique_ptr<XmlWriterContext> m_context;
};

}}} // namespace Azure::Storage::_internal
138 changes: 68 additions & 70 deletions sdk/storage/azure-storage-common/src/xml_wrapper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <cstring>
#include <limits>
#include <memory>
#include <mutex>
#include <stdexcept>

#if defined(AZ_PLATFORM_WINDOWS)
Expand All @@ -27,7 +28,10 @@ namespace Azure { namespace Storage { namespace _internal {

#if defined(AZ_PLATFORM_WINDOWS)

struct XmlReaderContext
void XmlGlobalInitialize() {}
void XmlGlobalDeinitialize() {}

struct XmlReader::XmlReaderContext
{
XmlReaderContext()
{
Expand Down Expand Up @@ -95,20 +99,14 @@ namespace Azure { namespace Storage { namespace _internal {
throw std::runtime_error("Unsupported xml encoding.");
}

m_context = context.release();
m_context = std::move(context);
}

XmlReader::~XmlReader()
{
if (m_context)
{
delete static_cast<XmlReaderContext*>(m_context);
}
}
XmlReader::~XmlReader() {}

XmlNode XmlReader::Read()
{
auto context = static_cast<XmlReaderContext*>(m_context);
auto& context = m_context;

auto moveToNext = [&]() {
HRESULT ret = WsReadNode(context->reader, context->error);
Expand Down Expand Up @@ -218,7 +216,7 @@ namespace Azure { namespace Storage { namespace _internal {
}
}

struct XmlWriterContext
struct XmlWriter::XmlWriterContext
{
XmlWriterContext()
{
Expand Down Expand Up @@ -272,20 +270,14 @@ namespace Azure { namespace Storage { namespace _internal {
throw std::runtime_error("Failed to initialize xml writer.");
}

m_context = context.release();
m_context = std::move(context);
}

XmlWriter::~XmlWriter()
{
if (m_context)
{
delete static_cast<XmlWriterContext*>(m_context);
}
}
XmlWriter::~XmlWriter() {}

void XmlWriter::Write(XmlNode node)
{
auto context = static_cast<XmlWriterContext*>(m_context);
auto& context = m_context;
if (node.Type == XmlNodeType::StartTag)
{
if (node.HasValue)
Expand Down Expand Up @@ -359,7 +351,7 @@ namespace Azure { namespace Storage { namespace _internal {

std::string XmlWriter::GetDocument()
{
auto context = static_cast<XmlWriterContext*>(m_context);
auto& context = m_context;

BOOL boolValueTrue = TRUE;
WS_XML_WRITER_PROPERTY writerProperty[2];
Expand Down Expand Up @@ -400,11 +392,15 @@ namespace Azure { namespace Storage { namespace _internal {

static void XmlGlobalInitialize() { static XmlGlobalInitializer globalInitializer; }

struct XmlReaderContext
struct XmlReader::XmlReaderContext
{
xmlTextReaderPtr reader = nullptr;
using XmlTextReaderPtr = std::unique_ptr<xmlTextReader, decltype(&xmlFreeTextReader)>;

XmlTextReaderPtr reader;
bool readingAttributes = false;
bool readingEmptyTag = false;

explicit XmlReaderContext(XmlTextReaderPtr&& reader_) : reader(std::move(reader_)) {}
};

XmlReader::XmlReader(const char* data, size_t length)
Expand All @@ -416,38 +412,38 @@ namespace Azure { namespace Storage { namespace _internal {
throw std::runtime_error("Xml data too big.");
}

xmlTextReaderPtr reader
= xmlReaderForMemory(data, static_cast<int>(length), nullptr, nullptr, 0);
auto reader = XmlReaderContext::XmlTextReaderPtr(
xmlReaderForMemory(data, static_cast<int>(length), nullptr, nullptr, 0), xmlFreeTextReader);

if (!reader)
{
throw std::runtime_error("Failed to parse xml.");
}

XmlReaderContext* context = new XmlReaderContext();
context->reader = reader;
m_context = context;
m_context = std::make_unique<XmlReaderContext>(std::move(reader));
}

XmlReader::~XmlReader()
XmlReader::XmlReader(XmlReader&& other) noexcept { *this = std::move(other); }

XmlReader& XmlReader::operator=(XmlReader&& other) noexcept
{
if (m_context)
{
auto context = static_cast<XmlReaderContext*>(m_context);
xmlFreeTextReader(static_cast<xmlTextReaderPtr>(context->reader));
delete context;
}
m_context = std::move(other.m_context);
return *this;
}

XmlReader::~XmlReader() = default;

XmlNode XmlReader::Read()
{
auto context = static_cast<XmlReaderContext*>(m_context);
XmlReaderContext* context = m_context.get();
xmlTextReader* reader = m_context->reader.get();
if (context->readingAttributes)
{
int ret = xmlTextReaderMoveToNextAttribute(context->reader);
int ret = xmlTextReaderMoveToNextAttribute(reader);
if (ret == 1)
{
const char* name = reinterpret_cast<const char*>(xmlTextReaderConstName(context->reader));
const char* value = reinterpret_cast<const char*>(xmlTextReaderConstValue(context->reader));
const char* name = reinterpret_cast<const char*>(xmlTextReaderConstName(reader));
const char* value = reinterpret_cast<const char*>(xmlTextReaderConstValue(reader));
return XmlNode{XmlNodeType::Attribute, name, value};
}
else if (ret == 0)
Expand All @@ -465,7 +461,7 @@ namespace Azure { namespace Storage { namespace _internal {
return XmlNode{XmlNodeType::EndTag};
}

int ret = xmlTextReaderRead(context->reader);
int ret = xmlTextReaderRead(reader);
if (ret == 0)
{
return XmlNode{XmlNodeType::End};
Expand All @@ -475,13 +471,13 @@ namespace Azure { namespace Storage { namespace _internal {
throw std::runtime_error("Failed to parse xml.");
}

int type = xmlTextReaderNodeType(context->reader);
bool is_empty = xmlTextReaderIsEmptyElement(context->reader) == 1;
bool has_value = xmlTextReaderHasValue(context->reader) == 1;
bool has_attributes = xmlTextReaderHasAttributes(context->reader) == 1;
int type = xmlTextReaderNodeType(reader);
bool is_empty = xmlTextReaderIsEmptyElement(reader) == 1;
bool has_value = xmlTextReaderHasValue(reader) == 1;
bool has_attributes = xmlTextReaderHasAttributes(reader) == 1;

const char* name = reinterpret_cast<const char*>(xmlTextReaderConstName(context->reader));
const char* value = reinterpret_cast<const char*>(xmlTextReaderConstValue(context->reader));
const char* name = reinterpret_cast<const char*>(xmlTextReaderConstName(reader));
const char* value = reinterpret_cast<const char*>(xmlTextReaderConstValue(reader));

if (has_attributes)
{
Expand Down Expand Up @@ -520,49 +516,53 @@ namespace Azure { namespace Storage { namespace _internal {
return Read();
}

struct XmlWriterContext
struct XmlWriter::XmlWriterContext
{
xmlBufferPtr buffer;
xmlTextWriterPtr writer;
using XmlBufferPtr = std::unique_ptr<xmlBuffer, decltype(&xmlBufferFree)>;
using XmlTextWriterPtr = std::unique_ptr<xmlTextWriter, decltype(&xmlFreeTextWriter)>;

XmlBufferPtr buffer;
XmlTextWriterPtr writer;

explicit XmlWriterContext(XmlBufferPtr&& buffer_, XmlTextWriterPtr&& writer_)
: buffer(std::move(buffer_)), writer(std::move(writer_))
{
}
};

XmlWriter::XmlWriter()
{
XmlGlobalInitialize();
auto buffer = xmlBufferCreate();
auto buffer = XmlWriterContext::XmlBufferPtr(xmlBufferCreate(), xmlBufferFree);

if (!buffer)
{
throw std::runtime_error("Failed to initialize xml writer.");
}

auto writer = xmlNewTextWriterMemory(static_cast<xmlBufferPtr>(buffer), 0);
auto writer = XmlWriterContext::XmlTextWriterPtr(
xmlNewTextWriterMemory(buffer.get(), 0), xmlFreeTextWriter);

if (!writer)
{
xmlBufferFree(static_cast<xmlBufferPtr>(buffer));
throw std::runtime_error("Failed to initialize xml writer.");
}

xmlTextWriterStartDocument(static_cast<xmlTextWriterPtr>(writer), nullptr, nullptr, nullptr);
xmlTextWriterStartDocument(writer.get(), nullptr, nullptr, nullptr);

auto context = new XmlWriterContext;
context->buffer = buffer;
context->writer = writer;
m_context = context;
m_context = std::make_unique<XmlWriterContext>(std::move(buffer), std::move(writer));
}

XmlWriter::~XmlWriter()
XmlWriter::XmlWriter(XmlWriter&& other) noexcept { *this = std::move(other); }

XmlWriter& XmlWriter::operator=(XmlWriter&& other) noexcept
{
if (m_context)
{
auto context = static_cast<XmlWriterContext*>(m_context);
xmlFreeTextWriter(static_cast<xmlTextWriterPtr>(context->writer));
xmlBufferFree(static_cast<xmlBufferPtr>(context->buffer));
delete context;
}
m_context = std::move(other.m_context);
return *this;
}

XmlWriter::~XmlWriter() = default;

namespace {
inline xmlChar* BadCast(const char* x)
{
Expand All @@ -572,8 +572,7 @@ namespace Azure { namespace Storage { namespace _internal {

void XmlWriter::Write(XmlNode node)
{
auto context = static_cast<XmlWriterContext*>(m_context);
xmlTextWriterPtr writer = context->writer;
xmlTextWriter* writer = m_context->writer.get();
if (node.Type == XmlNodeType::StartTag)
{
if (!node.HasValue)
Expand Down Expand Up @@ -611,9 +610,8 @@ namespace Azure { namespace Storage { namespace _internal {

std::string XmlWriter::GetDocument()
{
auto context = static_cast<XmlWriterContext*>(m_context);
xmlBufferPtr buffer = context->buffer;
return std::string(reinterpret_cast<const char*>(buffer->content), buffer->use);
return std::string(
reinterpret_cast<const char*>(m_context->buffer->content), m_context->buffer->use);
}

#endif
Expand Down