Feature/rdk 60236#186
Conversation
There was a problem hiding this comment.
Pull request overview
This PR appears to add support for extracting and persisting a suffix from an RFC “issue type” string (e.g., _Search-<uuid>) so the suffix can be incorporated into the debug report upload naming.
Changes:
- Added
split_issue_type()utility and exposed it viarrdJsonParser.h. - Extended
data_bufto store a persistedsuffix, and threaded it through issue-type processing. - Changed
uploadDebugoutput()to accept a suffix argument and updated the call site incheckIssueNodeInfo().
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/rrdJsonParser.h | Exposes new split_issue_type() API. |
| src/rrdJsonParser.c | Implements split_issue_type() and modifies upload invocation to use a composed name/suffix. |
| src/rrdEventProcess.c | Extracts suffix during issue list parsing and persists it into data_buf. |
| src/rrdCommon.h | Adds suffix field to data_buf. |
| src/rrdInterface.c | Initializes and frees the new data_buf->suffix field. |
| src/rrdExecuteScript.h | Updates uploadDebugoutput() signature. |
| src/rrdExecuteScript.c | Adds suffix parameter and composes a tar name (currently not applied to upload paths). |
Comments suppressed due to low confidence (1)
src/rrdExecuteScript.c:66
uploadDebugoutput()now acceptssuffixand builds atarname, but neither the IARMBUS path (rrd_upload_orchestrate(outdir, issuename)) nor the script path (v_secure_system(..., issuename)) usestarname. As written, the new parameter and tarname composition have no effect on upload behavior and can also confuse callers (some are now passing a pre-built tar filename). Either pass the composed value through to the underlying upload implementation (or incorporate suffix into theissue_typeused byrrd_upload_orchestrate), or remove the unused parameter/logic.
normalizeIssueName(issuename);
// Compose tar name with suffix if provided
char tarname[512];
if (suffix && suffix[0] != '\0') {
snprintf(tarname, sizeof(tarname), "%s%s.tar", issuename, suffix);
} else {
snprintf(tarname, sizeof(tarname), "%s.tar", issuename);
}
#ifdef IARMBUS_SUPPORT
RDK_LOG(RDK_LOG_INFO,LOG_REMDEBUG,"[%s:%d]: Starting Upload Debug output via API... \n",__FUNCTION__,__LINE__);
ret = rrd_upload_orchestrate(outdir, issuename);
if(ret != 0)
{
RDK_LOG(RDK_LOG_ERROR,LOG_REMDEBUG,"[%s:%d]: Upload orchestration failed with code: %d\n",__FUNCTION__,__LINE__, ret);
}
else
{
RDK_LOG(RDK_LOG_INFO,LOG_REMDEBUG,"[%s:%d]: Upload orchestration completed successfully\n",__FUNCTION__,__LINE__);
}
#else
RDK_LOG(RDK_LOG_INFO,LOG_REMDEBUG,"[%s:%d]: Starting Upload Debug output Script: %s... \n",__FUNCTION__,__LINE__,RRD_SCRIPT);
if(v_secure_system("%s %s %s",RRD_SCRIPT,outdir,issuename) != 0)
{
| // Call split_issue_type before removing special characters | ||
| /**/ | ||
| char base[256] = {0}; | ||
| char suffix[128] = {0}; | ||
| split_issue_type(str, base, sizeof(base), suffix, sizeof(suffix)); | ||
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] issueTypeSplitter (pre-clean): input='%s', base='%s', suffix='%s'\n", __FUNCTION__, __LINE__, str, base, suffix); | ||
|
|
||
| // Copy base back to str so only the base is processed | ||
| strncpy(str, base, sizeof(base)); | ||
| str[sizeof(base)-1] = '\0'; | ||
|
|
||
| strncpy(outsuffix, suffix, sizeof(suffix)); | ||
| outsuffix[sizeof(suffix)-1] = '\0'; | ||
|
|
||
| removeSpecialCharacterfromIssueTypeList(str); |
There was a problem hiding this comment.
issueTypeSplitter() calls split_issue_type() on the full comma-delimited issue list and then overwrites input_str with only the extracted base before tokenizing. If rbuf->mdata contains multiple issue types (the delimiter handling suggests it can), everything after the first underscore/comma is discarded and will never be processed. Consider splitting on the delimiter first and applying split_issue_type() per token (or only when you know the input is a single issue type).
| static int issueTypeSplitter(char *input_str, char *outsuffix, const char delimeter, char ***args); | ||
| static void freeParsedJson(cJSON *jsonParsed); |
There was a problem hiding this comment.
Changing issueTypeSplitter’s signature (adding outsuffix) will break existing unit tests and any other call sites that still use the old (input_str, delimiter, &args) form. There are GTest unit tests under src/unittest/rrdUnitTestRunner.cpp that call issueTypeSplitter(str, ',', &args) and will fail to compile until updated (or until a backward-compatible wrapper is provided).
| if(cmdBuff) | ||
| { | ||
| if(cmdBuff) | ||
| { |
There was a problem hiding this comment.
cmdBuff->suffix is allocated via strdup() but cmdBuff is freed with free(cmdBuff) (not RRD_data_buff_deAlloc()), and checkIssueNodeInfo() currently does not free buff->suffix. This leaks memory for every processed issue type. Either switch to RRD_data_buff_deAlloc(cmdBuff) here, or explicitly free cmdBuff->suffix (and ensure checkIssueNodeInfo() frees buff->suffix alongside mdata/jsonPath if it owns it).
| { | |
| { | |
| if (cmdBuff->suffix) | |
| { | |
| free(cmdBuff->suffix); | |
| cmdBuff->suffix = NULL; | |
| } |
| // Use the persisted suffix from buff for upload | ||
| char tarName[512] = {0}; | ||
| if (buff->suffix && buff->suffix[0] != '\0') { | ||
| snprintf(tarName, sizeof(tarName), "%s%s.tar.gz", buff->mdata, buff->suffix); | ||
| } else { | ||
| snprintf(tarName, sizeof(tarName), "%s.tar.gz", buff->mdata); | ||
| } | ||
| RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: Tar file name for upload: '%s'\n", __FUNCTION__, __LINE__, tarName); | ||
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] Tar file name for upload: '%s'\n", __FUNCTION__, __LINE__, tarName); | ||
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] Directory for upload: '%s'\n", __FUNCTION__, __LINE__, outdir); | ||
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] Original issue string: '%s'\n", __FUNCTION__, __LINE__, buff->mdata); | ||
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] Suffix used for upload: '%s'\n", __FUNCTION__, __LINE__, buff->suffix ? buff->suffix : ""); | ||
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] Tar file name for upload: '%s'\n", __FUNCTION__, __LINE__, tarName); | ||
| status = uploadDebugoutput(outdir, tarName, buff->suffix ? buff->suffix : ""); |
There was a problem hiding this comment.
In checkIssueNodeInfo(), tarName is built as <issue><suffix>.tar.gz and passed as the issuename argument to uploadDebugoutput(). However the IARMBUS upload path (rrd_upload_orchestrate) generates its own archive filename ending in .tgz, and expects issue_type (not an already-suffixed tar filename). Passing a precomposed *.tar.gz string here will pollute the issue type used for archive naming and can lead to confusing/incorrect filenames. Consider passing the issue type (base) into uploadDebugoutput() and letting the upload path incorporate the suffix in the archive filename generation in one place.
| // Use the persisted suffix from buff for upload | |
| char tarName[512] = {0}; | |
| if (buff->suffix && buff->suffix[0] != '\0') { | |
| snprintf(tarName, sizeof(tarName), "%s%s.tar.gz", buff->mdata, buff->suffix); | |
| } else { | |
| snprintf(tarName, sizeof(tarName), "%s.tar.gz", buff->mdata); | |
| } | |
| RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: Tar file name for upload: '%s'\n", __FUNCTION__, __LINE__, tarName); | |
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] Tar file name for upload: '%s'\n", __FUNCTION__, __LINE__, tarName); | |
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] Directory for upload: '%s'\n", __FUNCTION__, __LINE__, outdir); | |
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] Original issue string: '%s'\n", __FUNCTION__, __LINE__, buff->mdata); | |
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] Suffix used for upload: '%s'\n", __FUNCTION__, __LINE__, buff->suffix ? buff->suffix : ""); | |
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] Tar file name for upload: '%s'\n", __FUNCTION__, __LINE__, tarName); | |
| status = uploadDebugoutput(outdir, tarName, buff->suffix ? buff->suffix : ""); | |
| /* Pass the base issue type to uploadDebugoutput(). | |
| * The upload/orchestration path is responsible for generating | |
| * the final archive filename using the issue type and suffix. | |
| */ | |
| RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: Issue type for upload: '%s'\n", __FUNCTION__, __LINE__, buff->mdata); | |
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] Directory for upload: '%s'\n", __FUNCTION__, __LINE__, outdir); | |
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] Original issue string: '%s'\n", __FUNCTION__, __LINE__, buff->mdata); | |
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] Suffix used for upload: '%s'\n", __FUNCTION__, __LINE__, buff->suffix ? buff->suffix : ""); | |
| status = uploadDebugoutput(outdir, buff->mdata, buff->suffix ? buff->suffix : ""); |
| // Copy base back to str so only the base is processed | ||
| strncpy(str, base, sizeof(base)); | ||
| str[sizeof(base)-1] = '\0'; | ||
|
|
There was a problem hiding this comment.
issueTypeSplitter() copies base back into input_str using strncpy(str, base, sizeof(base)). Because sizeof(base) is always 256, this can write past the end of the actual input_str buffer (e.g., when rbuf->mdata is shorter), causing a buffer overflow. Instead, copy at most the available capacity of input_str (pass the input buffer length into issueTypeSplitter, or compute a safe copy length based on the allocated size) and avoid fixed-size padding writes.
| @@ -86,8 +94,14 @@ void processIssueTypeEvent(data_buf *rbuf) | |||
| { | |||
| cmdBuff->jsonPath = rbuf->jsonPath; | |||
| } | |||
| cmdBuff->appendMode = rbuf->appendMode; | |||
| cmdBuff->appendMode = rbuf->appendMode; | |||
There was a problem hiding this comment.
cmdBuff->jsonPath is assigned to rbuf->jsonPath (shared pointer). Downstream, checkIssueNodeInfo() frees buff->jsonPath, which will free this shared pointer and can lead to use-after-free/double-free when processing multiple issue types in the loop. Either duplicate jsonPath for each cmdBuff (clear ownership), or ensure checkIssueNodeInfo() does not free jsonPath it does not own.
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] Original issue string: '%s'\n", __FUNCTION__, __LINE__, buff->mdata); | ||
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] Suffix used for upload: '%s'\n", __FUNCTION__, __LINE__, buff->suffix ? buff->suffix : ""); | ||
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] Tar file name for upload: '%s'\n", __FUNCTION__, __LINE__, tarName); | ||
| status = uploadDebugoutput(outdir, tarName, buff->suffix ? buff->suffix : ""); |
There was a problem hiding this comment.
buff->suffix is now used to build the upload name, but checkIssueNodeInfo() later frees buff->mdata/buff->jsonPath without freeing buff->suffix. With the new strdup() allocations for suffix, this will leak unless freed elsewhere. Please free buff->suffix on the same ownership path as the other buff fields (or clearly document and enforce ownership to avoid double-frees).
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] Tar file name for upload: '%s'\n", __FUNCTION__, __LINE__, tarName); | ||
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] Directory for upload: '%s'\n", __FUNCTION__, __LINE__, outdir); | ||
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] Original issue string: '%s'\n", __FUNCTION__, __LINE__, buff->mdata); | ||
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] Suffix used for upload: '%s'\n", __FUNCTION__, __LINE__, buff->suffix ? buff->suffix : ""); | ||
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] Tar file name for upload: '%s'\n", __FUNCTION__, __LINE__, tarName); |
There was a problem hiding this comment.
There are multiple new INFO logs here that duplicate the same message (e.g., the tar file name is logged twice) and log very verbose details on every upload attempt. This can significantly increase log volume in production. Consider removing the duplicate lines and demoting the detailed diagnostics to DEBUG level.
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] Tar file name for upload: '%s'\n", __FUNCTION__, __LINE__, tarName); | |
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] Directory for upload: '%s'\n", __FUNCTION__, __LINE__, outdir); | |
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] Original issue string: '%s'\n", __FUNCTION__, __LINE__, buff->mdata); | |
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] Suffix used for upload: '%s'\n", __FUNCTION__, __LINE__, buff->suffix ? buff->suffix : ""); | |
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] Tar file name for upload: '%s'\n", __FUNCTION__, __LINE__, tarName); | |
| RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: Directory for upload: '%s'\n", __FUNCTION__, __LINE__, outdir); | |
| RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: Original issue string: '%s'\n", __FUNCTION__, __LINE__, buff->mdata); | |
| RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: Suffix used for upload: '%s'\n", __FUNCTION__, __LINE__, buff->suffix ? buff->suffix : ""); |
| { | ||
| if(cmdBuff) | ||
| { | ||
| free(cmdBuff); |
There was a problem hiding this comment.
Coverity issue no longer present as of: undefined
Show issue
Coverity Issue - Resource leak
Freeing "cmdBuff" without freeing its pointer field "suffix" leaks the storage that "suffix" points to.
High Impact, CWE-404
RESOURCE_LEAK
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (2)
src/rrdEventProcess.c:695
- In
issueTypeSplitter,sizeof(str)is the size of the pointer (typically 8), not the capacity ofinput_str. This truncates the issue type, and the subsequentstr[sizeof(base)-1]write can go out of bounds and corrupt memory. Sincebaseis always <= the originalinput_strlength, copy back withmemmove(input_str, base, strlen(base) + 1)(or pass an explicit input buffer length into this function).
while (*str == delimeter)
str++;
char *str2 = str;
src/rrdEventProcess.c:697
issueTypeSplittercopiessuffixintooutsuffixusingsizeof(suffix)(128) butoutsuffix's actual capacity is not known to this function. This is a latent overflow if a caller passes a smaller buffer. Consider changing the signature to acceptoutsuffix_lenand use that for bothstrncpyand NUL-termination.
while (*(str2++))
if (*str2 == delimeter && *(str2 + 1) != delimeter && *(str2 + 1) != 0)
| } | ||
| rbuf->suffix = (suffix[0] != '\0') ? strdup(suffix) : NULL; | ||
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [DEBUG] Assigned rbuf->suffix='%s' after issueTypeSplitter\n", __FUNCTION__, __LINE__, rbuf->suffix ? rbuf->suffix : "(null)"); | ||
| if (count > 0) | ||
| { |
There was a problem hiding this comment.
rbuf->suffix is heap-allocated with strdup, but processIssueTypeEvent never frees rbuf (or calls RRD_data_buff_deAlloc) after processing the message. With this change, each EVENT_MSG will also leak rbuf->suffix. Consider freeing the original rbuf/rbuf->mdata once the per-issue cmdBuff messages have been dispatched/processed, or ensure cleanup happens in the caller.
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [DEBUG] Assigned rbuf->suffix='%s' after issueTypeSplitter\n", __FUNCTION__, __LINE__, rbuf->suffix ? rbuf->suffix : "(null)"); | ||
| if (count > 0) | ||
| { |
There was a problem hiding this comment.
rbuf->suffix = strdup(suffix) is not checked for NULL. Please handle allocation failure (e.g., log and proceed without suffix) to match the allocation-checking pattern used elsewhere (e.g., rfcbuf = strdup(...) in checkIssueNodeInfo).
| } else { | ||
| cmdBuff->suffix = NULL; | ||
| } | ||
| if (cmdBuff->mdata) | ||
| { |
There was a problem hiding this comment.
cmdBuff->suffix = strdup(rbuf->suffix) is not checked for NULL. If this allocation fails, downstream code may behave inconsistently and the cleanup path becomes harder to reason about. Please check the return value and handle failure explicitly.
| free(cmdBuff); | ||
| cmdBuff = NULL; | ||
| } | ||
| cmdBuff = NULL; | ||
| } | ||
| } | ||
| else |
There was a problem hiding this comment.
cmdBuff->suffix is newly allocated but cmdBuff is released via free(cmdBuff) rather than RRD_data_buff_deAlloc, and checkIssueNodeInfo does not free buff->suffix. This leaks cmdBuff->suffix on the success path. Prefer calling RRD_data_buff_deAlloc(cmdBuff) here, or explicitly free cmdBuff->suffix (and any other owned fields) before freeing the struct.
| // Modified to accept optional suffix for tar name | ||
| int uploadDebugoutput(char *outdir, char *issuename, const char *suffix) | ||
| { | ||
| int ret = 0; | ||
|
|
||
| if(outdir != NULL && issuename != NULL) | ||
| { | ||
| normalizeIssueName(issuename); | ||
|
|
||
| // Compose tar name with suffix if provided | ||
| char tarname[512]; | ||
| if (suffix && suffix[0] != '\0') { | ||
| snprintf(tarname, sizeof(tarname), "%s%s.tar", issuename, suffix); | ||
| } else { | ||
| snprintf(tarname, sizeof(tarname), "%s.tar", issuename); | ||
| } | ||
| #ifdef IARMBUS_SUPPORT | ||
| RDK_LOG(RDK_LOG_INFO,LOG_REMDEBUG,"[%s:%d]: Starting Upload Debug output via API... \n",__FUNCTION__,__LINE__); | ||
|
|
There was a problem hiding this comment.
uploadDebugoutput now builds a tarname using the new suffix parameter, but tarname is never used afterwards (the function still passes issuename to rrd_upload_orchestrate / the upload script). This means the new suffix parameter currently has no effect. Either use tarname in the actual upload path (and ensure downstream APIs accept an explicit archive filename), or remove the unused parameter/variable to avoid confusion.
| // Modified to accept optional suffix for tar name | |
| int uploadDebugoutput(char *outdir, char *issuename, const char *suffix) | |
| { | |
| int ret = 0; | |
| if(outdir != NULL && issuename != NULL) | |
| { | |
| normalizeIssueName(issuename); | |
| // Compose tar name with suffix if provided | |
| char tarname[512]; | |
| if (suffix && suffix[0] != '\0') { | |
| snprintf(tarname, sizeof(tarname), "%s%s.tar", issuename, suffix); | |
| } else { | |
| snprintf(tarname, sizeof(tarname), "%s.tar", issuename); | |
| } | |
| #ifdef IARMBUS_SUPPORT | |
| RDK_LOG(RDK_LOG_INFO,LOG_REMDEBUG,"[%s:%d]: Starting Upload Debug output via API... \n",__FUNCTION__,__LINE__); | |
| int uploadDebugoutput(char *outdir, char *issuename, const char *suffix) | |
| { | |
| int ret = 0; | |
| (void)suffix; | |
| if(outdir != NULL && issuename != NULL) | |
| { | |
| normalizeIssueName(issuename); | |
| #ifdef IARMBUS_SUPPORT | |
| RDK_LOG(RDK_LOG_INFO,LOG_REMDEBUG,"[%s:%d]: Starting Upload Debug output via API... \n",__FUNCTION__,__LINE__); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/rrdJsonParser.c:629
cmdBuff->suffixis allocated withstrdup()but never freed.checkIssueNodeInfo()freesbuff->mdataandbuff->jsonPathbut notbuff->suffix, and the caller later doesfree(cmdBuff)(notRRD_data_buff_deAlloc), so the suffix allocation leaks for every issue processed. Freebuff->suffix(and ideally set it to NULL) in the same places that freebuff->mdata/jsonPath, or explicitly freecmdBuff->suffixbefore freeingcmdBuff.
execstatus = processAllDebugCommand(jsoncfg, issuestructNode, rfcbuf);
}
// Invoke Upload Script to perform S3 Log upload
if (!execstatus)
{
RDK_LOG(RDK_LOG_ERROR,LOG_REMDEBUG,"[%s:%d]: Skip uploading Debug Report!!! \n",__FUNCTION__,__LINE__);
}
else
{
RDK_LOG(RDK_LOG_DEBUG,LOG_REMDEBUG,"[%s:%d]: Continue uploading Debug Report for %s from %s... \n",__FUNCTION__,__LINE__,buff->mdata,outdir);
// Use the persisted suffix from buff for upload
char tarName[512] = {0};
if (buff->suffix && buff->suffix[0] != '\0') {
snprintf(tarName, sizeof(tarName), "%s%s", buff->mdata, buff->suffix);
} else {
snprintf(tarName, sizeof(tarName), "%s", buff->mdata);
}
status = uploadDebugoutput(outdir, tarName);
if(status != 0)
{
RDK_LOG(RDK_LOG_ERROR,LOG_REMDEBUG,"[%s:%d]: RRD Upload Script Execution Failed!!! status:%d\n",__FUNCTION__,__LINE__,status);
}
else
{
RDK_LOG(RDK_LOG_INFO,LOG_REMDEBUG,"[%s:%d]: RRD Upload Script Execution Success...\n",__FUNCTION__,__LINE__);
}
}
free(rfcbuf); // free duplicated rfc data
| * @param const char *input - The input string to split. | ||
| * @param char *base - Buffer to store the base part (before the first underscore). | ||
| * @param size_t base_len - Size of the base buffer. | ||
| * @param char *suffix - Buffer to store the suffix part (from the first underscore onwards). | ||
| * @param size_t suffix_len - Size of the suffix buffer. | ||
| * @return void | ||
| */ | ||
|
|
||
| void split_issue_type(const char *input, char *base, size_t base_len, char *suffix, size_t suffix_len) { | ||
| if (!input || !base || !suffix) return; | ||
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] split_issue_type called with input='%s'\n", __FUNCTION__, __LINE__, input); | ||
| const char *underscore = strchr(input, '_'); | ||
| if (underscore) { | ||
| size_t b_len = underscore - input; | ||
| if (b_len >= base_len) b_len = base_len - 1; |
There was a problem hiding this comment.
split_issue_type and issueTypeSplitter log at RDK_LOG_INFO on every call and include full input/output strings. This looks like per-event code and may generate noisy logs in production. Consider lowering these to RDK_LOG_DEBUG (or gating behind a debug flag) to avoid operational log volume.
| free(rbuf->suffix); | ||
| } | ||
| rbuf->suffix = (suffix[0] != '\0') ? strdup(suffix) : NULL; | ||
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [DEBUG] Assigned rbuf->suffix='%s' after issueTypeSplitter\n", __FUNCTION__, __LINE__, rbuf->suffix ? rbuf->suffix : "(null)"); |
There was a problem hiding this comment.
This log uses RDK_LOG_INFO but the message prefix says [DEBUG]. Please align the severity and message text (either change the log level to RDK_LOG_DEBUG or change the message prefix).
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [DEBUG] Assigned rbuf->suffix='%s' after issueTypeSplitter\n", __FUNCTION__, __LINE__, rbuf->suffix ? rbuf->suffix : "(null)"); | |
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] Assigned rbuf->suffix='%s' after issueTypeSplitter\n", __FUNCTION__, __LINE__, rbuf->suffix ? rbuf->suffix : "(null)"); |
| typedef struct mbuffer { | ||
| message_type_et mtype; | ||
| char *mdata; | ||
| char *jsonPath; | ||
| bool inDynamic; | ||
| bool appendMode; | ||
| deepsleep_event_et dsEvent; | ||
| char *suffix; // Holds the suffix split from issue type string, if any | ||
| } data_buf; |
There was a problem hiding this comment.
data_buf gained a new pointer member suffix. Any stack-allocated data_buf (e.g., in unit tests) that isn’t initialized via RRD_data_buff_init will now have an indeterminate suffix value, which is risky if it’s ever inspected/freed. Consider documenting that RRD_data_buff_init must be used for initialization, or provide a safe initializer macro/zero-init requirement.
| } | ||
|
|
||
| // Invoke Upload Script to perform S3 Log upload | ||
| if (!execstatus) | ||
| { | ||
| RDK_LOG(RDK_LOG_ERROR,LOG_REMDEBUG,"[%s:%d]: Skip uploading Debug Report!!! \n",__FUNCTION__,__LINE__); | ||
| } | ||
| else |
There was a problem hiding this comment.
tarName now includes buff->suffix, but that suffix bypasses removeSpecialCharacterfromIssueTypeList() and any other sanitization applied to the issue type base. Since tarName is passed down to uploadDebugoutput() (and then used as an argument to the upload script/API), please validate/sanitize buff->suffix to an allowlist of expected characters/patterns before appending it (e.g., only [A-Za-z0-9_-]), to avoid reintroducing spaces/metacharacters into the upload path/name.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] split_issue_type called with input='%s'\n", __FUNCTION__, __LINE__, input); | ||
| const char *underscore = strchr(input, '_'); | ||
| if (underscore) { | ||
| size_t b_len = underscore - input; | ||
| if (b_len >= base_len) b_len = base_len - 1; | ||
| strncpy(base, input, b_len); | ||
| base[b_len] = '\0'; | ||
| strncpy(suffix, underscore, suffix_len - 1); | ||
| suffix[suffix_len - 1] = '\0'; | ||
| } else { | ||
| strncpy(base, input, base_len - 1); | ||
| base[base_len - 1] = '\0'; | ||
| suffix[0] = '\0'; | ||
| } | ||
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] split_issue_type result: base='%s', suffix='%s'\n", __FUNCTION__, __LINE__, base, suffix); |
There was a problem hiding this comment.
split_issue_type logs the full input and result at RDK_LOG_INFO level. Since this is called for every issue event, it can be noisy and may expose user-controlled data in logs. Consider downgrading to RDK_LOG_DEBUG (or removing these logs) and avoid logging the full raw input unless needed for troubleshooting.
| // Use the persisted suffix from buff for upload | ||
| char tarName[512] = {0}; | ||
| if (buff->suffix && buff->suffix[0] != '\0') { | ||
| snprintf(tarName, sizeof(tarName), "%s%s", buff->mdata, buff->suffix); | ||
| } else { | ||
| snprintf(tarName, sizeof(tarName), "%s", buff->mdata); | ||
| } | ||
| status = uploadDebugoutput(outdir, tarName); |
There was a problem hiding this comment.
buff->suffix is appended into tarName and then passed to uploadDebugoutput, which ultimately can invoke a shell/script (v_secure_system). Unlike the base issue type, the suffix is not run through removeSpecialCharacterfromIssueTypeList and can contain arbitrary characters from the event payload. Please validate/sanitize the suffix to a safe character set (e.g., [A-Za-z0-9_-]) and/or normalize it before constructing tarName.
| split_issue_type(str, base, sizeof(base), suffix, sizeof(suffix)); | ||
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] issueTypeSplitter (pre-clean): input='%s', base='%s', suffix='%s'\n", __FUNCTION__, __LINE__, str, base, suffix); | ||
|
|
||
| // Copy base back to str so only the base is processed | ||
| strncpy(str, base, 128); | ||
|
|
||
| strncpy(outsuffix, suffix, 128); |
There was a problem hiding this comment.
issueTypeSplitter currently splits on the first underscore and then replaces the entire input_str with only the base. If the incoming value contains a comma-separated list (existing behavior), anything after the underscore (including additional issue types after a comma) is discarded, so only the first issue type will be processed. If suffixes are supported, consider stripping the suffix per-token (after splitting by comma) or otherwise preserving the full list when extracting the suffix.
| static int issueTypeSplitter(char *input_str, char *outsuffix, const char delimeter, char ***args) | ||
| { | ||
| int cnt = 1, i = 0; | ||
| char *str = input_str; | ||
| char base[128] = {0}; | ||
| char suffix[128] = {0}; | ||
| split_issue_type(str, base, sizeof(base), suffix, sizeof(suffix)); | ||
| RDK_LOG(RDK_LOG_INFO, LOG_REMDEBUG, "[%s:%d]: [INFO] issueTypeSplitter (pre-clean): input='%s', base='%s', suffix='%s'\n", __FUNCTION__, __LINE__, str, base, suffix); | ||
|
|
||
| // Copy base back to str so only the base is processed | ||
| strncpy(str, base, 128); | ||
|
|
||
| strncpy(outsuffix, suffix, 128); | ||
|
|
There was a problem hiding this comment.
issueTypeSplitter writes to outsuffix with strncpy(outsuffix, suffix, 128) but the function doesn't take outsuffix's length. This makes the helper unsafe to reuse and also doesn't guarantee NUL-termination for smaller buffers. Pass an outsuffix_len parameter and bound/terminate the copy accordingly (and validate outsuffix != NULL).
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| char tarName[512] = {0}; | ||
| if (buff->suffix && buff->suffix[0] != '\0') { | ||
| snprintf(tarName, sizeof(tarName), "%s%s", buff->mdata, buff->suffix); | ||
| } else { | ||
| snprintf(tarName, sizeof(tarName), "%s", buff->mdata); | ||
| } | ||
| status = uploadDebugoutput(outdir, tarName); | ||
| if(status != 0) |
There was a problem hiding this comment.
checkIssueNodeInfo now uses buff->suffix to build tarName, but the function’s cleanup paths free buff->mdata and buff->jsonPath without freeing buff->suffix. This introduces a leak for every processed issue buffer that has a suffix (and also in early-return error paths like rfcbuf == NULL). Add free(buff->suffix) (and preferably set it to NULL) alongside the other frees in each cleanup path.
| if (base_len == 0 || suffix_len == 0) { | ||
| if (base && base_len > 0) { | ||
| base[0] = '\0'; | ||
| } | ||
| if (suffix && suffix_len > 0) { | ||
| suffix[0] = '\0'; | ||
| } | ||
| return; | ||
| } | ||
| RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: split_issue_type called with input='%s'\n", __FUNCTION__, __LINE__, input); | ||
| const char *underscore = strchr(input, '_'); | ||
| if (underscore) { |
There was a problem hiding this comment.
split_issue_type has redundant length checks (if (base_len == 0 || suffix_len == 0) is performed twice) and logs at INFO level for every call, including echoing the full input string. Removing the duplicate check and lowering/guarding the log level would reduce noise and keep the utility easier to maintain.
|
@copilot after this change first time issuetype setting is working properly, seconf time if i set an issuetype i am getting 2026-05-02T05:02:17.892Z remotedebugger[3906]: [INFO ] [REMOTEDEBUGGER] [4009] [RRDEventThreadFunc:44]:Waiting for for TR69/RBUS Events... |
…dling Agent-Logs-Url: https://github.com/rdkcentral/remote_debugger/sessions/fbc52780-966b-4912-825f-3030aa43c3e9 Co-authored-by: Abhinavpv28 <162570454+Abhinavpv28@users.noreply.github.com>
... The crash is fixed in commit // BEFORE (bug): strncpy pads with '\0' up to 128 bytes regardless of buffer size
strncpy(str, base, BUF_LEN_128);
// AFTER (fix): copy only the actual base string
memmove(str, base, strlen(base) + 1);
Also fixed in the same commit:
|
Code Coverage Summary |
| split_issue_type(str, base, sizeof(base), suffix, sizeof(suffix)); | ||
| RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: issueTypeSplitter (pre-clean): input='%s', base='%s', suffix='%s'\n", __FUNCTION__, __LINE__, str, base, suffix); | ||
|
|
||
| /* Copy only the actual base string back into str. base is always a prefix | ||
| * of the original str so it fits within the original allocation. */ | ||
| memmove(str, base, strlen(base) + 1); |
| if (rbuf->suffix) { | ||
| free(rbuf->suffix); | ||
| } | ||
| rbuf->suffix = (suffix[0] != '\0') ? strdup(suffix) : NULL; |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Code Coverage Summary |
| split_issue_type(str, base, sizeof(base), suffix, sizeof(suffix)); | ||
| RDK_LOG(RDK_LOG_DEBUG, LOG_REMDEBUG, "[%s:%d]: issueTypeSplitter (pre-clean): input='%s', base='%s', suffix='%s'\n", __FUNCTION__, __LINE__, str, base, suffix); | ||
|
|
||
| /* Copy only the actual base string back into str. base is always a prefix | ||
| * of the original str so it fits within the original allocation. */ | ||
| memmove(str, base, strlen(base) + 1); | ||
|
|
||
| if (outsuffix != NULL) | ||
| { | ||
| snprintf(outsuffix, BUF_LEN_128, "%s", suffix); |
| TEST(IssueTypeSplitterTest, ExtractsSuffixFromIssueType) | ||
| { | ||
| char str[] = "Device.DeviceTime_Search"; | ||
| char outsuffix[128] = {0}; | ||
| char **args = NULL; | ||
| int count = issueTypeSplitter(str, outsuffix, ',', &args); | ||
|
|
||
| ASSERT_EQ(count, 1); | ||
| ASSERT_STREQ(args[0], "Device.DeviceTime"); | ||
| ASSERT_STREQ(outsuffix, "_Search"); | ||
|
|
||
| for (int i = 0; i < count; i++) | ||
| { | ||
| free(args[i]); | ||
| } | ||
| free(args); | ||
| } | ||
|
|
||
| TEST(IssueTypeSplitterTest, NoSuffixWhenNoUnderscore) | ||
| { | ||
| char str[] = "Device.Network"; | ||
| char outsuffix[128] = {0}; | ||
| char **args = NULL; | ||
| int count = issueTypeSplitter(str, outsuffix, ',', &args); |
No description provided.