-
Notifications
You must be signed in to change notification settings - Fork 8
RDKB-60656 : Available memory check for firmware downloads #37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 12 commits
c4ed9e6
184f727
36a7863
15e3a5f
0f4afc2
07b547e
95221a6
e06da95
822d23b
053329a
c5bacd0
0a42e9f
d3ac215
ae2c53f
e0e54e5
4b8e767
887b8bc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,10 @@ | ||||||||||||||
| # SPDX-License-Identifier: Apache-2.0 | ||||||||||||||
|
|
||||||||||||||
| AM_CFLAGS = | ||||||||||||||
|
|
||||||||||||||
| lib_LTLIBRARIES = libfw_download_chk.la | ||||||||||||||
|
|
||||||||||||||
| libfw_download_chk_la_SOURCES = fw_download_check.c | ||||||||||||||
|
||||||||||||||
| libfw_download_chk_la_SOURCES = fw_download_check.c | |
| libfw_download_chk_la_SOURCES = fw_download_check.c | |
| include_HEADERS = fw_download_check.h |
Copilot
AI
Feb 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The library header file (fw_download_check.h) is not installed or listed in any _HEADERS variable, which means it won't be available to other components that need to use this library. If this library is intended to be used by other components (which seems likely given it's built as a shared library), add a line like 'libfw_download_chk_la_HEADERS = fw_download_check.h' and specify the installation directory.
| libfw_download_chk_la_SOURCES = fw_download_check.c | |
| libfw_download_chk_la_SOURCES = fw_download_check.c | |
| include_HEADERS = fw_download_check.h |
bunnam988 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
bunnam988 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Outdated
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing copyright and license header. All other Makefile.am files in this codebase include a standard Apache 2.0 license header. This file should include the same header for legal compliance and consistency.
Outdated
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing library dependency. The code uses t2_event_d from telemetry_busmessage_sender.h (line 157 in fw_download_check.c), but the Makefile does not link against the telemetry library. Looking at source/FwBankInfo/Makefile.am:24, the telemetry library is linked as -ltelemetry_msgsender. This library should be added to libfw_download_chk_la_LDFLAGS to avoid linker errors.
| libfw_download_chk_la_LDFLAGS = -lsyscfg -lccsp_common | |
| libfw_download_chk_la_LDFLAGS = -lsyscfg -lccsp_common -ltelemetry_msgsender |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need -ltelemetry_msgsender ?
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,171 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /* | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * If not stated otherwise in this file or this component's LICENSE file the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Check failure on line 2 in source/FwDownloadChk/fw_download_check.c
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * following copyright and licenses apply: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Copyright 2023 RDK Management | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
bunnam988 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Copyright 2023 RDK Management | |
| * Copyright 2025 RDK Management |
Check failure on line 7 in source/FwDownloadChk/fw_download_check.c
GitHub Actions / call-fossid-workflow / Fossid Annotate PR
FossID License Issue Detected
Source code with 'Apache-2.0' license found in local file 'source/FwDownloadChk/fw_download_check.c' (Match: schmidtw/rbus-rdk/0.0.0-20241009145012-bb6165daa26e, 15 lines, url: https://proxy.golang.org/github.com/schmidtw/rbus-rdk/@v/v0.0.0-20241009145012-bb6165daa26e.zip, file: utils/rbuscli/rbuscli.c)
bunnam988 marked this conversation as resolved.
Show resolved
Hide resolved
bunnam988 marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File handle leak when fopen fails: If fopen fails at line 21, fp is set to stderr (line 23), but then at line 41, the code tries to fclose(fp), which would attempt to close stderr. While most implementations ignore fclose(stderr), this is technically undefined behavior. Instead, track whether a file was successfully opened and only close it if it was, leaving stderr open.
Copilot
AI
Feb 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When fopen fails and fp is set to stderr, the function never attempts to close stderr (correctly), but it also doesn't log or track the fact that logging to file failed. This could lead to log messages being lost if stderr is redirected or closed. Consider adding a one-time warning when the log file cannot be opened, or use a different fallback mechanism.
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The custom logging implementation writes to a hardcoded file path and reimplements timestamp formatting. The comment in Makefile.am mentions "ccsp_common for logging headers" but the code doesn't use any CCSP logging facilities. Consider using the standard CCSP logging macros if they're available, which would provide consistent log formatting, log levels, and log rotation across the codebase.
| #define XCONF_LOG_PATH "/rdklogs/logs/xconf.txt.0" | |
| #define XCONF_LOG_INFO(...) xconf_log_write("INFO", __VA_ARGS__) | |
| #define XCONF_LOG_ERROR(...) xconf_log_write("ERROR", __VA_ARGS__) | |
| static void xconf_log_write(const char *level, const char *fmt, ...) | |
| { | |
| FILE *fp = fopen(XCONF_LOG_PATH, "a"); | |
| if (!fp) { | |
| fp = stderr; | |
| } | |
| time_t now = time(NULL); | |
| struct tm tm_info; | |
| localtime_r(&now, &tm_info); | |
| char ts[32]; | |
| strftime(ts, sizeof(ts), "%Y-%m-%d %H:%M:%S", &tm_info); | |
| fprintf(fp, "%s [%s] ", ts, level); | |
| va_list ap; | |
| va_start(ap, fmt); | |
| vfprintf(fp, fmt, ap); | |
| va_end(ap); | |
| if (fp != stderr) { | |
| fflush(fp); | |
| fclose(fp); | |
| } else { | |
| fflush(stderr); | |
| } | |
| } | |
| #include "ccsp_trace.h" | |
| #define XCONF_LOG_INFO(...) CcspTraceInfo((__VA_ARGS__)) | |
| #define XCONF_LOG_ERROR(...) CcspTraceError((__VA_ARGS__)) |
Copilot
AI
Feb 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When syscfg_get returns a non-zero value, it doesn't necessarily mean the buffer is invalid. The code should explicitly clear or validate the buffer content to ensure it doesn't contain partial or stale data before returning. Consider initializing or checking the buffer after a failed syscfg_get call.
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code logs potentially sensitive information (the full firmware URL) to a log file. Depending on the deployment, URLs might contain authentication tokens, API keys, or other sensitive information in query parameters. Consider redacting or sanitizing the URL before logging, or at minimum documenting that xconf_url should not contain sensitive credentials.
| XCONF_LOG_INFO("[FWCHK] xconf_url: %s\n", url); | |
| XCONF_LOG_INFO("[FWCHK] xconf_url retrieved from syscfg\n"); |
bunnam988 marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent spacing in syscfg_get calls. Line 60 has no space after the comma in syscfg_get(NULL,"xconf_url",url, sizeof(url)), while line 128 has a space: syscfg_get(NULL, "FwDwld_AvlMem_RsrvThreshold", buf, sizeof(buf)). For consistency with the rest of the function, add a space after NULL on lines 60 and 66.
| if(syscfg_get(NULL,"xconf_url",url, sizeof(url)) != 0){ | |
| XCONF_LOG_ERROR("[FWCHK] Failed to get xconf_url\n"); | |
| return -1; | |
| } | |
| XCONF_LOG_INFO("[FWCHK] xconf_url: %s\n", url); | |
| if(syscfg_get(NULL,"fw_to_upgrade",fname,sizeof(fname)) != 0){ | |
| if(syscfg_get(NULL, "xconf_url",url, sizeof(url)) != 0){ | |
| XCONF_LOG_ERROR("[FWCHK] Failed to get xconf_url\n"); | |
| return -1; | |
| } | |
| XCONF_LOG_INFO("[FWCHK] xconf_url: %s\n", url); | |
| if(syscfg_get(NULL, "fw_to_upgrade",fname,sizeof(fname)) != 0){ |
bunnam988 marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After retrieving syscfg values, the code doesn't validate that they are non-empty. An empty 'url' or 'fname' would result in a malformed curl command. The code should check that these strings have non-zero length after syscfg_get succeeds, before using them in the curl command construction.
| } | |
| XCONF_LOG_INFO("[FWCHK] xconf_url: %s\n", url); | |
| if(syscfg_get(NULL,"fw_to_upgrade",fname,sizeof(fname)) != 0){ | |
| XCONF_LOG_ERROR("[FWCHK] Failed to get fw_to_upgrade\n"); | |
| return -1; | |
| } | |
| } | |
| if (url[0] == '\0') { | |
| XCONF_LOG_ERROR("[FWCHK] xconf_url is empty\n"); | |
| return -1; | |
| } | |
| XCONF_LOG_INFO("[FWCHK] xconf_url: %s\n", url); | |
| if(syscfg_get(NULL,"fw_to_upgrade",fname,sizeof(fname)) != 0){ | |
| XCONF_LOG_ERROR("[FWCHK] Failed to get fw_to_upgrade\n"); | |
| return -1; | |
| } | |
| if (fname[0] == '\0') { | |
| XCONF_LOG_ERROR("[FWCHK] fw_to_upgrade is empty\n"); | |
| return -1; | |
| } |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The curl command uses grep with case-insensitive matching (-i) but doesn't handle multiple Content-Length headers or malformed responses. If a server sends multiple Content-Length headers (which can happen with misconfigured proxies), grep will return multiple lines, and only the first one will be read. This could lead to using an incorrect size. Consider using curl's -w option with %{size_download} or %{content_length_download} to get the value directly from curl, which handles HTTP protocol edge cases correctly.
| "curl -sI '%s%s%s' | grep -i Content-Length | awk '{print $2}'", | |
| "curl -sI -w '%%{content_length_download}\n' -o /dev/null '%s%s%s'", |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The awk command assumes the Content-Length header format is exactly "Content-Length: ", but HTTP headers can have variable whitespace. The current awk '{print $2}' assumes field 2 is the value, but if there are multiple spaces or tabs between the colon and value, this might fail. While grep -i handles case-insensitivity, consider using a more robust parsing approach or adding -F':' to awk and trimming whitespace explicitly.
| "curl -sI '%s%s%s' | grep -i Content-Length | awk '{print $2}'", | |
| "curl -sI '%s%s%s' | awk -F':' 'BEGIN{IGNORECASE=1} " | |
| "/^[[:space:]]*Content-Length[[:space:]]*:/ {" | |
| "gsub(/^[ \\t]+/, \"\", $2); " | |
| "gsub(/[ \\t\\r\\n]+$/, \"\", $2); " | |
| "print $2; exit}'", |
bunnam988 marked this conversation as resolved.
Show resolved
Hide resolved
bunnam988 marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Command injection vulnerability: The URL and filename from syscfg are used directly in a shell command without validation or sanitization. An attacker who can control these syscfg values could inject arbitrary shell commands. For example, if url contains "'; rm -rf / #", it would be executed by the shell. Consider using libcurl directly instead of shell commands, or if you must use shell commands, sanitize the inputs by validating that they only contain expected characters and escaping shell metacharacters.
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential buffer overflow in snprintf. The 'url' can be up to 1024 bytes, 'fname' can be up to 256 bytes, plus the curl command template and potential '/' separator. The total could exceed the 1500-byte 'cmd' buffer. While snprintf will truncate, this would result in a malformed command. The code should either use a larger buffer, or validate that url and fname lengths won't cause truncation.
| snprintf(cmd, sizeof(cmd), | |
| "curl -sI '%s%s%s' | grep -i Content-Length | awk '{print $2}'", | |
| url, (url[strlen(url)-1] == '/') ? "" : "/", fname); | |
| int n = snprintf(cmd, sizeof(cmd), | |
| "curl -sI '%s%s%s' | grep -i Content-Length | awk '{print $2}'", | |
| url, (url[strlen(url)-1] == '/') ? "" : "/", fname); | |
| if (n < 0 || (size_t)n >= sizeof(cmd)) { | |
| XCONF_LOG_ERROR("[FWCHK] Command buffer too small for curl request\n"); | |
| return -1; | |
| } |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The URL construction logic checks if url ends with '/' to avoid duplication, but this doesn't account for the case where fname starts with '/'. If both url ends with '/' and fname starts with '/', the ternary will add an empty string but fname still starts with '/', resulting in a double slash "//". While most HTTP servers handle this, it's cleaner to also check if fname[0] == '/' to ensure proper URL formation.
| url, (url[strlen(url)-1] == '/') ? "" : "/", fname); | |
| url, | |
| ((url[0] != '\0') && (url[strlen(url) - 1] == '/' || fname[0] == '/')) ? "" : "/", | |
| (fname[0] == '/') ? (fname + 1) : fname); |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The curl command is constructed using unsanitized user input from syscfg variables "xconf_url" and "fw_to_upgrade". These values are directly interpolated into a shell command executed via popen(), which creates a command injection vulnerability. If these syscfg values contain shell metacharacters (such as ';', '|', '$', backticks, etc.), an attacker could execute arbitrary commands.
Consider using libcurl's API directly instead of shelling out to curl. This would eliminate the shell injection risk and provide better error handling. Alternatively, if you must use popen, implement strict input validation to ensure the URL and filename contain only expected characters.
bunnam988 marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical security vulnerability: Command injection risk. The 'url' and 'fname' variables from syscfg are directly interpolated into a shell command without any sanitization or validation. An attacker who can control these syscfg values could execute arbitrary commands. For example, if fname contains '; rm -rf /', this would be executed.
The code should either:
- Use libcurl API instead of shell commands to make HTTP requests securely
- If shell commands must be used, employ proper escaping/quoting and validate that the inputs contain only allowed characters
- Use v_secure_popen from the secure_wrapper library (as seen in bridge_utils code at source/bridge_utils/bridge_utils_bin/bridge_util.c:433) which provides some command injection protection
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using plain popen is a security risk. The codebase uses v_secure_popen from the secure_wrapper library in other places (e.g., source/LTime/LTime.c:72, source/bridge_utils/bridge_utils_bin/bridge_util.c:433) for safer command execution. This code should follow the same pattern and use v_secure_popen instead of popen.
bunnam988 marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing error handling for curl failures: The code only checks if fgets returns NULL but doesn't verify the exit status of the popen command. If curl fails (network error, DNS failure, authentication required, etc.), the command may return an empty line or error message instead of a Content-Length value. The code would then parse this as 0 and incorrectly report "Invalid Content-Length". Check the return value of pclose() to distinguish between curl failures and missing Content-Length headers, and provide appropriate error messages for each case.
| XCONF_LOG_ERROR("[FWCHK] Content-Length not found\n"); | |
| pclose(fp); | |
| return 0; | |
| } | |
| pclose(fp); | |
| int status = pclose(fp); | |
| if (status == -1) { | |
| XCONF_LOG_ERROR("[FWCHK] pclose() failed after curl\n"); | |
| } else if (status != 0) { | |
| XCONF_LOG_ERROR("[FWCHK] curl command failed with status %d\n", status); | |
| } else { | |
| XCONF_LOG_ERROR("[FWCHK] Content-Length not found\n"); | |
| } | |
| return 0; | |
| } | |
| int status = pclose(fp); | |
| if (status == -1) { | |
| XCONF_LOG_ERROR("[FWCHK] pclose() failed after curl\n"); | |
| return 0; | |
| } else if (status != 0) { | |
| XCONF_LOG_ERROR("[FWCHK] curl command failed with status %d\n", status); | |
| return 0; | |
| } |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The curl command output may contain a trailing newline or carriage return characters (e.g., "\r\n" from HTTP headers). The code passes the raw line to strtoull, which will stop at the first non-digit character. While this works, it would be more robust to explicitly strip whitespace from the line before parsing to avoid potential issues with different line endings or extra whitespace.
| /* Strip trailing newline and carriage return characters before parsing */ | |
| line[strcspn(line, "\r\n")] = '\0'; |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making an HTTP HEAD request via curl for every firmware download check could have performance and reliability implications. If the remote server is slow or unreachable, this will block the calling code. Consider:
- Adding a timeout to the curl command (e.g., --max-time or --connect-timeout)
- Documenting the expected latency of this operation
- If this function is called frequently, consider caching the Content-Length value with a reasonable TTL
Additionally, relying on an external network call makes this function's behavior dependent on network conditions, which may not be desirable for a pre-download check.
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Content-Length value extracted from the HTTP response may contain a trailing carriage return (\r) or newline (\n) from the HTTP headers, which could affect the strtoull() parsing. While strtoull() typically stops at non-digit characters, explicitly stripping trailing whitespace/CR/LF before parsing would make the code more robust and ensure consistent behavior.
Outdated
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check 'if (bytes == 0)' will also trigger if strtoull fails to parse the input. However, this could also legitimately happen if the Content-Length header is actually "0". A more precise error check would use the endptr parameter of strtoull to verify that parsing succeeded and consumed the expected input.
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check for avail_kb == 0 is used to detect both parsing failure and the case where MemAvailable is not found in /proc/meminfo. However, this conflates two different error conditions. If the line "MemAvailable: 0 kB" exists (theoretically possible on a severely memory-constrained system), the code would incorrectly report it as "not found" rather than recognizing the actual zero value. Consider using a flag variable to track whether the MemAvailable line was found, separate from checking if the value is zero.
| while (fgets(line, sizeof(line), fp)) { | |
| if (strncmp(line, "MemAvailable:", 13) == 0) { | |
| char *p = line + 13; | |
| while (*p && !isdigit((unsigned char)*p)) ++p; | |
| avail_kb = strtoull(p, NULL, 10); | |
| break; | |
| } | |
| } | |
| fclose(fp); | |
| if (avail_kb == 0) { | |
| int memavail_found = 0; | |
| while (fgets(line, sizeof(line), fp)) { | |
| if (strncmp(line, "MemAvailable:", 13) == 0) { | |
| char *p = line + 13; | |
| while (*p && !isdigit((unsigned char)*p)) ++p; | |
| avail_kb = strtoull(p, NULL, 10); | |
| memavail_found = 1; | |
| break; | |
| } | |
| } | |
| fclose(fp); | |
| if (!memavail_found) { |
bunnam988 marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using atoi() to parse numeric values from syscfg is problematic because atoi() cannot distinguish between a valid "0" value and a parsing error. If the syscfg value contains invalid data, atoi() will silently return 0, which may be a valid threshold value. Consider using strtoul() or strtoull() with proper error checking (checking errno and ensuring the entire string was consumed) to detect parsing failures, similar to how Content-Length is parsed on line 153.
bunnam988 marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using atoi for parsing configuration values is problematic because it returns 0 on error, which is indistinguishable from a valid value of "0". If the syscfg value is invalid or empty, atoi will silently return 0 without any error indication. Consider using strtol or strtoul with proper error checking (via endptr) to detect parsing failures, similar to how strtoull is used on line 110.
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using atoi() to parse numeric values from syscfg is problematic because atoi() cannot distinguish between a valid "0" value and a parsing error. If the syscfg value contains invalid data, atoi() will silently return 0, which may be a valid percentage value. Consider using strtol() with proper error checking (checking errno and ensuring the entire string was consumed) to detect parsing failures, similar to how Content-Length is parsed on line 153.
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing input validation for percentage: The code checks if imgp_pct is negative but doesn't validate the upper bound. If FwDwld_ImageProcMemPercent is set to an unreasonably large value (e.g., 10000), it could cause the required memory calculation to overflow or produce nonsensical results. Add validation to ensure imgp_pct is within a reasonable range (0-100 or 0-200 if overhead can exceed 100%).
| if (imgp_pct < 0) imgp_pct = 0; | |
| if (imgp_pct < 0) { | |
| XCONF_LOG_ERROR("[FWCHK] Invalid FwDwld_ImageProcMemPercent '%s' (negative), clamping to 0\n", buf); | |
| imgp_pct = 0; | |
| } else if (imgp_pct > 200) { | |
| XCONF_LOG_ERROR("[FWCHK] Invalid FwDwld_ImageProcMemPercent '%s' (greater than 200), clamping to 200\n", buf); | |
| imgp_pct = 200; | |
| } |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing validation of syscfg values. The FwDwld_AvlMem_RsrvThreshold and FwDwld_ImageProcMemPercent values from syscfg are converted using atoi without validation. If these syscfg values are missing, empty, or contain non-numeric data, atoi will return 0, which could lead to incorrect memory calculations. Consider validating that the buffers are not empty and contain valid numeric values before conversion.
bunnam988 marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The calculation of img_proc_kb could theoretically overflow if fw_kb is very large (close to UINT64_MAX). While unlikely in practice for firmware sizes, adding the "+99ULL" before division could cause overflow. Consider checking fw_kb bounds or restructuring the calculation to avoid potential overflow, for example: "uint64_t img_proc_kb = (fw_kb / 100ULL) * (uint64_t)imgp_pct + ((fw_kb % 100ULL) * (uint64_t)imgp_pct + 99ULL) / 100ULL;".
| uint64_t img_proc_kb = (fw_kb * (uint64_t)imgp_pct + 99ULL) / 100ULL; | |
| uint64_t img_proc_kb = (fw_kb / 100ULL) * (uint64_t)imgp_pct | |
| + ((fw_kb % 100ULL) * (uint64_t)imgp_pct + 99ULL) / 100ULL; |
bunnam988 marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Integer overflow risk in memory calculation. When calculating 'img_proc_kb = (fw_kb * imgp_pct + 99) / 100', if fw_kb is very large (e.g., > UINT64_MAX/100), the multiplication could overflow. While this is unlikely with realistic firmware sizes and percentages, the code should either validate the inputs are within reasonable bounds or use overflow-safe arithmetic.
| uint64_t img_proc_kb = (fw_kb * (uint64_t)imgp_pct + 99ULL) / 100ULL; | |
| uint64_t required_kb = fw_kb + rsrv_kb + img_proc_kb; | |
| uint64_t img_proc_kb = 0; | |
| if (imgp_pct > 0) { | |
| uint64_t imgp_pct_u = (uint64_t)imgp_pct; | |
| /* Check for overflow in fw_kb * imgp_pct_u */ | |
| if (fw_kb > UINT64_MAX / imgp_pct_u) { | |
| XCONF_LOG_ERROR("[FWCHK] Overflow in image processing memory multiplication\n"); | |
| return -1; | |
| } | |
| uint64_t prod = fw_kb * imgp_pct_u; | |
| /* Check for overflow in prod + 99ULL */ | |
| if (prod > UINT64_MAX - 99ULL) { | |
| XCONF_LOG_ERROR("[FWCHK] Overflow in image processing memory addition\n"); | |
| return -1; | |
| } | |
| img_proc_kb = (prod + 99ULL) / 100ULL; | |
| } | |
| /* Calculate required_kb = fw_kb + rsrv_kb + img_proc_kb with overflow checks */ | |
| uint64_t required_kb = 0; | |
| if (fw_kb > UINT64_MAX - rsrv_kb) { | |
| XCONF_LOG_ERROR("[FWCHK] Overflow in required memory calculation (fw_kb + rsrv_kb)\n"); | |
| return -1; | |
| } | |
| uint64_t tmp_required = fw_kb + rsrv_kb; | |
| if (tmp_required > UINT64_MAX - img_proc_kb) { | |
| XCONF_LOG_ERROR("[FWCHK] Overflow in required memory calculation (sum + img_proc_kb)\n"); | |
| return -1; | |
| } | |
| required_kb = tmp_required + img_proc_kb; |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Integer overflow risk in memory calculation. When calculating 'required_kb = fw_kb + rsrv_kb + img_proc_kb', if the sum exceeds UINT64_MAX, it will overflow. While unlikely with typical values, the code should validate that the addition doesn't overflow, especially since this calculation determines whether a firmware download proceeds.
bunnam988 marked this conversation as resolved.
Show resolved
Hide resolved
bunnam988 marked this conversation as resolved.
Show resolved
Hide resolved
bunnam988 marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing copyright and license header. All other source files in this codebase include a standard Apache 2.0 license header at the beginning. This file should include the same header for legal compliance and consistency.
Copilot
AI
Feb 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new firmware download check functionality lacks test coverage. The repository has a comprehensive testing infrastructure (using gtest) with extensive tests for bridge_utils. The can_proceed_fw_download function has complex logic including syscfg interactions, shell command execution, file parsing, and memory calculations that should be tested. Consider adding unit tests for this module to cover success cases, error cases (missing syscfg values, curl failures, invalid Content-Length, missing MemAvailable), and edge cases (boundary conditions for memory calculations).
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,24 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /* | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * If not stated otherwise in this file or this component's LICENSE file the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Check failure on line 2 in source/FwDownloadChk/fw_download_check.h
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * following copyright and licenses apply: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Copyright 2023 RDK Management | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
bunnam988 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * you may not use this file except in compliance with the License. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * You may obtain a copy of the License at | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Unless required by applicable law or agreed to in writing, software | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * See the License for the specific language governing permissions and | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * limitations under the License. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #ifndef FW_DOWNLOAD_CHECK_H | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
bunnam988 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #define FW_DOWNLOAD_CHECK_H | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | |
| * Checks whether a firmware download can safely proceed. | |
| * | |
| * This function evaluates whether the system meets the necessary | |
| * conditions to start a firmware download (for example, sufficient | |
| * available memory or other resource constraints). | |
| * | |
| * Return value: | |
| * 1 - The firmware download may proceed. | |
| * 0 - The firmware download must be blocked (e.g. insufficient memory). | |
| * -1 - An error occurred while performing the check. | |
| */ |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function lacks documentation. Following the pattern seen in other parts of the codebase, public API functions should have comments explaining their purpose, parameters, return values, and any side effects. This is especially important for a function that will be called by other components to make critical decisions about firmware downloads.
| /** | |
| * @brief Determine whether a firmware download is allowed to proceed. | |
| * | |
| * This function is part of the firmware download decision logic and is | |
| * intended to be called by other components before initiating a firmware | |
| * download. The implementation may consult device state, configuration, | |
| * or policy rules to decide whether the download should continue. | |
| * | |
| * @return An integer status code indicating whether a firmware download | |
| * may proceed. The exact semantics of the return value (for example, | |
| * which values represent "allowed" versus "denied") are defined by | |
| * the implementation in the corresponding source file. | |
| * | |
| * @note This function may have side effects depending on the implementation | |
| * (such as logging or updating internal state). Callers should consult | |
| * the implementation details if such effects are relevant. | |
| */ |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function return values are inconsistent and potentially confusing. It returns -1 on error, 0 to block download, and 1 to proceed. This mixes error codes with boolean-like success/failure values in a non-standard way. A clearer API design would be to either:
- Return 0 for success with an out parameter indicating whether to proceed, and -1 for errors
- Return distinct positive values for different outcomes (e.g., 1 = proceed, 2 = block) and negative for errors
- Use an enum for return values to make the meaning explicit
The current design makes it unclear whether callers should treat -1 the same as 0 (both prevent download) or differently.
| int can_proceed_fw_download(void); | |
| typedef enum { | |
| FW_DOWNLOAD_CHECK_ERROR = -1, | |
| FW_DOWNLOAD_CHECK_BLOCK = 0, | |
| FW_DOWNLOAD_CHECK_PROCEED = 1 | |
| } fw_download_check_result_t; | |
| fw_download_check_result_t can_proceed_fw_download(void); |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing copyright and license header. All other header files in this codebase include a standard Apache 2.0 license header. This file should include the same header for legal compliance and consistency.
Uh oh!
There was an error while loading. Please reload this page.