RDKB-62330 : Selfheal to recover devices when subdoc version mismatch #89
Conversation
ksures101
force-pushed
the
feature/RDKB-62330
branch
from
07cdea7 to
f32b277
Compare
ksures101
force-pushed
the
feature/RDKB-62330
branch
from
f32b277 to
171c85b
Compare
ksures101
force-pushed
the
feature/RDKB-62330
branch
13 times, most recently
from
2cdc310 to
acef2c2
Compare
ksures101
force-pushed
the
feature/RDKB-62330
branch
2 times, most recently
from
88d02a4 to
320e2d5
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a selfheal mechanism to detect and recover from subdoc version mismatches between the webconfig client and components. The implementation reads webconfig properties, compares subdoc versions stored in the database against component versions in syscfg, and triggers a force reset for mismatched subdocs via RBUS.
Changes:
- Added new webcfg_selfheal module with property parsing and version mismatch detection logic
- Integrated selfheal boot check into ssp_main initialization sequence
- Added webcfg_selfheal.c to Makefile build sources
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 23 comments.
| File | Description |
|---|---|
| source/TandDSsp/webcfg_selfheal.h | New header file defining types, constants, and function declarations for the selfheal module |
| source/TandDSsp/webcfg_selfheal.c | New implementation file containing property parsing, version checking, and RBUS reset logic |
| source/TandDSsp/ssp_main.c | Integration of selfheal initialization and boot check after RBUS init |
| source/TandDSsp/Makefile.am | Added webcfg_selfheal.c to build sources |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
source/TandDSsp/webcfg_selfheal.c
Outdated
| CcspTraceInfo(("The value in supportedbits get is %s\n", supported_bits)); | ||
| return supported_bits; | ||
| } | ||
|
|
||
| char *getsupportedVersion(void) { | ||
| CcspTraceInfo(("The value in supportedversion get is %s\n", supported_version)); | ||
| return supported_version; | ||
| } | ||
|
|
||
| char *getsupplementaryDocs(void) { | ||
| CcspTraceInfo(("The value in supplementary_docs get is %s\n", supplementary_docs)); |
There was a problem hiding this comment.
Potential null pointer dereference: If supplementary_docs is NULL, the log statement will attempt to print a NULL pointer with %s format specifier, which can cause undefined behavior. Add a null check before logging or use a conditional operator to print a safe string like "(null)".
| CcspTraceInfo(("The value in supportedbits get is %s\n", supported_bits)); | |
| return supported_bits; | |
| } | |
| char *getsupportedVersion(void) { | |
| CcspTraceInfo(("The value in supportedversion get is %s\n", supported_version)); | |
| return supported_version; | |
| } | |
| char *getsupplementaryDocs(void) { | |
| CcspTraceInfo(("The value in supplementary_docs get is %s\n", supplementary_docs)); | |
| CcspTraceInfo(("The value in supportedbits get is %s\n", | |
| supported_bits ? supported_bits : "(null)")); | |
| return supported_bits; | |
| } | |
| char *getsupportedVersion(void) { | |
| CcspTraceInfo(("The value in supportedversion get is %s\n", | |
| supported_version ? supported_version : "(null)")); | |
| return supported_version; | |
| } | |
| char *getsupplementaryDocs(void) { | |
| CcspTraceInfo(("The value in supplementary_docs get is %s\n", | |
| supplementary_docs ? supplementary_docs : "(null)")); |
source/TandDSsp/webcfg_selfheal.c
Outdated
| data = realloc(data, len + n + 1); | ||
| memcpy(data + len, buf, n); len += n; |
There was a problem hiding this comment.
Potential buffer overflow: Using realloc without checking for allocation failure. If realloc fails and returns NULL, the function will dereference a NULL pointer when calling memcpy, causing a crash. Check if realloc returns NULL and handle the error appropriately by freeing the original data pointer and returning NULL.
| data = realloc(data, len + n + 1); | |
| memcpy(data + len, buf, n); len += n; | |
| char *tmp = realloc(data, len + n + 1); | |
| if(!tmp) { | |
| free(data); | |
| return NULL; | |
| } | |
| data = tmp; | |
| memcpy(data + len, buf, n); | |
| len += n; |
source/TandDSsp/webcfg_selfheal.c
Outdated
| temp = get_global_spInfoTail(); | ||
| temp->next = spInfo; | ||
| g_spInfoTail = spInfo; |
There was a problem hiding this comment.
Inconsistent spacing: This code uses tabs for indentation while the rest of the function uses spaces. Maintain consistent indentation throughout the file to improve readability.
| temp = get_global_spInfoTail(); | |
| temp->next = spInfo; | |
| g_spInfoTail = spInfo; | |
| temp = get_global_spInfoTail(); | |
| temp->next = spInfo; | |
| g_spInfoTail = spInfo; |
source/TandDSsp/ssp_main.c
Outdated
|
|
||
| // SelfHeal Subdoc Version Mismatch | ||
| initWebcfgProperties(WEBCFG_PROPERTIES_FILE); | ||
| webcfg_subdoc_mismatch_boot_check(); |
There was a problem hiding this comment.
Inconsistent spacing: This code uses tabs for indentation while the rest of the function uses spaces. Maintain consistent indentation throughout the file to improve readability.
source/TandDSsp/ssp_main.c
Outdated
|
|
||
| // SelfHeal Subdoc Version Mismatch | ||
| initWebcfgProperties(WEBCFG_PROPERTIES_FILE); | ||
| webcfg_subdoc_mismatch_boot_check(); |
There was a problem hiding this comment.
The webcfg_subdoc_mismatch_boot_check function is called during initialization but lacks error handling if it fails. Since this function performs critical selfheal operations and uses RBUS, consider adding error checking and logging to ensure initialization issues are properly reported. Additionally, verify that all required services (syscfg, RBUS, filesystem access) are available at this point in the startup sequence.
| webcfg_subdoc_mismatch_boot_check(); | |
| int webcfg_ret = webcfg_subdoc_mismatch_boot_check(); | |
| if (webcfg_ret != 0) | |
| { | |
| fprintf(stderr, "Error: webcfg_subdoc_mismatch_boot_check failed during initialization (ret=%d)\n", webcfg_ret); | |
| } |
| sdInfo = (SubDocSupportMap_t *)malloc(sizeof(SubDocSupportMap_t)); | ||
| if(sdInfo == NULL) { | ||
| fclose(fp); | ||
| CcspTraceError(("Unable to allocate memory\n")); | ||
| return; | ||
| } | ||
| memset(sdInfo, 0, sizeof(SubDocSupportMap_t)); | ||
|
|
||
| subtoken = strtok(subdoc, ":"); | ||
| if(subtoken == NULL) { | ||
| fclose(fp); | ||
| free(sdInfo); | ||
| return; |
There was a problem hiding this comment.
Memory leak on error path: When malloc fails or subtoken is NULL, the function returns without freeing the previously allocated linked list nodes in g_sdInfoHead/g_sdInfoTail. This causes a memory leak of all successfully parsed subdocs before the error occurred. Consider implementing proper cleanup of the linked list on error paths.
source/TandDSsp/webcfg_selfheal.c
Outdated
| reset_list = realloc(reset_list, reset_len + name_len + 2); | ||
| sprintf(reset_list + reset_len, "%s%s", reset_len ? "," : "", subdoc); |
There was a problem hiding this comment.
Potential buffer overflow: Using realloc without checking for allocation failure. If realloc fails and returns NULL, the original reset_list pointer is lost (memory leak) and the subsequent sprintf will cause a crash by writing to NULL. Store the realloc result in a temporary variable, check for NULL, and handle the error appropriately.
| char support[8]; | ||
| struct SubDocSupportMap *next; | ||
| } SubDocSupportMap_t; | ||
|
|
There was a problem hiding this comment.
Missing function documentation: This public function lacks a documentation comment explaining its purpose, parameters, return value, and any side effects. Add a function header comment describing what this function does, what it returns, and its error handling behavior.
| /** | |
| * @brief Initialize web configuration properties from a properties file. | |
| * | |
| * This function reads configuration values from the specified properties | |
| * file (for example, WEBCFG_PROPERTIES_FILE) and initializes the internal | |
| * web configuration state used by other WebConfig helper functions. | |
| * | |
| * @param[in] filename | |
| * Path to the properties file to be loaded. If NULL or an invalid path | |
| * is provided, the behavior is defined by the function implementation | |
| * and may result in partial or no initialization. | |
| * | |
| * @return void | |
| * | |
| * @note Error handling behavior (such as logging, use of defaults, or | |
| * leaving configuration unchanged) is implementation-specific. Callers | |
| * should rely on subsequent status-returning APIs to determine whether | |
| * initialization was successful. | |
| */ |
source/TandDSsp/webcfg_selfheal.c
Outdated
| const char *subdoc = name->valuestring; | ||
| int db_ver = ver->valueint; | ||
|
|
||
| if(is_ignored_subdoc(subdoc)) continue; | ||
| if (isSubDocSupported((char*)subdoc) != WEBCFG_SUCCESS) | ||
| { | ||
| CcspTraceInfo(("Skipping %s: subdoc not supported in webcfg.properties\n", subdoc)); | ||
| continue; | ||
| } | ||
|
|
||
| int comp_ver = -1; | ||
| if(Get_Component_Version(subdoc, &comp_ver) != 0) continue; | ||
|
|
||
| if(comp_ver != db_ver) { | ||
| CcspTraceInfo(("MISMATCH %s: DB=%d COMP=%d\n", subdoc, db_ver, comp_ver)); | ||
| count++; | ||
|
|
||
| size_t name_len = strlen(subdoc); | ||
| reset_list = realloc(reset_list, reset_len + name_len + 2); | ||
| sprintf(reset_list + reset_len, "%s%s", reset_len ? "," : "", subdoc); | ||
| reset_len = strlen(reset_list); |
There was a problem hiding this comment.
Inadequate error handling: The function doesn't validate that subdoc names from the JSON are safe to use in syscfg keys and RBUS parameter names. A malicious or corrupted JSON could contain subdoc names with special characters or format string specifiers that could cause issues. Consider adding input validation to ensure subdoc names contain only expected characters (alphanumeric and safe punctuation).
source/TandDSsp/webcfg_selfheal.c
Outdated
| #include "webcfg_selfheal.h" | ||
| #include "tad_rbus_apis.h" | ||
|
|
||
| rbusHandle_t g_rbusHandle; |
There was a problem hiding this comment.
This redeclares the global variable g_rbusHandle which is already defined in tad_rbus_apis.c. This will cause a linker error due to multiple definitions of the same symbol. Instead, this should be declared as 'extern rbusHandle_t g_rbusHandle;' to reference the existing definition, or the declaration should be added to tad_rbus_apis.h and included here.
| rbusHandle_t g_rbusHandle; | |
| extern rbusHandle_t g_rbusHandle; |
ksures101
force-pushed
the
feature/RDKB-62330
branch
from
320e2d5 to
e8ae7fd
Compare
Reason for change: Selfheal to recover devices when webconfig client and components have subdoc version mismatch. Test Procedure: Verify that when webconfig client and components have subdoc version mismatch the device should forcerest and recover the device. Priority: P1 Risks: None Signed-off-by: KavyaChowdahalli_Suresh@comcast.com
ksures101
force-pushed
the
feature/RDKB-62330
branch
from
e8ae7fd to
cd9fb62
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 29 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| int webcfg_ret = webcfg_subdoc_mismatch_boot_check(); | ||
| if (webcfg_ret != 0) | ||
| { | ||
| fprintf(stderr, "Error: webcfg_subdoc_mismatch_boot_check failed during initialization (ret=%d)\n", webcfg_ret); | ||
| } |
There was a problem hiding this comment.
The function webcfg_subdoc_mismatch_boot_check is declared with a void return type in the header file, but the calling code in ssp_main.c is attempting to capture and check a return value. The function should either be changed to return an int status, or the calling code should not expect a return value.
| SubDocSupportMap_t *sd = get_global_sdInfoHead(); | ||
|
|
||
| while(sd != NULL) { | ||
| if(strncmp(sd->name, subDoc, strlen(subDoc)) == 0) { |
There was a problem hiding this comment.
The string comparison uses strncmp with strlen(subDoc), which creates a potential issue. If sd->name is longer than subDoc and starts with subDoc as a prefix, this will match incorrectly. For example, "test" would match "testing". The comparison should verify both strings have the same length or use strcmp instead.
| if(strncmp(sd->name, subDoc, strlen(subDoc)) == 0) { | |
| if(strcmp(sd->name, subDoc) == 0) { |
| * initialization was successful. | ||
| */ | ||
|
|
||
| void initWebcfgProperties(char *filename); |
There was a problem hiding this comment.
The function parameter should be 'const char *filename' instead of 'char *filename' since the function only reads from the filename string and does not modify it. This improves const-correctness and allows callers to pass string literals or const strings without warnings.
| * initialization was successful. | |
| */ | |
| void initWebcfgProperties(char *filename); | |
| * initialization was successful. | |
| */ | |
| void initWebcfgProperties(const char *filename); |
| return; | ||
| } | ||
| memset(spInfo, 0, sizeof(SupplementaryDocs_t)); | ||
| spInfo->name = strdup(token); |
There was a problem hiding this comment.
When strdup(token) is called for spInfo->name, there is no check for allocation failure. If strdup returns NULL, this could lead to undefined behavior when the name is later accessed. A NULL check should be added after the strdup call.
| spInfo->name = strdup(token); | |
| spInfo->name = strdup(token); | |
| if(spInfo->name == NULL) { | |
| CcspTraceError(("Unable to allocate memory for supplementary doc name\n")); | |
| free(spInfo); | |
| free(docs_var); | |
| return; | |
| } |
| CcspTraceError(("Failed to grow reset_list buffer\n")); | ||
| free(reset_list); | ||
| reset_list = NULL; | ||
| break; // or return; depending on desired behavior |
There was a problem hiding this comment.
The comment "or return; depending on desired behavior" on line 470 indicates uncertainty about the correct error handling behavior. This should be resolved - the function should either break (continuing to check remaining subdocs) or return (stopping immediately). The current implementation breaks but leaves the cJSON object leaked, which suggests return might be more appropriate after cleanup.
| break; // or return; depending on desired behavior | |
| cJSON_Delete(arr); | |
| return; |
| SubDocSupportMap_t *tmp = NULL; | ||
| tmp = g_sdInfoHead; | ||
| return tmp; | ||
| } | ||
|
|
||
| SupplementaryDocs_t *get_global_spInfoHead(void) { | ||
| SupplementaryDocs_t *tmp = NULL; | ||
| tmp = g_spInfoHead; | ||
| return tmp; | ||
| } | ||
|
|
||
| SubDocSupportMap_t *get_global_sdInfoTail(void) | ||
| { | ||
| SubDocSupportMap_t *tmp = NULL; | ||
| tmp = g_sdInfoTail; | ||
| return tmp; | ||
| } | ||
|
|
||
| SupplementaryDocs_t *get_global_spInfoTail(void) | ||
| { | ||
| SupplementaryDocs_t *tmp = NULL; | ||
| tmp = g_spInfoTail; | ||
| return tmp; |
There was a problem hiding this comment.
These getter functions create an unnecessary temporary variable 'tmp' that simply holds the global variable value before returning it. The functions could directly return the global variable (e.g., 'return g_sdInfoHead;') for better code simplicity and efficiency.
| SubDocSupportMap_t *tmp = NULL; | |
| tmp = g_sdInfoHead; | |
| return tmp; | |
| } | |
| SupplementaryDocs_t *get_global_spInfoHead(void) { | |
| SupplementaryDocs_t *tmp = NULL; | |
| tmp = g_spInfoHead; | |
| return tmp; | |
| } | |
| SubDocSupportMap_t *get_global_sdInfoTail(void) | |
| { | |
| SubDocSupportMap_t *tmp = NULL; | |
| tmp = g_sdInfoTail; | |
| return tmp; | |
| } | |
| SupplementaryDocs_t *get_global_spInfoTail(void) | |
| { | |
| SupplementaryDocs_t *tmp = NULL; | |
| tmp = g_spInfoTail; | |
| return tmp; | |
| return g_sdInfoHead; | |
| } | |
| SupplementaryDocs_t *get_global_spInfoHead(void) { | |
| return g_spInfoHead; | |
| } | |
| SubDocSupportMap_t *get_global_sdInfoTail(void) | |
| { | |
| return g_sdInfoTail; | |
| } | |
| SupplementaryDocs_t *get_global_spInfoTail(void) | |
| { | |
| return g_spInfoTail; |
| } | ||
|
|
||
| if (is_ignored_subdoc(subdoc)) continue; | ||
| if (isSubDocSupported((char*)subdoc) != WEBCFG_SUCCESS) { |
There was a problem hiding this comment.
The const qualifier is being cast away when calling isSubDocSupported. The variable 'subdoc' is 'const char *' (from cJSON valuestring), but it's being cast to 'char *' to match the function signature. This is unsafe and could lead to undefined behavior if isSubDocSupported were to modify the string. The function signature should accept 'const char *' instead.
| if (isSubDocSupported((char*)subdoc) != WEBCFG_SUCCESS) { | |
| char *subdoc_mut = strdup(subdoc); | |
| if (!subdoc_mut) { | |
| CcspTraceError(("Failed to allocate memory for subdoc name '%s'\n", subdoc)); | |
| continue; | |
| } | |
| int subdoc_supported = isSubDocSupported(subdoc_mut); | |
| free(subdoc_mut); | |
| if (subdoc_supported != WEBCFG_SUCCESS) { |
|
|
||
| CcspTandDSsp_SOURCES = tad_rbus_apis.c ssp_messagebus_interface.c ssp_action_priv.c ssp_main.c ssp_action.c ssp_messagebus_interface_priv.c dm_pack_datamodel.c current_time.c | ||
| CcspTandDSsp_SOURCES = tad_rbus_apis.c ssp_messagebus_interface.c ssp_action_priv.c ssp_main.c ssp_action.c ssp_messagebus_interface_priv.c dm_pack_datamodel.c current_time.c webcfg_selfheal.c | ||
| CcspTandDSsp_LDFLAGS = -lccsp_common -ldl -rdynamic -lrbus -lsyscfg -lsecure_wrapper -ltelemetry_msgsender |
There was a problem hiding this comment.
The new webcfg_selfheal.c file uses cJSON library functions (cJSON_Parse, cJSON_Delete, cJSON_GetObjectItemCaseSensitive, etc.) but the Makefile.am does not include -lcjson in the LDFLAGS. This will likely cause linker errors unless cJSON is already implicitly linked through another dependency. The LDFLAGS should explicitly include -lcjson.
| CcspTandDSsp_LDFLAGS = -lccsp_common -ldl -rdynamic -lrbus -lsyscfg -lsecure_wrapper -ltelemetry_msgsender | |
| CcspTandDSsp_LDFLAGS = -lccsp_common -ldl -rdynamic -lrbus -lsyscfg -lsecure_wrapper -ltelemetry_msgsender -lcjson |
| static int Set_Webcfg_ForceReset(const char *reset_list) | ||
| { | ||
| rbusError_t err; | ||
|
|
||
| if(g_rbusHandle == NULL) | ||
| { | ||
| CcspTraceError(("%s: g_rbusHandle is NULL, RBUS not initialized\n", __FUNCTION__)); | ||
| return -1; | ||
| } | ||
|
|
||
| CcspTraceInfo(("%s: Setting webcfgSubdocForceReset='%s' via RBUS\n", | ||
| __FUNCTION__, reset_list ? reset_list : "(null)")); | ||
|
|
||
| err = rbus_setStr(g_rbusHandle, | ||
| "Device.X_RDK_WebConfig.webcfgSubdocForceReset", | ||
| (char *)reset_list); | ||
|
|
||
| if(err != RBUS_ERROR_SUCCESS) | ||
| { | ||
| CcspTraceError(("%s: rbus_setStr failed for webcfgSubdocForceReset, err=%d\n", | ||
| __FUNCTION__, err)); | ||
| return -1; | ||
| } | ||
|
|
||
| CcspTraceInfo(("%s: Successfully set webcfgSubdocForceReset\n", __FUNCTION__)); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
The function name 'Set_Webcfg_ForceReset' uses PascalCase with underscores, which is inconsistent with the C naming convention used elsewhere in the file (e.g., 'is_ignored_subdoc', 'read_pipe_data'). Static/private functions should use consistent snake_case naming for better code consistency.
|
|
||
| strtok(NULL, ":"); | ||
| subtoken = strtok(NULL, ":"); | ||
| if(subtoken != NULL) { | ||
| strncpy(sdInfo->support, subtoken, sizeof(sdInfo->support)-1); |
There was a problem hiding this comment.
The code at line 184 calls strtok(NULL, ":") and discards the result, presumably to skip a middle field in the format "name:middle:support". However, there's no validation that this call succeeds or that the format is correct before calling strtok again at line 185. If the format is "name:support" (missing middle field), line 185 would return NULL and support would remain empty (from memset at line 173), which may be the intended behavior but should be documented.
| strtok(NULL, ":"); | |
| subtoken = strtok(NULL, ":"); | |
| if(subtoken != NULL) { | |
| strncpy(sdInfo->support, subtoken, sizeof(sdInfo->support)-1); | |
| /* Parse optional middle and support fields. | |
| * Supported formats: | |
| * name:middle:support -> use third field as support | |
| * name:support -> use second field as support | |
| * name -> leave support empty | |
| */ | |
| subtoken = strtok(NULL, ":"); /* may be middle or support */ | |
| if(subtoken != NULL) { | |
| char *supportToken = strtok(NULL, ":"); | |
| if(supportToken != NULL) { | |
| /* name:middle:support */ | |
| strncpy(sdInfo->support, supportToken, sizeof(sdInfo->support)-1); | |
| } else { | |
| /* name:support (no middle field) */ | |
| strncpy(sdInfo->support, subtoken, sizeof(sdInfo->support)-1); | |
| } |
Reason for change: Selfheal to recover devices when webconfig client and components have subdoc version mismatch.
Test Procedure: Verify that when webconfig client and components have subdoc version mismatch the device should forcerest and recover the device.
Priority: P1
Risks: None
Signed-off-by: KavyaChowdahalli_Suresh@comcast.com