From bbf911d522b1cfa9006da3d779b62215591a8713 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Mon, 13 Mar 2023 16:34:14 +0000 Subject: [PATCH] [Tizen] Fix deadlock caused by mutex lock inversion (#25573) 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. --- src/platform/Tizen/DnssdImpl.cpp | 99 ++++++++++++++++++++------------ src/platform/Tizen/DnssdImpl.h | 7 +++ 2 files changed, 69 insertions(+), 37 deletions(-) diff --git a/src/platform/Tizen/DnssdImpl.cpp b/src/platform/Tizen/DnssdImpl.cpp index 12a1da134ba70c..abdfc110831f6b 100644 --- a/src/platform/Tizen/DnssdImpl.cpp +++ b/src/platform/Tizen/DnssdImpl.cpp @@ -302,29 +302,41 @@ void GetTextEntries(unsigned short txtLen, uint8_t * txtRecord, std::vector(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 textEntries; - chip::Dnssd::DnssdService dnssdService = {}; - chip::Inet::IPAddress ipAddr; - CHIP_ERROR err = CHIP_NO_ERROR; + auto rCtx = reinterpret_cast(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(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); @@ -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(port); + rCtx->mResult.mPort = static_cast(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(&txtRecord)); + ret = dnssd_service_get_all_txt_record(service, &rCtx->mResultTxtRecordLen, reinterpret_cast(&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(&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(), GetChipError(ret)); - } - else - { - rCtx->mCallback(rCtx->mCbContext, nullptr, chip::Span(), err); - } + rCtx->MainLoopQuit(); + rCtx->Finalize(ret != DNSSD_ERROR_NONE ? GetChipError(ret) : err); rCtx->mInstance->RemoveContext(rCtx); } @@ -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(), error)); + + std::vector 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(&ipAddr, 1), CHIP_NO_ERROR); +} CHIP_ERROR DnssdTizen::Init(DnssdAsyncReturnCallback initCallback, DnssdAsyncReturnCallback errorCallback, void * context) { diff --git a/src/platform/Tizen/DnssdImpl.h b/src/platform/Tizen/DnssdImpl.h index 5872fa01e58d14..bfa34e7dab242f 100644 --- a/src/platform/Tizen/DnssdImpl.h +++ b/src/platform/Tizen/DnssdImpl.h @@ -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