Skip to content
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.

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

Backport bug fixes into the msix-sdk repo #420

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jyvenugo
Copy link
Contributor

The two bug fixes made by Phil being backported into this repo by this PR are,
-fix memory leak external/msix-sdk/src/msix/PAL/Crypto/Win32/Crypto.cpp
-fix regression in inflatestream.cpp that results in unnecessary memory pressure.

@@ -45,7 +45,7 @@ namespace MSIX {
{
ThrowErrorIfNot(Error::InflateRead,(self->m_compressionObject->GetAvailableSourceSize() == 0), "uninflated bytes overwritten");
ULONG available = 0;
self->m_compressedBuffer = std::make_unique<std::vector<std::uint8_t>>(BufferSize);
self->m_compressedBuffer.swap(std::make_unique<std::vector<std::uint8_t>>(BufferSize));
Copy link
Member

Choose a reason for hiding this comment

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

It is unclear what value this change is providing, unless there are bugs in the STL/compiler being used.

@@ -142,6 +142,8 @@ namespace MSIX {

InflateStream::~InflateStream()
{
m_compressedBuffer = nullptr;
Copy link
Member

Choose a reason for hiding this comment

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

It seems odd that there is memory pressure from having this allocated while Cleanup is running.

struct unique_hash_handle_deleter {
void operator()(BCRYPT_HASH_HANDLE h) const {
BCryptDestroyHash(h);
#include <deque>
Copy link
Member

Choose a reason for hiding this comment

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

I don't agree with the change here; either our usage of unique_ptr is flawed and should be fixed, or this is unnecessary. Adding this additional overhead isn't the right answer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants