Skip to content

Commit

Permalink
[Tizen] Fix deadlock caused by mutex lock inversion (#25573)
Browse files Browse the repository at this point in the history
Tizen mDNS API uses callbacks for notifying caller about particular
events, like resolving service. Unfortunately, Tizen API internally
uses global recursive mutex for every API call, which is not unlocked
before calling callbacks...

The problem arises when the caller uses its own mutex which needs to be
locked inside a callback function passed to mDNS API. We might have a
case when:

- in thread 1, we are running callback function, which holds mDNS API
	internal lock, and in that callback we need to lock our lock
- in thread 2, mDNS API is called under callers lock, which internally
	tries to lock its internal lock

As a workaround mDNS callback function will not lock matter stack mutex,
but instead it will only gather all data and schedule further data
processing to a glib idle source callback.
  • Loading branch information
arkq authored and pull[bot] committed Dec 11, 2023
1 parent 0fa5104 commit 1089287
Show file tree
Hide file tree
Showing 2 changed files with 69 additions and 37 deletions.
99 changes: 62 additions & 37 deletions src/platform/Tizen/DnssdImpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -302,29 +302,41 @@ void GetTextEntries(unsigned short txtLen, uint8_t * txtRecord, std::vector<chip
}
}

void OnResolve(dnssd_error_e result, dnssd_service_h service, void * data)
gboolean OnResolveFinalize(gpointer userData)
{
ChipLogDetail(DeviceLayer, "DNSsd %s", __func__);
auto rCtx = reinterpret_cast<chip::Dnssd::ResolveContext *>(data);

char * name = nullptr;
char * ipv4 = nullptr;
char * ipv6 = nullptr;
int port = 0;
char * interface = nullptr;
unsigned short txtLen = 0;
uint8_t * txtRecord = nullptr;
std::vector<chip::Dnssd::TextEntry> textEntries;
chip::Dnssd::DnssdService dnssdService = {};
chip::Inet::IPAddress ipAddr;
CHIP_ERROR err = CHIP_NO_ERROR;
auto rCtx = reinterpret_cast<chip::Dnssd::ResolveContext *>(userData);

rCtx->MainLoopQuit();

{
// Lock the stack mutex when calling the callback function, so that the callback
// function could safely perform message exchange (e.g. PASE session pairing).
chip::DeviceLayer::StackLock lock;
rCtx->Finalize(CHIP_NO_ERROR);
}

rCtx->mInstance->RemoveContext(rCtx);
return G_SOURCE_REMOVE;
}

