-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
FIX(server): Handle WinAPI objects lifetime properly in Zeroconf #7133
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,43 +5,31 @@ | |
|
|
||
| #include "Zeroconf.h" | ||
|
|
||
| #define GET_SYMBOL(symbol) (symbol = reinterpret_cast< decltype(symbol) >(GetProcAddress(handle, #symbol))) | ||
| #define GET_SYMBOL(symbol) (symbol = reinterpret_cast< decltype(symbol) >(GetProcAddress(m_module, #symbol))) | ||
|
|
||
| Zeroconf::Zeroconf() : m_ok(false) { | ||
| #ifdef Q_OS_WIN | ||
| auto handle = GetModuleHandle(L"dnsapi.dll"); | ||
| if (handle) { | ||
| GET_SYMBOL(DnsServiceConstructInstance); | ||
| GET_SYMBOL(DnsServiceFreeInstance); | ||
| GET_SYMBOL(DnsServiceRegister); | ||
| GET_SYMBOL(DnsServiceDeRegister); | ||
| GET_SYMBOL(DnsServiceRegisterCancel); | ||
|
|
||
| if (DnsServiceConstructInstance && DnsServiceFreeInstance && DnsServiceRegister && DnsServiceDeRegister | ||
| && DnsServiceRegisterCancel) { | ||
| m_ok = true; | ||
| return; | ||
| } | ||
| if (m_reg.isOk()) { | ||
| m_ok = true; | ||
| return; | ||
| } | ||
|
|
||
| qWarning("Zeroconf: Native mDNS/DNS-SD API not available, falling back to third-party API"); | ||
|
|
||
| handle = LoadLibrary(L"dnssd.dll"); | ||
| if (!handle) { | ||
| HMODULE module = LoadLibrary(L"dnssd.dll"); | ||
| if (!module) { | ||
| qWarning("Zeroconf: Failed to load dnssd.dll, assuming third-party API is not available"); | ||
| return; | ||
| } | ||
| FreeLibrary(handle); | ||
|
|
||
| FreeLibrary(module); | ||
| #endif | ||
| resetHelper(); | ||
|
|
||
| m_ok = true; | ||
| } | ||
|
|
||
| Zeroconf::~Zeroconf() { | ||
| if (!m_helper) { | ||
| unregisterService(); | ||
| } | ||
| } | ||
|
|
||
| void Zeroconf::resetHelper() { | ||
|
|
@@ -50,17 +38,102 @@ void Zeroconf::resetHelper() { | |
| } | ||
|
|
||
| bool Zeroconf::registerService(const BonjourRecord &record, const uint16_t port) { | ||
| if (!m_ok) { | ||
| return false; | ||
| } | ||
|
|
||
| unregisterService(); | ||
|
|
||
| if (m_helper) { | ||
| m_helper->registerService(record, port); | ||
| return true; | ||
| } | ||
| #ifdef Q_OS_WIN | ||
| return m_reg.request(record, port); | ||
| #else | ||
| return false; | ||
| #endif | ||
| } | ||
|
|
||
| bool Zeroconf::unregisterService() { | ||
| if (m_helper) { | ||
| resetHelper(); | ||
| return true; | ||
| } | ||
| #ifdef Q_OS_WIN | ||
| return m_reg.cancel(); | ||
| #else | ||
| return false; | ||
| #endif | ||
| } | ||
|
|
||
| void Zeroconf::helperError(const DNSServiceErrorType error) { | ||
| qWarning("Zeroconf: Third-party API reports error %d, service registration probably failed", error); | ||
| } | ||
|
|
||
| #ifdef Q_OS_WIN | ||
| Zeroconf::Reg::Reg() : m_instance(nullptr, InstanceDeleter{ DnsServiceFreeInstance }) { | ||
| m_module = GetModuleHandle(L"dnsapi.dll"); | ||
| if (!m_module) { | ||
| return; | ||
| } | ||
|
|
||
| GET_SYMBOL(DnsServiceConstructInstance); | ||
| GET_SYMBOL(DnsServiceFreeInstance); | ||
| GET_SYMBOL(DnsServiceRegister); | ||
| GET_SYMBOL(DnsServiceDeRegister); | ||
| GET_SYMBOL(DnsServiceRegisterCancel); | ||
|
|
||
| if (!DnsServiceConstructInstance || !DnsServiceFreeInstance || !DnsServiceRegister || !DnsServiceDeRegister | ||
| || !DnsServiceRegisterCancel) { | ||
| FreeModule(m_module); | ||
| m_module = nullptr; | ||
| } | ||
| } | ||
|
|
||
| Zeroconf::Reg::~Reg() { | ||
| if (!isOk()) { | ||
| return; | ||
| } | ||
|
|
||
| cancel(); | ||
| FreeLibrary(m_module); | ||
| } | ||
|
|
||
| bool Zeroconf::Reg::cancel() { | ||
| if (!isOk()) { | ||
| return false; | ||
| } | ||
|
|
||
| if (m_cancel) { | ||
| const auto ret = DnsServiceRegisterCancel(&m_cancel.value()); | ||
| if (ret != ERROR_SUCCESS && ret != ERROR_CANCELLED) { | ||
| qWarning("Zeroconf: DnsServiceRegisterCancel() failed with error %u!", ret); | ||
|
|
||
| m_cancel.reset(); | ||
| m_instance.reset(); | ||
|
|
||
| return false; | ||
| } | ||
| } else if (m_instance) { | ||
| DNS_SERVICE_REGISTER_REQUEST req{}; | ||
| req.Version = DNS_QUERY_REQUEST_VERSION1; | ||
| req.pServiceInstance = m_instance.get(); | ||
| req.pRegisterCompletionCallback = callback; | ||
| req.pQueryContext = this; | ||
|
|
||
| const auto ret = DnsServiceDeRegister(&req, nullptr); | ||
| if (ret != DNS_REQUEST_PENDING) { | ||
| qWarning("Zeroconf: DnsServiceDeRegister() failed with error %u!", ret); | ||
|
|
||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
Comment on lines
+90
to
+130
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 Result: The DnsServiceDeRegister function (from windns.h in Windows 10+ DNS-SD API) is asynchronous. When called successfully, it immediately returns DNS_REQUEST_PENDING (9506), indicating the deregistration request is pending. The callback specified in the original DNS_SERVICE_REGISTER_REQUEST's pRegisterCompletionCallback field (of type DNS_SERVICE_REGISTER_COMPLETE) is invoked asynchronously later, when deregistration completes. The callback receives the status, user context, and a copy of the original DNS_SERVICE_INSTANCE. There is no separate deregister-specific callback type; it reuses the register completion callback. The function call itself is not synchronous—the callback timing is asynchronous, typically after some network/mDNS propagation delay. Citations:
🏁 Script executed: # First, let's find the callback function definition referenced at line 128
rg -n "callback\s*\(" src/murmur/Zeroconf.cpp -A 10 -B 2Repository: mumble-voip/mumble Length of output: 422 🏁 Script executed: # Look for the complete Reg class definition to understand the callback and member variables
rg -n "class.*Reg" src/murmur/Zeroconf.cpp -A 50Repository: mumble-voip/mumble Length of output: 44 🏁 Script executed: # Find where the callback function is defined (it's likely a static member)
rg -n "DNS_SERVICE_REGISTER_COMPLETE|void.*callback" src/murmur/Zeroconf.cpp -B 5 -A 20Repository: mumble-voip/mumble Length of output: 706 🏁 Script executed: # Check the Server.cpp:652-662 context mentioned in the review
sed -n '650,665p' src/murmur/Server.cppRepository: mumble-voip/mumble Length of output: 250 🏁 Script executed: # Look for any event or synchronization mechanisms in the Zeroconf class
rg -n "event|Event|WaitFor|condition|Condition" src/murmur/Zeroconf.cppRepository: mumble-voip/mumble Length of output: 44 Use-after-free in async deregistration callback. When
Either wait for the callback to complete before destruction (using an event or condition variable), use synchronous deregistration, or ensure callbacks are protected via shared/weak pointers. 🤖 Prompt for AI Agents
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is indeed a problem, smart pointers are the way to go to fix this. |
||
|
|
||
| bool Zeroconf::Reg::request(const BonjourRecord &record, const uint16_t port) { | ||
| if (!isOk()) { | ||
| return false; | ||
| } | ||
|
|
||
| DWORD size = 0; | ||
| GetComputerNameEx(ComputerNameDnsHostname, nullptr, &size); | ||
| std::vector< wchar_t > hostname(size); | ||
|
|
@@ -84,76 +157,47 @@ bool Zeroconf::registerService(const BonjourRecord &record, const uint16_t port) | |
| return false; | ||
| } | ||
|
|
||
| m_request.reset(new DNS_SERVICE_REGISTER_REQUEST{}); | ||
| m_request->Version = DNS_QUERY_REQUEST_VERSION1; | ||
| m_request->pServiceInstance = instance; | ||
| m_request->pRegisterCompletionCallback = callbackRegisterComplete; | ||
| m_request->pQueryContext = this; | ||
| DNS_SERVICE_REGISTER_REQUEST req{}; | ||
| req.Version = DNS_QUERY_REQUEST_VERSION1; | ||
| req.pServiceInstance = instance; | ||
| req.pRegisterCompletionCallback = callback; | ||
| req.pQueryContext = this; | ||
|
|
||
| m_cancel.reset(new DNS_SERVICE_CANCEL{}); | ||
| const auto ret = DnsServiceRegister(m_request.get(), m_cancel.get()); | ||
| DnsServiceFreeInstance(instance); | ||
| m_cancel = DNS_SERVICE_CANCEL{}; | ||
| m_instance.reset(instance); | ||
|
|
||
| if (ret == DNS_REQUEST_PENDING) { | ||
| return true; | ||
| } | ||
| const auto ret = DnsServiceRegister(&req, &m_cancel.value()); | ||
| if (ret != DNS_REQUEST_PENDING) { | ||
| qWarning("Zeroconf: DnsServiceRegister() failed with error %u!", ret); | ||
|
|
||
| qWarning("Zeroconf: DnsServiceRegister() failed with error %u!", ret); | ||
| m_request.reset(); | ||
| m_cancel.reset(); | ||
| #endif | ||
| return false; | ||
| } | ||
| m_instance.reset(); | ||
| m_cancel.reset(); | ||
|
|
||
| bool Zeroconf::unregisterService() { | ||
| if (!m_ok) { | ||
| return false; | ||
| } | ||
|
|
||
| if (m_helper) { | ||
| resetHelper(); | ||
| return true; | ||
| } | ||
| #ifdef Q_OS_WIN | ||
| if (m_cancel) { | ||
| const auto ret = DnsServiceRegisterCancel(m_cancel.get()); | ||
| if (ret == ERROR_SUCCESS || ret == ERROR_CANCELLED) { | ||
| return true; | ||
| } | ||
|
|
||
| m_cancel.reset(); | ||
| qWarning("Zeroconf: DnsServiceRegisterCancel() failed with error %u!", ret); | ||
| } else if (m_request) { | ||
| const auto ret = DnsServiceDeRegister(m_request.get(), nullptr); | ||
| if (ret == DNS_REQUEST_PENDING) { | ||
| return true; | ||
| } | ||
|
|
||
| qWarning("Zeroconf: DnsServiceDeRegister() failed with error %u!", ret); | ||
| } | ||
| #endif | ||
| return false; | ||
| return true; | ||
| } | ||
|
|
||
| void Zeroconf::helperError(const DNSServiceErrorType error) { | ||
| qWarning("Zeroconf: Third-party API reports error %d, service registration probably failed", error); | ||
| } | ||
| #ifdef Q_OS_WIN | ||
| void WINAPI Zeroconf::callbackRegisterComplete(const DWORD status, void *context, DNS_SERVICE_INSTANCE *instance) { | ||
| auto zeroconf = static_cast< Zeroconf * >(context); | ||
| void WINAPI Zeroconf::Reg::callback(const DWORD status, void *context, DNS_SERVICE_INSTANCE *instance) { | ||
| auto ® = *static_cast< Reg * >(context); | ||
|
|
||
| if (instance) { | ||
| zeroconf->DnsServiceFreeInstance(instance); | ||
| } | ||
| reg.m_instance.reset(instance); | ||
|
|
||
| if (status == ERROR_CANCELLED) { | ||
| if (!reg.m_cancel) { | ||
| // No cancel handle, which means this is a de-registration. | ||
| return; | ||
| } | ||
|
|
||
| zeroconf->m_cancel.reset(); | ||
| reg.m_cancel.reset(); | ||
|
|
||
| if (status != ERROR_SUCCESS) { | ||
| qWarning("Zeroconf: DnsServiceRegister() reports status code %u, service registration probably failed", status); | ||
| switch (status) { | ||
| case ERROR_SUCCESS: | ||
| case ERROR_CANCELLED: | ||
| break; | ||
| default: | ||
| qWarning("Zeroconf: DnsServiceRegister() reports status code %u, service registration probably failed", | ||
| status); | ||
| } | ||
|
Comment on lines
+182
to
201
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 Result: In the Windows DnsServiceRegister API, the pInstance parameter passed to the DNS_SERVICE_REGISTER_COMPLETE callback is a new allocated structure describing the registered service. It is not the same pointer as the pServiceInstance provided in the original DNS_SERVICE_REGISTER_REQUEST. The caller must free this callback pInstance using DnsServiceFreeInstance if it is not nullptr. For deregistration, use DnsServiceDeRegister with the original DNS_SERVICE_REGISTER_REQUEST structure (pRequest). The deregistration callback receives a copy of the original DNS_SERVICE_INSTANCE (from registration request), not the callback pInstance. No sources indicate the callback pInstance pointer is reused or relevant for deregistration. The same pointer from input is not returned in the callback; a new one is allocated. Citations:
Improper Windows API memory management for callback pInstance; deregistration errors silently ignored. Two concerns with this callback:
🤖 Prompt for AI Agents
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The smart pointer has a custom deleter defined that takes care of calling
This is intended and matches the previous behavior. |
||
| } | ||
| #endif | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🌐 Web query:
GetModuleHandleEx vs GetModuleHandle reference count behavior Windows💡 Result:
GetModuleHandle retrieves a handle to a loaded module without incrementing its reference count. Do not call FreeLibrary on the returned handle, as this could cause the module to unload prematurely. GetModuleHandleEx retrieves a module handle and increments the reference count unless the GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT flag is specified. When the flag is used, its behavior matches GetModuleHandle (no refcount increment). Without the flag (or with other flags like 0 or GET_MODULE_HANDLE_EX_FLAG_PIN), it increments the refcount, and FreeLibrary must be called to decrement it when done. Key flags for GetModuleHandleEx: - GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT (0x00000002): No refcount change, equivalent to GetModuleHandle. Do not FreeLibrary. - No flag or GET_MODULE_HANDLE_EX_FLAG_PIN (0x00000001): Increments refcount, requires FreeLibrary. - GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS (0x00000004): Treats lpModuleName as address in module; refcount behavior depends on other flags.
Citations:
🏁 Script executed:
cat -n src/murmur/Zeroconf.cpp | head -120Repository: mumble-voip/mumble
Length of output: 3778
🏁 Script executed:
grep -r "FreeModule" src/murmur/Zeroconf.cppRepository: mumble-voip/mumble
Length of output: 85
🏁 Script executed:
grep -r "#define FreeModule" src/Repository: mumble-voip/mumble
Length of output: 44
🏁 Script executed:
grep -r "FreeModule" src/murmur/Zeroconf.hRepository: mumble-voip/mumble
Length of output: 44
🏁 Script executed:
Repository: mumble-voip/mumble
Length of output: 44
🏁 Script executed:
rg "FreeModule" src/murmur/Repository: mumble-voip/mumble
Length of output: 109
🏁 Script executed:
Repository: mumble-voip/mumble
Length of output: 85
🏁 Script executed:
Repository: mumble-voip/mumble
Length of output: 697
🏁 Script executed:
Repository: mumble-voip/mumble
Length of output: 753
🏁 Script executed:
Repository: mumble-voip/mumble
Length of output: 44
🏁 Script executed:
Repository: mumble-voip/mumble
Length of output: 255
🏁 Script executed:
# Check recent git history for this line to understand if FreeModule was intentional git log --oneline -n 20 -- src/murmur/Zeroconf.cppRepository: mumble-voip/mumble
Length of output: 135
🏁 Script executed:
Repository: mumble-voip/mumble
Length of output: 50374
🏁 Script executed:
Repository: mumble-voip/mumble
Length of output: 50377
🏁 Script executed:
git show e7e9ead2 -- src/murmur/Zeroconf.cpp | head -200Repository: mumble-voip/mumble
Length of output: 5752
🏁 Script executed:
Repository: mumble-voip/mumble
Length of output: 109
🏁 Script executed:
Repository: mumble-voip/mumble
Length of output: 762
Incorrect module handle lifetime management:
GetModuleHandlepaired withFreeLibrary.GetModuleHandle()at line 72 retrieves a handle to an already-loaded module without incrementing the reference count. However,FreeLibrary()at line 96 decrements the reference count. This mismatch can cause undefined behavior—potentially unloadingdnsapi.dllprematurely if its reference count was already 1.Additionally, line 85 calls
FreeModule(), which is not a standard Windows API function. This should likely beFreeLibrary()as well.Use
GetModuleHandleExwith default flags (which increments the ref count) or switch toLoadLibrary/FreeLibrarypairing.🔧 Proposed fix using GetModuleHandleEx
Zeroconf::Reg::Reg() : m_instance(nullptr, InstanceDeleter{ DnsServiceFreeInstance }) { - m_module = GetModuleHandle(L"dnsapi.dll"); - if (!m_module) { + if (!GetModuleHandleEx(0, L"dnsapi.dll", &m_module)) { + m_module = nullptr; return; }🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LoadLibrary()/FreeLibrary()is indeed the correct pair to use.We were already using
GetModuleHandle()before but without callingFreeLibrary(), which is guaranteed to work because there is at least one dependency (Qt Network) loadingdnsapi.dll.However, since we're actively using functions that are exported by the library we should indeed increment the reference count to ensure it doesn't get unloaded prematurely.