Skip to content

Fix memory leak in xml_wrapper #4647

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

Closed
kysucix wants to merge 7 commits into from
Closed

Fix memory leak in xml_wrapper #4647

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a1cdb8c
Add xml patch
kysucix May 23, 2023
f9e07c3
Add missing <memory> include
kysucix May 23, 2023
dbc9372
clang-format
kysucix May 23, 2023
42ceb1b
Add windows fixes
kysucix May 23, 2023
7564388
format
kysucix May 23, 2023
f731fb3
Update sdk/storage/azure-storage-common/inc/azure/storage/common/inte…
kysucix May 26, 2023
f9476c2
Update sdk/storage/azure-storage-common/inc/azure/storage/common/inte…
kysucix May 26, 2023
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
45 changes: 19 additions & 26 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 All @@ -19,62 +20,54 @@ namespace Azure { namespace Storage { namespace _internal {

struct XmlNode final
{
explicit XmlNode(XmlNodeType type, std::string name = std::string())
: Type(type), Name(std::move(name))
{
}

explicit XmlNode(XmlNodeType type, std::string name, std::string value)
: Type(type), Name(std::move(name)), Value(std::move(value)), HasValue(true)
explicit XmlNode(
XmlNodeType type,
std::string name = std::string(),
std::string value = std::string())

@LarryOsterman LarryOsterman May 25, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
std::string value = std::string())
std::string value = {}))

Q2:
Why are you using a copy constructor for name and value and then moving them to Name? Why not pass nme and value as const& and then construct Name and Value from the reference?

In other words, something like:

explicit XmlNode(
  XmlNodeType type,
  std::string const& name={},
  std::string const& type={}) : Type(type), Name(name), Value(value) {}

@kysucix kysucix May 26, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed it's better to pass arguments as const&.

I'll fix it thanks!

Bear in mind that I'm porting a fix from https://github.com/ClickHouse/azure-sdk-for-cpp and it's not my code :).

@Jinming-Hu Jinming-Hu May 29, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LarryOsterman About passing by value or reference, if you look at sdk/storage/azure-storage-common/src/xml_wrapper.cpp, at all of calling sites of this constructor, we pass by right values (e.g. XmlNode(type, std::move(name), std::move(value))).

So if the constructor argument is passed by value, the actual string data is first moved from the local variable of calling site, then moved to XmlNode private members, there's no copy here.
If by const reference, we'll need to make two copies.

There are a lot of discussions on stackoverflow regarding this topic, like https://stackoverflow.com/questions/4321305/best-form-for-constructors-pass-by-value-or-reference

: Type(type), Name(std::move(name)), Value(std::move(value))
{
}

XmlNodeType Type;
std::string Name;
std::string Value;
bool HasValue = false;
};

class XmlReader final {
public:
explicit XmlReader(const char* data, size_t length);
XmlReader(const char* data, size_t length);

@LarryOsterman LarryOsterman May 25, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
XmlReader(const char* data, size_t length);
XmlReader(uint8_t const* data, size_t length);

Binary data should be std::uint8_t, not char - char should be reserved for characters.

@Jinming-Hu Jinming-Hu May 29, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XML data is seldom binary. It's usually printable strings. We should use char* instead of `uint8_t* in this case, shouldn't we?

@Jinming-Hu Jinming-Hu May 29, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LarryOsterman

@LarryOsterman LarryOsterman May 30, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is string data, it should be std::string. The incantation of pointer+length is normally used for binary data. If this is intended for signed 8 bit integers, you should add a comment explaining why this cannot be a std::string. Also why 8 bit signed integer is the correct representation of this data.

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();
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;
};

void XmlGlobalInitialize();
void XmlGlobalDeinitialize();

}}} // namespace Azure::Storage::_internal
Loading