Skip to content

Commit

Permalink
Resolve memory leak in MDnsProvider and do clean up
Browse files Browse the repository at this point in the history
  • Loading branch information
Rawad Hilal committed Feb 21, 2023
1 parent d705c18 commit b6e7b1e
Show file tree
Hide file tree
Showing 6 changed files with 171 additions and 61 deletions.
4 changes: 2 additions & 2 deletions src/platform/Darwin/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ static_library("Darwin") {
"DnssdImpl.h",
"InetPlatformConfig.h",
"Logging.cpp",
"MDnsProviderCpp.cpp",
"MDnsProviderCpp.h",
"MDnsProvider.cpp",
"MDnsProvider.h",
"MdnsError.cpp",
"MdnsError.h",
"NetworkCommissioningDriver.h",
Expand Down
23 changes: 1 addition & 22 deletions src/platform/Darwin/DnssdContexts.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -278,27 +278,6 @@ void RegisterContext::DispatchSuccess()
mHostNameRegistrar.Register();
}

BrowseContext::BrowseContext(void * cbContext, DnssdBrowseCallback cb, DnssdServiceProtocol cbContextProtocol)
{
type = ContextType::Browse;
context = cbContext;
callback = cb;
protocol = cbContextProtocol;
}

void BrowseContext::DispatchFailure(DNSServiceErrorType err)
{
ChipLogError(Discovery, "Mdns: Browse failure (%s)", Error::ToString(err));
callback(context, nullptr, 0, true, Error::ToChipError(err));
MdnsContexts::GetInstance().Remove(this);
}

void BrowseContext::DispatchSuccess()
{
callback(context, services.data(), services.size(), true, CHIP_NO_ERROR);
MdnsContexts::GetInstance().Remove(this);
}

ResolveContext::ResolveContext(void * cbContext, DnssdResolveCallback cb, chip::Inet::IPAddressType cbAddressType)
{
type = ContextType::Resolve;
Expand Down Expand Up @@ -503,7 +482,7 @@ CHIP_ERROR DnsProviderBrowseContext::Finalize(int err)
DispatchFailure(err);
}

delete(this);
chip::Platform::Delete(this);
}

void DnsProviderBrowseContext::DispatchFailure(int err)
Expand Down
4 changes: 2 additions & 2 deletions src/platform/Darwin/DnssdImpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
*/
#include "DnssdImpl.h"
#include "MdnsError.h"
#include "MDnsProviderCpp.h"
#include "MDnsProvider.h"

#include <cstdio>
#include <sstream>
Expand Down Expand Up @@ -270,7 +270,7 @@ static void OnMDnsProviderBrowse

if ((flags & kMDnsProviderBrowseFlagsBatchComplete))
{
sdCtx->Finalize(false);
sdCtx->Finalize();
}
}

Expand Down
17 changes: 2 additions & 15 deletions src/platform/Darwin/DnssdImpl.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
#include <lib/dnssd/platform/Dnssd.h>

#include "DnssdHostNameRegistrar.h"
#include "MDnsProviderCpp.h"
#include "MDnsProvider.h"

#include <map>
#include <string>
Expand Down Expand Up @@ -107,26 +107,13 @@ struct RegisterContext : public GenericContext
bool matches(const char * sType) { return mType.compare(sType) == 0; }
};

struct BrowseContext : public GenericContext
{
DnssdBrowseCallback callback;
std::vector<DnssdService> services;
DnssdServiceProtocol protocol;

BrowseContext(void * cbContext, DnssdBrowseCallback cb, DnssdServiceProtocol cbContextProtocol);
virtual ~BrowseContext() {}

void DispatchFailure(DNSServiceErrorType err) override;
void DispatchSuccess() override;
};

