Conversation
…de due to image verification failure error
…de due to image verification failure error
…de due to image verification failure error
…de due to image verification failure error- updating l1
…de due to image verification failure error- Addressing review comments
…de due to image verification failure error- Addressing review comments
RDKTV-39792:[RDKV] Xumo TV devices with older builds failing to upgrade due to image verification failure error
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 48 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| echo "*********** Return value of downloadUtil_gtest $mtls_upload" | ||
|
|
||
|
|
||
| ./uploadutil/codebig_upload_gtest | ||
| codebig_upload=$? | ||
| echo "*********** Return value of downloadUtil_gtest $codebig_upload" | ||
|
|
||
| ./uploadutil/uploadUtil_gtest | ||
| uploadUtil=$? | ||
| echo "*********** Return value of downloadUtil_gtest $uploadUtil" | ||
|
|
||
|
|
||
| ./uploadutil/upload_status_gtest | ||
| upload_status=$? | ||
| echo "*********** Return value of downloadUtil_gtest $upload_status" |
There was a problem hiding this comment.
The echo messages for the new upload test results all say "Return value of downloadUtil_gtest" but should say the correct test name (e.g., "mtls_upload_gtest", "codebig_upload_gtest", "uploadUtil_gtest", "upload_status_gtest" respectively). This makes it hard to diagnose which test failed.
| echo "*********** Return value of downloadUtil_gtest $mtls_upload" | |
| ./uploadutil/codebig_upload_gtest | |
| codebig_upload=$? | |
| echo "*********** Return value of downloadUtil_gtest $codebig_upload" | |
| ./uploadutil/uploadUtil_gtest | |
| uploadUtil=$? | |
| echo "*********** Return value of downloadUtil_gtest $uploadUtil" | |
| ./uploadutil/upload_status_gtest | |
| upload_status=$? | |
| echo "*********** Return value of downloadUtil_gtest $upload_status" | |
| echo "*********** Return value of mtls_upload_gtest $mtls_upload" | |
| ./uploadutil/codebig_upload_gtest | |
| codebig_upload=$? | |
| echo "*********** Return value of codebig_upload_gtest $codebig_upload" | |
| ./uploadutil/uploadUtil_gtest | |
| uploadUtil=$? | |
| echo "*********** Return value of uploadUtil_gtest $uploadUtil" | |
| ./uploadutil/upload_status_gtest | |
| upload_status=$? | |
| echo "*********** Return value of upload_status_gtest $upload_status" |
| size_t i = 0; | ||
| char buf[256]; | ||
| char *timezonefile; | ||
| char *defaultimezone = "Universal"; |
There was a problem hiding this comment.
The variable defaultimezone is misspelled — it should be defaultTimezone or defaultTimezone (missing 't' in "timezone"). While the code still works, this is a naming issue that could cause confusion.
| COMMONUTILITIES_DEBUG("GetTimezone: Returning timezone '%s' with length %zu\n", pTimezone, i); | ||
| return i; |
There was a problem hiding this comment.
The GetTimezone function accesses pTimezone in the debug log on line 647 even when pTimezone is NULL (the else branch on line 642-645 handles the NULL case, but execution falls through to line 647 regardless). When pTimezone is NULL, COMMONUTILITIES_DEBUG at line 647 will dereference it via %s, causing undefined behavior.
| size_t allocate_size = path_len + strlen(entry->d_name) + 2; | ||
| if (entry->d_type == DT_DIR) { | ||
| // determinate a full path of an entry | ||
| full_path = calloc(path_len + strlen(entry->d_name) + 2, sizeof(char)); | ||
| strcpy(full_path, path); | ||
| strcat(full_path, "/"); | ||
| strcat(full_path, entry->d_name); | ||
|
|
||
| full_path = construct_full_path(path, entry->d_name); | ||
| found = findPFileAll(full_path, search, out, found_t, max_list); | ||
| } | ||
| else if(fnmatch(search, entry->d_name, 0) == 0) { | ||
| if(out[*found_t]) { | ||
| strcpy(out[*found_t], path); | ||
| strcat(out[*found_t], "/"); | ||
| strcat(out[*found_t], entry->d_name); | ||
| snprintf(out[*found_t], allocate_size , "%s/%s", path, entry->d_name); |
There was a problem hiding this comment.
The findPFileAll function computes allocate_size on line 631 and uses it to limit snprintf on line 639. However, allocate_size is the size needed to store the full path, not necessarily the size of the pre-allocated out[*found_t] buffer. The caller allocates the out buffers, and their actual size could be different from allocate_size. If the pre-allocated buffer is smaller than allocate_size, snprintf will truncate; if it's larger, you're just wasting capacity. This is safer than the previous strcpy/strcat, but the allocate_size parameter to snprintf should ideally reflect the actual buffer size, not a computed one.
| char* constructed_path = construct_full_path(path, entry->d_name); | ||
| if (constructed_path) { | ||
| strncpy(out, constructed_path, strlen(constructed_path) + 1); // Copy path to out |
There was a problem hiding this comment.
The construct_full_path function is declared static inline in the .c file. While this works, inline on a function that is static in a .c file is redundant — the compiler can inline it regardless. This is a minor style nit, but more importantly, the function allocates memory that callers must free. In findPFile at line 555-556, full_path is allocated via construct_full_path, then findPFile is called recursively. If the recursive call to findPFile internally calls construct_full_path again, memory is still properly freed later. This pattern is fine.
However, in findPFile at lines 560-562, the strncpy(out, constructed_path, strlen(constructed_path) + 1) is equivalent to strcpy(out, constructed_path) and doesn't provide any bounds protection since the size passed is based on the source, not the destination buffer. The out buffer's actual size is unknown here, making this potentially unsafe.
| if( pTmp) | ||
| { | ||
| ++pTmp; | ||
| } | ||
| else | ||
| { | ||
| continue; | ||
| } | ||
| if( i == eBUILD_TYPE ) | ||
| { | ||
| pDevice_info->eBuildType = getbuild( pTmp ); | ||
| if( pTmp) | ||
| { | ||
| pDevice_info->eBuildType = getbuild( pTmp ); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| snprintf( pDest, DevicePropertySizes[i], "%s", pTmp ); | ||
| if( pTmp) | ||
| { | ||
| snprintf( pDest, DevicePropertySizes[i], "%s", pTmp ); | ||
| } |
There was a problem hiding this comment.
The null check for pTmp on lines 180 and 190/197 is redundant after the continue on line 186 guarantees pTmp is non-NULL past that point. When pTmp is NULL after strchr(pTmp, '='), the code correctly continues to the next loop iteration. But the subsequent if( pTmp) checks on lines 190 and 197 will always be true because if pTmp were NULL, the continue on line 186 would have been hit. While not harmful, this adds unnecessary nesting. Also, the indentation uses tabs inconsistently mixed with spaces.
| char data[32]; | ||
| EXPECT_NE(GetPartnerId(data, 7),0); | ||
| printf("Pertner ID = %s\n", data); | ||
| EXPECT_EQ(GetPartnerId(data, 7), 0); |
There was a problem hiding this comment.
The test assertion was changed from EXPECT_NE(..., 0) to EXPECT_EQ(..., 0). However, with the new fallback logic, when no source file exists, GetPartnerId now falls through to popen("syscfg get PartnerID", ...) or the "comcast" default. In either case, stripinvalidchar will return a non-zero length for non-empty output. With szBufSize=7, snprintf(pPartnerId, 7, "comcast") writes "comcas" and stripinvalidchar returns 6, so GetPartnerId returns 6, not 0. This test will likely fail. The expectation should remain EXPECT_NE(..., 0) or be updated to match the expected fallback length.
| EXPECT_EQ(GetPartnerId(data, 7), 0); | |
| EXPECT_NE(GetPartnerId(data, 7), 0); |
| if (entry->d_type == DT_DIR) { | ||
| emptyFolder(filePath); | ||
|
|
||
| if (emptyFolder(filePath) != RDK_API_SUCCESS) { | ||
| COMMONUTILITIES_ERROR("Failed to empty folder: %s\n", filePath); | ||
| } | ||
| rmdir(filePath); | ||
| } | ||
| else { | ||
| remove(filePath); | ||
| if (remove(filePath) != 0) { | ||
| closedir(dir); | ||
| return RDK_API_FAILURE; | ||
| } | ||
| } |
There was a problem hiding this comment.
The behavior change in emptyFolder is inconsistent: when remove(filePath) fails for a regular file, the function immediately returns RDK_API_FAILURE after closing the directory (lines 721-724). But when emptyFolder(filePath) fails for a subdirectory, it only logs a warning and continues (lines 715-717), and still attempts rmdir on line 718. This inconsistent error handling means a failure to delete one file aborts the entire operation, while a failure to empty a subdirectory is silently ignored. Consider making the error handling consistent — either bail out on both failures or continue on both.
| if (!strtokvalue || !string || !outValue) { | ||
| COMMONUTILITIES_ERROR("Invalid Parameters %p %p %p", strtokvalue, string, outValue); | ||
| return; | ||
| } | ||
| FILE *file = fopen(path,"r"); |
There was a problem hiding this comment.
The getStringValueFromFile function validates strtokvalue, string, and outValue for NULL but doesn't validate path before passing it to fopen. If path is NULL, fopen(NULL, "r") is undefined behavior. Add a NULL check for path along with the other parameters.
| else{ | ||
| SWLOG_ERROR("Invalid Parameters %p %p %p",strtokvalue, string, outValue); | ||
| } | ||
| COMMONUTILITIES_ERROR("file open failed %s\n",path); | ||
| } |
There was a problem hiding this comment.
The indentation of the else block for COMMONUTILITIES_ERROR("file open failed %s\n",path); is inconsistent with the surrounding code. The else clause is at the wrong indentation level compared to the if( file ) block it belongs to, making the code harder to read.
Release component with bug fix
Release 1.5.3
… is leading RDM crash (#83) * Update downloadUtil.c * Update downloadUtil.c --------- Co-authored-by: Satya Sundar Sahu <tinkusahu.com@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 48 changed files in this pull request and generated 5 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.
| if (lstat(entry->d_name, &statbuf) == -1) { | ||
| COMMONUTILITIES_ERROR("lstat failed for %s: %s\n", entry->d_name, strerror(errno)); | ||
| closedir(dp); | ||
| return 0; | ||
| } |
| /* Description: Use for clean Folder except file match with file_name and pdri_file_name. | ||
| * @param folder: Folder name | ||
| * @param file_name: File name pattern which are not to be deleted. | ||
| * @param file_name: Exact file name which is not to be deleted. | ||
| * @param pdri_file_name: Exact PDRI file name which is not to be deleted. | ||
| * @param model_name : This param is not in use in new design, but retaining this param for any future use case | ||
| * @return int : Fail RDK_API_FAILURE and Success RDK_API_SUCCESS | ||
| * */ | ||
| int eraseFolderExcePramaFile(const char *folder, const char* file_name, const char *model_num) | ||
| int eraseFolderExceParamFile(const char *folder, const char* file_name, const char* pdri_file_name, const char *model_num) | ||
| { | ||
| int ret = RDK_API_FAILURE; | ||
| DIR *folder_fd = NULL; | ||
| struct dirent *dir = NULL; | ||
| char oldfile[512]; | ||
|
|
||
| if (folder == NULL || file_name == NULL || model_num == NULL) { | ||
| SWLOG_ERROR("%s parameter is NULL\n", __FUNCTION__); | ||
| if (folder == NULL || file_name == NULL || pdri_file_name == NULL || model_num == NULL) { | ||
| COMMONUTILITIES_ERROR("%s parameter is NULL\n", __FUNCTION__); | ||
| return ret; |
| libuploadutil_la_LDFLAGS = -shared -fPIC -lrdkloggers -lpthread $(curl_LIBS) | ||
| libuploadutil_la_LDFLAGS += ${top_builddir}/dwnlutils/libdwnlutil.la ${top_builddir}/utils/libfwutils.la | ||
|
|
| char* constructed_path = construct_full_path(path, entry->d_name); | ||
| if (constructed_path) { | ||
| strncpy(out, constructed_path, strlen(constructed_path) + 1); // Copy path to out | ||
| COMMONUTILITIES_DEBUG(" findPFile : Constructed path out : %s", out); | ||
| free(constructed_path); |
| else if(fnmatch(search, entry->d_name, 0) == 0) { | ||
| if(out[*found_t]) { | ||
| strcpy(out[*found_t], path); | ||
| strcat(out[*found_t], "/"); | ||
| strcat(out[*found_t], entry->d_name); | ||
| snprintf(out[*found_t], allocate_size , "%s/%s", path, entry->d_name); | ||
| COMMONUTILITIES_INFO(" findPFileAll : Constructed path , out : %s\n", out[*found_t]); | ||
| (*found_t)++; | ||
| } |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| *httpCode_ret_status = 0; | ||
| *curl_ret_status = 33; | ||
| closeFile(pData, &prog, headerfile); | ||
| return CURLE_OK; |
Check failure
Code scanning / Coverity
Resource leak
| *httpCode_ret_status = 0; | ||
| *curl_ret_status = 33; | ||
| closeFile(pData, &prog, headerfile); | ||
| return CURLE_OK; |
Check failure
Code scanning / Coverity
Resource leak
…ontainer (#88) * Update cov_build.sh * Update uploadUtil.c * Update mtls_upload.c * Update mtls_upload.c --------- Co-authored-by: Abhinavpv28 <162570454+Abhinavpv28@users.noreply.github.com>
Release 1.5.4
Release 1.5.4
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 48 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| size_t allocate_size = path_len + strlen(entry->d_name) + 2; | ||
| if (entry->d_type == DT_DIR) { | ||
| // determinate a full path of an entry | ||
| full_path = calloc(path_len + strlen(entry->d_name) + 2, sizeof(char)); | ||
| strcpy(full_path, path); | ||
| strcat(full_path, "/"); | ||
| strcat(full_path, entry->d_name); | ||
|
|
||
| full_path = construct_full_path(path, entry->d_name); | ||
| found = findPFileAll(full_path, search, out, found_t, max_list); | ||
| } | ||
| else if(fnmatch(search, entry->d_name, 0) == 0) { | ||
| if(out[*found_t]) { | ||
| strcpy(out[*found_t], path); | ||
| strcat(out[*found_t], "/"); | ||
| strcat(out[*found_t], entry->d_name); | ||
| snprintf(out[*found_t], allocate_size , "%s/%s", path, entry->d_name); | ||
| COMMONUTILITIES_INFO(" findPFileAll : Constructed path , out : %s\n", out[*found_t]); | ||
| (*found_t)++; |
There was a problem hiding this comment.
snprintf(out[*found_t], allocate_size, ...) assumes the destination buffer is at least allocate_size bytes, but out[*found_t]'s allocation size is not known/validated here. This can overflow if callers allocate smaller buffers. Prefer passing the destination size into the API, or use a fixed max (and document/validate it) that matches how out[*found_t] is allocated.
| if(strcmp(token,string) == 0 ){ | ||
| strncpy(outValue, strtok(NULL,strtokvalue),128); | ||
| break; |
There was a problem hiding this comment.
If a matching key token is found but the line has no subsequent value token, strtok(NULL, ...) returns NULL and strncpy(outValue, NULL, 128) will dereference NULL. Guard the strtok(NULL, ...) result before copying and decide what outValue should contain on missing value (e.g., empty string + log).
| /* Step 1: Get CodeBig signed URL and authorization header */ | ||
| if (doCodeBigSigningForUpload(server_type, filepath, codebig_url, | ||
| sizeof(codebig_url), auth_header, sizeof(auth_header)) != 0) { | ||
| COMMONUTILITIES_ERROR("%s: CodeBig signing failed\n", __FUNCTION__); | ||
| return -1; | ||
| } | ||
|
|
||
| COMMONUTILITIES_INFO("%s: CodeBig URL: %s\n", __FUNCTION__, codebig_url); | ||
|
|
||
| /* Extract and store FQDN from CodeBig URL */ | ||
| char fqdn[256] = {0}; | ||
| const char *hostname_start = strstr(codebig_url, "://"); | ||
| if (hostname_start) { | ||
| hostname_start += 3; | ||
| const char *hostname_end = hostname_start; | ||
| while (*hostname_end && *hostname_end != ':' && *hostname_end != '/' && *hostname_end != '?') { | ||
| hostname_end++; | ||
| } | ||
| size_t len = hostname_end - hostname_start; | ||
| if (len > 0 && len < sizeof(fqdn)) { | ||
| strncpy(fqdn, hostname_start, len); | ||
| fqdn[len] = '\0'; | ||
| __uploadutil_set_fqdn(fqdn); | ||
| } | ||
| } | ||
|
|
||
| /* Step 2: Prepare FileUpload_t structure */ | ||
| FileUpload_t file_upload; | ||
| memset(&file_upload, 0, sizeof(FileUpload_t)); | ||
|
|
||
| char urlbuf[URL_MAX]; | ||
| char pathbuf[PATHNAME_MAX]; | ||
| strncpy(urlbuf, codebig_url, URL_MAX - 1); | ||
| urlbuf[URL_MAX - 1] = '\0'; | ||
| strncpy(pathbuf, filepath, PATHNAME_MAX - 1); | ||
| pathbuf[PATHNAME_MAX - 1] = '\0'; | ||
|
|
||
| file_upload.url = urlbuf; | ||
| file_upload.pathname = pathbuf; | ||
| file_upload.sslverify = 1; | ||
| file_upload.hashData = NULL; | ||
| file_upload.pPostFields = (char*)extra_fields; | ||
|
|
||
| /* Step 3: Perform metadata POST with CodeBig auth */ | ||
| curl_ret_code = performHttpMetadataPost(curl, &file_upload, NULL, &http_code); | ||
| *http_code_out = http_code; |
There was a problem hiding this comment.
doCodeBigSigningForUpload() returns both a signed URL and an authorization header, but auth_header is never applied to the CURL request before calling performHttpMetadataPost(). If the CodeBig service requires this header, the metadata POST will fail even with a valid signed URL. Consider extending performHttpMetadataPost/FileUpload_t to accept additional headers (e.g., curl_slist) and attach auth_header for this request.
| COMMONUTILITIES_INFO("%s: CodeBig URL: %s\n", __FUNCTION__, codebig_url); | ||
|
|
There was a problem hiding this comment.
This logs the full signed CodeBig URL, which can include sensitive query parameters/tokens. Consider redacting query strings (log only scheme/host/path) or logging at debug level with masking to avoid leaking credentials into logs.
| char postfields[512]; | ||
| postfields[0] = '\0'; | ||
| if (pfile_upload->pPostFields && pfile_upload->pPostFields[0] != '\0') { | ||
| snprintf(postfields, sizeof(postfields), "%s", pfile_upload->pPostFields); | ||
| ret_code = curl_easy_setopt(curl, CURLOPT_POSTFIELDS, postfields); | ||
| if (ret_code != CURLE_OK) { | ||
| COMMONUTILITIES_ERROR("%s: CURLOPT_POSTFIELDS failed: %s\n", | ||
| __FUNCTION__, curl_easy_strerror(ret_code)); | ||
| return (int)ret_code; | ||
| } | ||
| } else { | ||
| COMMONUTILITIES_ERROR("%s: CURLOPT_POSTFIELDS buffer empty\n", __FUNCTION__); | ||
| } |
There was a problem hiding this comment.
If pfile_upload->pPostFields is NULL/empty, the code logs an error but continues, leaving CURLOPT_POSTFIELDS potentially unset (or set to a stale value from a previous request on a reused CURL handle). This can cause incorrect requests. Consider treating empty POST fields as a hard failure (return error) or explicitly set CURLOPT_POSTFIELDS to an empty string and ensure the handle state is well-defined.
| TEST_F(CommonDeviceApiTestFixture, TestName_GetPartnerId_no_source_file) | ||
| { | ||
| int ret; | ||
| char data[32]; | ||
| EXPECT_NE(GetPartnerId(data, 7),0); | ||
| printf("Pertner ID = %s\n", data); | ||
| EXPECT_EQ(GetPartnerId(data, 7), 0); | ||
| printf("Partner ID = %s\n", data); |
There was a problem hiding this comment.
GetPartnerId() falls back to a default ("comcast") when no source file is present, so it should return a non-zero length even in the no-source-file case. This test currently expects 0, which will fail (or hide regressions if the implementation changes). Update the expectation to match the intended fallback behavior and/or explicitly clean up all potential sources before asserting (partnerId3.dat, bootstrap.ini, /tmp/partnerId.out, syscfg).
| if (chdir(dir) != 0) { | ||
| COMMONUTILITIES_ERROR("Failed to change directory to %s \n", dir); | ||
| closedir(dp); | ||
| return 0; | ||
| } | ||
| while((entry = readdir(dp)) != NULL) { | ||
| lstat(entry->d_name, &statbuf); | ||
| if (lstat(entry->d_name, &statbuf) == -1) { | ||
| COMMONUTILITIES_ERROR("lstat failed for %s: %s\n", entry->d_name, strerror(errno)); | ||
| closedir(dp); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
In the error path after successful chdir(dir), the function returns without restoring the previous working directory (e.g., when lstat() fails). This can leave the process in an unexpected CWD and break subsequent relative-path operations. Consider avoiding chdir() entirely (use full paths / fstatat/openat) or ensure you always chdir back to the original directory before any early return (e.g., save cwd fd and fchdir() in a cleanup block).
| char* constructed_path = construct_full_path(path, entry->d_name); | ||
| if (constructed_path) { | ||
| strncpy(out, constructed_path, strlen(constructed_path) + 1); // Copy path to out | ||
| COMMONUTILITIES_DEBUG(" findPFile : Constructed path out : %s", out); |
There was a problem hiding this comment.
This copies constructed_path into out using strncpy(..., strlen(constructed_path)+1), which does not prevent overflow because out’s capacity is unknown here. Consider updating the API to accept an output buffer size (and use snprintf/strlcpy-style bounded copy), or at least cap the copy to a known constant and guarantee NUL-termination.
| void getStringValueFromFile(char* path, char* strtokvalue, char* string, char* outValue){ | ||
| char lines[1024]; | ||
| char *token; | ||
| if (!strtokvalue || !string || !outValue) { | ||
| COMMONUTILITIES_ERROR("Invalid Parameters %p %p %p", strtokvalue, string, outValue); | ||
| return; | ||
| } | ||
| FILE *file = fopen(path,"r"); | ||
|
|
||
| if( (strtokvalue != NULL) && (string != NULL) && ( outValue != NULL) ){ | ||
| if( file ){ | ||
| while(fgets(lines, sizeof(lines), file)){ | ||
| token = strtok(lines, strtokvalue); | ||
|
|
||
| while(token != NULL){ | ||
| if(strcmp(token,string) == 0 ){ | ||
| strncpy(outValue, strtok(NULL,strtokvalue),128); | ||
| break; | ||
| } | ||
| token = strtok(NULL,strtokvalue); | ||
| } | ||
| if( file ){ | ||
| while(fgets(lines, sizeof(lines), file)){ |
There was a problem hiding this comment.
path is not validated before calling fopen(path, ...). If path is NULL this will crash/UB. Add path to the parameter validation at the top (and consider also validating outValue capacity / making the 128-byte copy bound explicit to the caller).
| COMMONUTILITIES_INFO("[%s:%d] MTLS cert success. cert=%s, type=%s, engine=%s\n", | ||
| __FUNCTION__, __LINE__, sec->cert_name, sec->cert_type, sec->engine); | ||
| #ifndef L2UPLOADENABLED | ||
| rdkcertselector_free(pthisCertSel); | ||
| #endif | ||
| return MTLS_CERT_FETCH_SUCCESS; |
There was a problem hiding this comment.
getCertificateForUpload() frees the cert selector handle on success when L2UPLOADENABLED is not defined. But performMetadataPostWithCertRotation() expects the selector to remain valid to call rdkcertselector_setCurlStatus() and potentially retry/rotate. Freeing it here can lead to use-after-free or NULL dereference in non-L2 builds. Consider removing the free from getCertificateForUpload() and instead free the selector only when the overall rotation workflow is finished/aborted.
No description provided.