RDKEMW-17624: Fixed potential causes of plugin crash (#303)#312
Open
gururaajar wants to merge 3 commits into
Open
RDKEMW-17624: Fixed potential causes of plugin crash (#303)#312gururaajar wants to merge 3 commits into
gururaajar wants to merge 3 commits into
Conversation
Reason for Change: Fixed potential causes of plugin crash Test Procedure: Monitor for networkmanager plugin crash Priority: P1 Risks: Medium Signed-off-by: Gururaaja ESR<Gururaja_ErodeSriranganRamlingham@comcast.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to harden the NetworkManager plugin against potential crashes by guarding against null iterator/pointer returns from Core::Service<...>::Create, isolating the libnm NMClient/GMainContext from the global default context (with a new platform_deinit() to clean them up), serializing access to m_defaultInterface via getter/setter under a mutex, and tightening cleanup/error handling in the Gnome WPS flow.
Changes:
- Replaced direct
m_defaultInterfacereads/writes with thread-safegetDefaultInterface()/setDefaultInterface()accessors and made the NMClient/GMainContext instance-scoped. - Added null checks after
Core::Service<...>::Create<>calls in JSON-RPC, legacy, and implementation paths to fail gracefully rather than dereferencing a null iterator. - Added a
platform_deinit()lifecycle hook, isolated GMainContext draining before libnm reads, and added missing context/error cleanup in the WPS process loop; also skip link-local IPv4 addresses inGetIPSettings.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| plugin/NetworkManagerImplementation.h | Adds mutex-guarded default-interface accessors, NMClient/GMainContext members, and platform_deinit() declaration |
| plugin/NetworkManagerImplementation.cpp | Routes m_defaultInterface access through accessors; calls platform_deinit(); null-checks new iterators |
| plugin/NetworkManagerConnectivity.cpp | Switches to getDefaultInterface() and snapshots it locally; refactors connectivity-check loop accordingly |
| plugin/NetworkManagerJsonRpc.cpp | Null-checks StringIterator results from Create and returns early via returnJson |
| plugin/rdk/NetworkManagerRDKProxy.cpp | Adds RDK platform_deinit() stub, accessor migration, and iterator null checks |
| plugin/gnome/NetworkManagerGnomeProxy.cpp | Isolates NMClient on a dedicated GMainContext, drains it before reads, skips IPv4 link-local addresses, and adds deinit |
| plugin/gnome/NetworkManagerGnomeWIFI.cpp | Adds missing GMainContext cleanup and scan-error handling in WPS process; fixes a log typo |
| plugin/gnome/gdbus/NetworkManagerGdbusProxy.cpp | Adds gdbus platform_deinit() stub and migrates to accessors / null-checks |
| legacy/LegacyNetworkAPIs.cpp | Adds null check for endpointsIter |
| legacy/LegacyWiFiManagerAPIs.cpp | Adds null check for ssids iterator |
| tests/l2Test/libnm/l2_test_libnmproxy.cpp | Removes one now-extra WillOnce to match the new single-call path in GetIPSettings |
Comments suppressed due to low confidence (1)
plugin/gnome/NetworkManagerGnomeWIFI.cpp:1660
- Same issue as the
wifidevice == NULLbranch above:client(created at line 1594) is not unreferenced beforebreak, leaking the NMClient. Please addg_object_unref(client); client = NULL;here as well.
g_main_context_pop_thread_default(wpsContext);
g_main_context_release(wpsContext);
g_main_context_unref(wpsContext);
wpsContext = NULL;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| g_main_context_pop_thread_default(wpsContext); | ||
| g_main_context_release(wpsContext); | ||
| g_main_context_unref(wpsContext); | ||
| wpsContext = NULL; |
Comment on lines
+318
to
+322
| if (m_nmContext) { | ||
| for (int i = 0; i < 100 && g_main_context_iteration(m_nmContext, FALSE); ++i){ | ||
| // Intentional empty body: just flushing the event queue | ||
| } | ||
| } |
Comment on lines
+329
to
+343
| string getDefaultInterface() const | ||
| { | ||
| std::lock_guard<std::mutex> lock(m_defaultInterfaceMutex); | ||
| return m_defaultInterface; | ||
| } | ||
|
|
||
| void setDefaultInterface(const string& iface) | ||
| { | ||
| std::lock_guard<std::mutex> lock(m_defaultInterfaceMutex); | ||
| m_defaultInterface = iface; | ||
| } | ||
|
|
||
| private: | ||
| string m_defaultInterface; | ||
| mutable std::mutex m_defaultInterfaceMutex; |
Comment on lines
+652
to
+656
| bool triggerConnectivityCheck; | ||
| if(m_ethConnected.load()) | ||
| m_defaultInterface = "eth0"; // If Ethernet is connected, make it the default interface | ||
|
|
||
| if(m_defaultInterface == interface) | ||
| setDefaultInterface("eth0"); // If Ethernet is connected, make it the default interface | ||
| triggerConnectivityCheck = (getDefaultInterface() == interface); | ||
| if(triggerConnectivityCheck) |
Reason for Change: Fixed potential causes of plugin crash Test Procedure: Monitor for networkmanager plugin crash Priority: P1 Risks: Medium Signed-off-by: Gururaaja ESR<Gururaja_ErodeSriranganRamlingham@comcast.com>
…ager into topic/RDKEMW-16152
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reason for Change: Fixed potential causes of plugin crash
Test Procedure: Monitor for networkmanager plugin crash
Priority: P1
Risks: Medium