RDKB-64747: CcspPandM component is crashing on Device.Time.Status subscription - #126
RDKB-64747: CcspPandM component is crashing on Device.Time.Status subscription#126JESTINJM wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a crash path in the RBUS value-change subscription/polling logic when a subscribed DM parameter returns a NULL parameterValue (e.g., Device.Time.Status derived from ntp_status).
Changes:
- Added NULL checks before calling
strdup(val[0]->parameterValue)when caching the initial value on subscribe. - Added NULL checks before calling
strdup(val[0]->parameterValue)when updating the cached value in the polling thread.
Comments suppressed due to low confidence (1)
source/util_api/ccsp_msg_bus/ccsp_rbus_value_change.c:431
- If val[0]->parameterValue is NULL during initial subscription, rec->value stays NULL. Downstream code assumes rec->value is a valid C string (e.g., strcmp(rec->value, ...) in the polling thread and rbusMessage_SetString(msg, rec->value) when publishing oldValue), so subscribing to a parameter that returns NULL can still crash even though strdup(NULL) is avoided here.
if(val[0]->parameterValue)
{
rec->value = strdup(val[0]->parameterValue);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
source/util_api/ccsp_msg_bus/ccsp_rbus_value_change.c:338
- Even with the new NULL-safe strcmp, the filter path can still crash: when rec->filter is set, rbusValueChange_getFilterResult calls rbusValue_SetFromString on rec->value and val[0]->parameterValue. This change now allows those strings to be NULL, so a subscription using a filter could still dereference NULL during filter evaluation. Normalizing NULLs to a safe sentinel (e.g., empty string) before calling the filter helper avoids that crash while preserving filter semantics.
if(changed)
{
int filterResult = -1;
bool publish = true;
veeraputhiran-thangavel
left a comment
There was a problem hiding this comment.
The utopia script still initializes ntp_status to 0 on boot, which is the root cause identified in RDKB-64747. Is there a separate PR/ticket planned to fix that initialization to a valid value ?
I don't see any separate ticket/PR to fix that initialization. These changes would be good to have as it would be a generic fix to handle similar scenarios with other DMs. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
source/util_api/ccsp_msg_bus/ccsp_rbus_value_change.c:325
- The new NULL guard in the polling thread
continues without freeingval, leaking the parameterValStruct allocations. It also skips initialization whenrec->valueis NULL, so if the initial subscription captured a NULL value, the record will never initialize when the parameter later becomes non-NULL (and will keep leaking). Freevalbefore continuing, and initializerec->valuefrom the first non-NULL value (and reset it when current becomes NULL).
if((!rec->value) || (!val[0]->parameterValue))
{
CcspTraceDebug (("%s: previous value or current value is NULL\n", __FUNCTION__));
continue;
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
source/util_api/ccsp_msg_bus/ccsp_rbus_value_change.c:326
- The new NULL guard in the polling thread skips processing when
rec->valueis NULL. If a subscription starts with a NULL value (as described in the PR) and later becomes non-NULL,rec->valuewill never be initialized, so this code willcontinueforever and never detect/publish future changes for that parameter.
if((!rec->value) || (!val[0]->parameterValue))
{
CcspTraceDebug (("%s: previous value or current value is NULL\n", __FUNCTION__));
free_parameterValStruct_t(rec->handle, 1, val);
continue;
}
…scription Reason for change: To fix PandM crash observed when subscribing to Device.Time.Status baked with a NULL value that is out of range from syscfg parameter ntp_status Test Procedure: Execute 'syscfg set ntp_status 0' and then subscribe to the corresponding DM with command 'rbuscli sub Device.Time.Status' and confirm CcspPandM process is NOT crashed. Risks: Low Priority: P1 Signed-off-by: jmathe064 <Jestin_Mathew@comcast.com>
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.
Suppressed comments (1)
source/util_api/ccsp_msg_bus/ccsp_rbus_value_change.c:326
- The new NULL guard treats a NULL cached value (rec->value) as an error and
continues. If the initial read inCcsp_RbusValueChange_Subscribereturns NULL (sorec->valuestays NULL), the polling thread will never initializerec->valueeven after the parameter later becomes non-NULL, so value-change detection/publishing will never recover for that subscription.
if((!rec->value) || (!val[0]->parameterValue))
{
CcspTraceError (("%s: previous value or current value is NULL\n", __FUNCTION__));
free_parameterValStruct_t(rec->handle, 1, val);
continue;
RDKB-64747: CcspPandM component is crashing on Device.Time.Status subscription
Reason for change: To fix PandM crash observed when subscribing to Device.Time.Status baked with a NULL value that is out of range from syscfg parameter ntp_status
Test Procedure: Execute 'syscfg set ntp_status 0' and then subscribe to the corresponding DM with command
'rbuscli sub Device.Time.Status' and confirm CcspPandM process is NOT crashed.
Risks: Low
Priority: P1