void OnResolve(dnssd_error_e result, dnssd_service_h service, void * userData)
{
ChipLogDetail(DeviceLayer, "DNSsd %s", __func__);
auto rCtx = reinterpret_cast<chip::Dnssd::ResolveContext *>(userData);

char * name = nullptr;
char * ipv4 = nullptr;
char * ipv6 = nullptr;
int port = 0;
char * interface = nullptr;
chip::Inet::IPAddress ipAddr;
CHIP_ERROR err = CHIP_NO_ERROR;

int ret = dnssd_service_get_name(service, &name);
VerifyOrExit(ret == DNSSD_ERROR_NONE, ChipLogError(DeviceLayer, "dnssd_service_get_name() failed. ret: %d", ret));

chip::Platform::CopyString(dnssdService.mName, name);
chip::Platform::CopyString(rCtx->mResult.mName, name);
g_free(name);

ret = dnssd_service_get_ip(service, &ipv4, &ipv6);
Expand Down Expand Up @@ -359,43 +371,38 @@ void OnResolve(dnssd_error_e result, dnssd_service_h service, void * data)
ret = dnssd_service_get_port(service, &port);
VerifyOrExit(ret == DNSSD_ERROR_NONE, ChipLogError(DeviceLayer, "dnssd_service_get_port() failed. ret: %d", ret));

dnssdService.mPort = static_cast<uint16_t>(port);
rCtx->mResult.mPort = static_cast<uint16_t>(port);

ret = dnssd_service_get_interface(service, &interface);
VerifyOrExit(ret == DNSSD_ERROR_NONE, ChipLogError(DeviceLayer, "dnssd_service_get_interface() failed. ret: %d", ret));

err = chip::Inet::InterfaceId::InterfaceNameToId(interface, dnssdService.mInterface);
err = chip::Inet::InterfaceId::InterfaceNameToId(interface, rCtx->mResult.mInterface);
VerifyOrExit(
err == CHIP_NO_ERROR,
ChipLogError(DeviceLayer, "chip::Inet::InterfaceId::InterfaceNameToId() failed. ret: %" CHIP_ERROR_FORMAT, err.Format()));

ret = dnssd_service_get_all_txt_record(service, &txtLen, reinterpret_cast<void **>(&txtRecord));
ret = dnssd_service_get_all_txt_record(service, &rCtx->mResultTxtRecordLen, reinterpret_cast<void **>(&rCtx->mResultTxtRecord));
VerifyOrExit(ret == DNSSD_ERROR_NONE, ChipLogError(DeviceLayer, "dnssd_service_get_all_txt_record() failed. ret: %d", ret));

GetTextEntries(txtLen, txtRecord, textEntries);
dnssdService.mTextEntries = textEntries.empty() ? nullptr : textEntries.data();
dnssdService.mTextEntrySize = textEntries.size();
rCtx->mResult.mAddress.SetValue(ipAddr);

{ // Lock the stack mutex when calling the callback function, so that the callback
// function could safely perform message exchange (e.g. PASE session pairing).
chip::DeviceLayer::StackLock lock;
rCtx->mCallback(rCtx->mCbContext, &dnssdService, chip::Span<chip::Inet::IPAddress>(&ipAddr, 1), CHIP_NO_ERROR);
{
// Before calling the Resolve() callback, we need to lock stack mutex.
// However, we cannot lock the stack mutex from here, because we might
// face lock inversion problem. This callback (OnResolve()) is called
// with the NSD internal mutex locked, which is also locked by the
// dnssd_create_remote_service() function called in the Resolve(), and
// the Resolve() itself is called with the stack mutex locked.
auto * sourceIdle = g_idle_source_new();
g_source_set_callback(sourceIdle, OnResolveFinalize, rCtx, NULL);
g_source_attach(sourceIdle, g_main_loop_get_context(rCtx->mMainLoop));
}

g_free(txtRecord);

rCtx->mInstance->RemoveContext(rCtx);
return;

exit:
if (err == CHIP_NO_ERROR)
{
rCtx->mCallback(rCtx->mCbContext, nullptr, chip::Span<chip::Inet::IPAddress>(), GetChipError(ret));
}
else
{
rCtx->mCallback(rCtx->mCbContext, nullptr, chip::Span<chip::Inet::IPAddress>(), err);
}
rCtx->MainLoopQuit();
rCtx->Finalize(ret != DNSSD_ERROR_NONE ? GetChipError(ret) : err);
rCtx->mInstance->RemoveContext(rCtx);
}

Expand Down Expand Up @@ -483,7 +490,25 @@ ResolveContext::ResolveContext(DnssdTizen * instance, const char * name, const c
mCbContext = context;
}

ResolveContext::~ResolveContext() {}
ResolveContext::~ResolveContext()
{
g_free(mResultTxtRecord);
}

void ResolveContext::Finalize(CHIP_ERROR error)
{
// In case of error, run the callback function with nullptr as the result.
VerifyOrReturn(error == CHIP_NO_ERROR, mCallback(mCbContext, nullptr, chip::Span<chip::Inet::IPAddress>(), error));

std::vector<chip::Dnssd::TextEntry> textEntries;
GetTextEntries(mResultTxtRecordLen, mResultTxtRecord, textEntries);
mResult.mTextEntries = textEntries.empty() ? nullptr : textEntries.data();
mResult.mTextEntrySize = textEntries.size();

chip::Inet::IPAddress ipAddr = mResult.mAddress.Value();

mCallback(mCbContext, &mResult, chip::Span<chip::Inet::IPAddress>(&ipAddr, 1), CHIP_NO_ERROR);
}

CHIP_ERROR DnssdTizen::Init(DnssdAsyncReturnCallback initCallback, DnssdAsyncReturnCallback errorCallback, void * context)
{
Expand Down
7 changes: 7 additions & 0 deletions src/platform/Tizen/DnssdImpl.h
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,16 @@ struct ResolveContext : public GenericContext
dnssd_service_h mServiceHandle = 0;
bool mIsResolving = false;

// Resolved service
DnssdService mResult = {};
uint8_t * mResultTxtRecord = nullptr;
unsigned short mResultTxtRecordLen = 0;

ResolveContext(DnssdTizen * instance, const char * name, const char * type, uint32_t interfaceId, DnssdResolveCallback callback,
void * context);
~ResolveContext() override;

void Finalize(CHIP_ERROR error);
};

class DnssdTizen
Expand Down

0 comments on commit 1089287

Please sign in to comment.