struct DnsProviderBrowseContext
{
std::vector<DnssdService> services;
DnssdBrowseCallback callback;
uint32_t interfaceId;
DnssdServiceProtocol protocol;
MDnsProvider* provider;
MDnsProviderRef provider;
void* context;

DnsProviderBrowseContext();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,17 +1,27 @@
//
// MDnsProviderCpp.c
// MDNS-SD
//
// Created by Hilal, Rawad on 2/15/23.
//
/*
*
* Copyright (c) 2021 Project CHIP Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#include "MDnsProviderCpp.h"
#include "MDnsProvider.h"
#include <Network/Network.h>
#include <lib/support/CHIPMem.h>

namespace chip {
namespace Dnssd {

typedef struct MDnsProvider
{
nw_browser_t browser;
Expand Down Expand Up @@ -109,7 +119,6 @@ void MDnsProviderStopBrowsing(MDnsProvider* provider)
void _MDnsProviderInitializeBrowser(MDnsProvider* provider)
{
provider->browser = nw_browser_create(provider->browse_descriptor, provider->browse_parameters);
nw_retain(provider->browser);

nw_browser_set_queue(provider->browser, dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0));
nw_browser_set_state_changed_handler(provider->browser, ^(nw_browser_state_t state, nw_error_t _Nullable error) {
Expand Down Expand Up @@ -168,6 +177,7 @@ void _MDnsProviderInitializeBrowser(MDnsProvider* provider)
const char *name = nw_endpoint_get_bonjour_service_name(endpoint);
const char *type = nw_endpoint_get_bonjour_service_type(endpoint);
const char *domain = nw_endpoint_get_bonjour_service_domain(endpoint);
nw_release(endpoint);

__block size_t interface_count = nw_browse_result_get_interfaces_count(result);

Expand All @@ -191,6 +201,6 @@ void _MDnsProviderInitializeBrowser(MDnsProvider* provider)
});
});
}

}

}
}
Original file line number Diff line number Diff line change
@@ -1,12 +1,24 @@
//
// MDnsProviderCpp.h
// MDNS-SD
//
// Created by Hilal, Rawad on 2/15/23.
//
/*
*
* Copyright (c) 2021 Project CHIP Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#ifndef MDnsProvider_h
#define MDnsProvider_h

#ifndef MDnsProviderCpp_hpp
#define MDnsProviderCpp_hpp
#include <lib/support/CHIPMem.h>

#include <stdio.h>
#include <stdint.h>
Expand Down Expand Up @@ -128,6 +140,128 @@ void MDnsProviderStartBrowsing(MDnsProvider* provider);
*/
void MDnsProviderStopBrowsing(MDnsProvider* provider);

/**
* In C++, you can take advantage of the MDnsProviderRef which automatically manages memory and encapsulates the MDnsProvider.
*
* When all references to the object are lost, the class will automatically release the underlying MDnsProvider object and terminate the browsing process.
*/
class MDnsProviderRef
{
private:
const int kInitRefCount = 1;
using CounterType = uint32_t;

static void* operator new(size_t) = delete;
static void* operator new[](size_t) = delete;
static void operator delete(void*) = delete;
static void operator delete[](void*) = delete;

public:

explicit MDnsProviderRef(MDnsProvider* ptr = nullptr)
{
_provider = ptr;
_counter = chip::Platform::New<CounterType>();
*_counter = kInitRefCount;
}

MDnsProviderRef(const char* service_type, const char* domain)
{
_provider = MDnsProviderCreate(service_type, domain);
_counter = chip::Platform::New<CounterType>();
*_counter = kInitRefCount;
}

MDnsProviderRef(const MDnsProviderRef& ref)
{
_provider = ref._provider;
_counter = ref._counter;
if (ref._provider != nullptr)
{
(*_counter)++;
}
}

MDnsProviderRef(MDnsProviderRef&& ref) noexcept
{
_cleanup();

_provider = ref._provider;
_counter = ref._counter;
ref._provider = nullptr;
ref._counter = nullptr;
}

~MDnsProviderRef()
{
_cleanup();
_provider = nullptr;
_counter = nullptr;
}

MDnsProvider* get()
{
return _provider;
}

MDnsProvider& operator*()
{
return *_provider;
}

MDnsProvider* operator->()
{
return _provider;
}

MDnsProviderRef& operator= (const MDnsProviderRef& ref)
{
if (this != &ref)
{
_cleanup();
_provider = ref._provider;
_counter = ref._counter;
(*_counter)++;
}
return *this;
}

MDnsProviderRef& operator= (MDnsProviderRef && ref) noexcept
{
if (this != &ref)
{
_cleanup();

_provider = ref._provider;
_counter = ref._counter;

ref._provider = nullptr;
ref._counter = nullptr;
}
return *this;
}

private:
void _cleanup()
{
if (_counter != nullptr && --(*_counter) == 0)
{
if (_provider != nullptr)
{
MDnsProviderDelete(_provider);
_provider = nullptr;
}
chip::Platform::Delete(_counter);
_counter = nullptr;
}
}

private:
MDnsProvider* _provider = nullptr;
CounterType* _counter = nullptr;
};

}
}
#endif /* MDnsProviderCpp_hpp */

#endif /* MDnsProvider_h */

0 comments on commit b6e7b1e

Please sign in to comment.