RDKEMW-13215: BtrCore_BTGetPairedDeviceInfo crash#48
RDKEMW-13215: BtrCore_BTGetPairedDeviceInfo crash#48DamianoBaroneSky merged 2 commits intordkcentral:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a crash/memory-leak scenario in the BlueZ5 DBus backend when fetching paired device info (BtrCore_BTGetPairedDeviceInfo), primarily by tightening NULL handling and adjusting DBus message lifecycle management.
Changes:
- Added broader NULL checks (including
pstlhBtIfce/pDBusConn) before issuing DBus calls. - Corrected DBus boolean parsing to use
dbus_bool_tand added bounds/empty-path guards when collecting paired device paths. - Hardened DBus object cleanup by unref’ing and NULLing
DBusMessage/DBusPendingCallpointers on more paths.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| if (i >= num) { | ||
| BTRCORELOG_INFO("num is: %d, i is %d\n", num, i); | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| BTRCORELOG_INFO ("lpDBusReply is already NULL\n"); | ||
| } | ||
| } | ||
| else { | ||
| BTRCORELOG_INFO ("lpDBusPendC is already NULL\n"); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| if (i >= num) { | ||
| BTRCORELOG_INFO("2 - num is: %d, i is %d\n", num, i); | ||
| } | ||
|
|
||
|
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| dbus_error_init(&lDBusErr); | ||
| lpDBusReply = btrCore_BTSendMethodCall(pstlhBtIfce->pDBusConn, "/", DBUS_INTERFACE_OBJECT_MANAGER, "GetManagedObjects"); | ||
| if (!lpDBusReply) { | ||
| BTRCORELOG_ERROR ("org.bluez.Manager.ListAdapters returned an error: '%s'\n", lDBusErr.message); | ||
| dbus_error_free(&lDBusErr); | ||
| if (dbus_error_is_set(&lDBusErr)) { | ||
| BTRCORELOG_ERROR ("GetManagedObjects returned an error: '%s'\n", lDBusErr.message); | ||
| dbus_error_free(&lDBusErr); | ||
| } | ||
| else { | ||
| BTRCORELOG_ERROR ("GetManagedObjects returned an error:unknown error\n"); | ||
| } | ||
|
|
||
| return -1; |
There was a problem hiding this comment.
DBusError lDBusErr is initialized/checked here, but btrCore_BTSendMethodCall() (bluez5) does not take/return a DBusError, so lDBusErr will never be set for this call. This makes the dbus_error_is_set(&lDBusErr) branch effectively dead code and the resulting logging potentially misleading. Consider removing the DBusError handling here, or enhancing btrCore_BTSendMethodCall() / its callers to surface an actual error reason when GetManagedObjects fails.
| dbus_error_free(&lDBusErr); | ||
| } | ||
| else { | ||
| BTRCORELOG_ERROR ("GetManagedObjects returned an error:unknown error\n"); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| return -1; | ||
| } | ||
|
|
||
| BTRCORELOG_INFO("Hitting BtrCore_BTGetPairedDeviceInfo\n"); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| BTRCORELOG_INFO("device_prop changed from pointer to bool, %d\n", device_prop); | ||
| if ((adapter_path) && (adapter_path[0] != '\0') && (d < BT_MAX_NUM_DEVICE)) { | ||
| strncpy(&paths[d][0], adapter_path, (strlen(adapter_path) < BT_MAX_DEV_PATH_LEN) ? strlen(adapter_path) : BT_MAX_DEV_PATH_LEN - 1); | ||
| ++d; | ||
| ++d; | ||
| BTRCORELOG_INFO("path is copied for paired devices.\n"); | ||
| } | ||
| else { | ||
| if (!adapter_path) { | ||
| BTRCORELOG_INFO("adapter_path is NULL\n"); | ||
| } | ||
| else if (adapter_path[0] == '\0') { | ||
| BTRCORELOG_INFO("adapter_path string is empty\n"); | ||
| } | ||
| else if (d >= BT_MAX_NUM_DEVICE) { | ||
| BTRCORELOG_WARN("Paired device list full; dropping extra entries\n"); | ||
| } | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
dcce741 to
d4259bd
Compare
d4259bd to
1dcd64b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
1dcd64b to
125a996
Compare
125a996 to
2d798e6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
4e80e57 to
1027b7d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!lpDBusReply) { | ||
| BTRCORELOG_ERROR ("org.bluez.Manager.ListAdapters returned an error: '%s'\n", lDBusErr.message); | ||
| dbus_error_free(&lDBusErr); | ||
| BTRCORELOG_ERROR ("GetManagedObjects returned an error:unknown error\n"); |
There was a problem hiding this comment.
The GetManagedObjects failure log message is misleading/low-signal: btrCore_BTSendMethodCall already logs the specific D-Bus error name when it receives an error reply, but this path always logs "unknown error" (and misses a space after the colon). Consider changing this to a generic "GetManagedObjects call failed" (or include the actual error name if available) to avoid confusing operators/debugging.
| BTRCORELOG_ERROR ("GetManagedObjects returned an error:unknown error\n"); | |
| BTRCORELOG_ERROR ("GetManagedObjects call failed\n"); |
| return -1; | ||
| } | ||
|
|
||
| dbus_message_append_args(lpDBusMsg, DBUS_TYPE_STRING, &pdeviceInterface, DBUS_TYPE_INVALID); |
There was a problem hiding this comment.
dbus_message_append_args returns a boolean indicating OOM/append failure, but its return value is ignored. If it fails, the method call is sent without required arguments and can lead to hard-to-debug failures. Capture the return value and, on failure, unref lpDBusMsg and return an error (similar to the send_with_reply failure path).
| dbus_message_append_args(lpDBusMsg, DBUS_TYPE_STRING, &pdeviceInterface, DBUS_TYPE_INVALID); | |
| if (!dbus_message_append_args(lpDBusMsg, DBUS_TYPE_STRING, &pdeviceInterface, DBUS_TYPE_INVALID)) { | |
| BTRCORELOG_ERROR ("Failed to append arguments to message ...\n"); | |
| dbus_message_unref(lpDBusMsg); | |
| lpDBusMsg = NULL; | |
| return -1; | |
| } |
Reason for change: Crash and memleak fix Test Procedure: runbook test cases for bluetooth devices Risks: Low Priority: P1 Signed-off-by: ppalan289 <preethi_palanisamy@comcast.com>
1027b7d to
f0e6403
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!dbus_connection_send_with_reply(pstlhBtIfce->pDBusConn, lpDBusMsg, &lpDBusPendC, -1)) { | ||
| BTRCORELOG_ERROR ("failed to send message"); | ||
| dbus_message_unref(lpDBusMsg); | ||
| lpDBusMsg = NULL; | ||
| return -1; | ||
| } | ||
|
|
||
| dbus_connection_flush(pstlhBtIfce->pDBusConn); |
There was a problem hiding this comment.
This loop dereferences pstlhBtIfce->pDBusConn without re-checking it. In earlier discussion it was noted the DBus connection may be changed externally; if pDBusConn can become NULL between the entry check and this send, this can reintroduce a crash. Consider re-validating pstlhBtIfce/pDBusConn immediately before use (or holding a stable reference under a lock) and failing gracefully if it’s gone.
| if (!dbus_connection_send_with_reply(pstlhBtIfce->pDBusConn, lpDBusMsg, &lpDBusPendC, -1)) { | |
| BTRCORELOG_ERROR ("failed to send message"); | |
| dbus_message_unref(lpDBusMsg); | |
| lpDBusMsg = NULL; | |
| return -1; | |
| } | |
| dbus_connection_flush(pstlhBtIfce->pDBusConn); | |
| DBusConnection* pDBusConn = (pstlhBtIfce != NULL) ? pstlhBtIfce->pDBusConn : NULL; | |
| if (pDBusConn == NULL) { | |
| BTRCORELOG_ERROR("DBus connection is not available\n"); | |
| dbus_message_unref(lpDBusMsg); | |
| lpDBusMsg = NULL; | |
| return -1; | |
| } | |
| if (!dbus_connection_send_with_reply(pDBusConn, lpDBusMsg, &lpDBusPendC, -1)) { | |
| BTRCORELOG_ERROR ("failed to send message"); | |
| dbus_message_unref(lpDBusMsg); | |
| lpDBusMsg = NULL; | |
| return -1; | |
| } | |
| dbus_connection_flush(pDBusConn); |
| lpDBusReply = btrCore_BTSendMethodCall(pstlhBtIfce->pDBusConn, "/", DBUS_INTERFACE_OBJECT_MANAGER, "GetManagedObjects"); | ||
| if (!lpDBusReply) { | ||
| BTRCORELOG_ERROR ("org.bluez.Manager.ListAdapters returned an error: '%s'\n", lDBusErr.message); | ||
| dbus_error_free(&lDBusErr); | ||
| BTRCORELOG_ERROR ("GetManagedObjects call failed\n"); | ||
| return -1; |
There was a problem hiding this comment.
btrCore_BTSendMethodCall() can leak the allocated DBusMessage when dbus_connection_send_with_reply() fails (it returns NULL without unref’ing the message). Since this function is used here for GetManagedObjects, the leak still exists on the failure path; consider fixing btrCore_BTSendMethodCall() to always unref lpDBusMsg before returning, even on send failure.
Reason for change: Crash and memleak fix - latest
Test Procedure: runbook test cases for bluetooth devices
Risks: Low
Priority: P1