rebase

[Abhinavpv28]

No description provided.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
6 out of 8 committers have signed the CLA.

✅ Saranya2421
✅ nhanasi
✅ AravindanNC
✅ Abhinavpv28
✅ venkat0557
✅ Alan-Ryan
❌ Saranya
❌ naveenkumarhanasi

---

Saranya seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[Copilot]

Pull request overview

Copilot reviewed 62 out of 62 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Variable node is not used.

[Copilot]

Variable subnode is not used.

[Copilot]

Variable ISSUE_FOUND is not used.

[Copilot]

Import pollutes the enclosing namespace, as the imported module helper_functions does not define 'all'.

[Copilot]

Pull request overview

Copilot reviewed 62 out of 62 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (10)

test/functional-tests/tests/test_rrd_deepsleep_static_report.py:1

• subprocess is used but never imported in this file, which will raise NameError at runtime. Add import subprocess (and any other required stdlib imports) at the top of the test module.
  test/functional-tests/tests/test_rrd_c_api_upload.py:1
• subprocess is used but not imported in this new test file, which will fail at runtime. Add import subprocess at the top of the file.
  test/functional-tests/tests/test_rrd_append_report.py:1
• This test module mutates /etc/rrd/remote_debugger.json at import time (before pytest collects/runs tests), which can cause non-deterministic failures and pollute the test environment. Move this mutation into a test setup/fixture with a clear teardown (restore the original file), or write to a test-scoped copy (e.g., under /tmp) and point the system-under-test at that copy.
  test/functional-tests/tests/deepsleep_main.c:1
• RRDEventThreadFunc loops forever and only breaks on msgrcv error, but main() unconditionally pthread_joins the thread. As written, the test harness can hang indefinitely. Add a termination mechanism (e.g., a shared atomic “stop” flag + msgrcv with timeout / non-blocking + periodic check, or cancel/detach the thread and exit cleanly after the events are injected).
  src/rrd_upload.c:1
• UploadSTBLogsParams/TRIGGER_ONDEMAND come from uploadstblogs.h, but src/rrd_upload.h excludes that include when GTEST_ENABLE is defined. If rrd_upload.c is compiled with -DGTEST_ENABLE, this will not compile. Consider providing a small local typedef/enum shim under #ifdef GTEST_ENABLE (in a C header used by rrd_upload.c), or include uploadstblogs.h unconditionally for builds that compile this file.
  src/rrd_upload.c:1
• Calling system("rm -rf %s") with an unquoted path is shell-injection prone if dir_path ever contains whitespace or shell metacharacters. Replace this with a safe recursive delete implementation (e.g., nftw + unlink/rmdir, or openat/unlinkat walking), or at minimum use an execv-style call to /bin/rm with -- and an argument vector (no shell).
  src/uploadRRDLogs.c:1
• This PR introduces a substantial new orchestration surface with multiple distinct error codes (config load, MAC/timestamp, dir validation, sanitize, archive create, upload, cleanup). Add gtest unit coverage that asserts the returned error codes and key log/behavior for the main failure paths (e.g., invalid params → 1, config failure → 3, empty dir → 6, archive failure → 10, upload failure → 11) using mocks for sysinfo/upload and temp directories for filesystem behavior.
  src/rrd_sysinfo.h:1
• The header comment says the MAC buffer needs “min 18 bytes”, but the implementation strips colons and only requires 12 hex chars + terminator (13 bytes). Update the documentation (or the implementation) so the required minimum buffer size is consistent.
  test/functional-tests/tests/power_controller.h:1
• Correct the spelling in the enum comment: 'Critial' → 'Critical'.
  test/functional-tests/features/rrd_dynamic_profile_subcategory_report.feature:1
• Fix typo in the scenario step: 'shopuld' → 'should'.

[Copilot]

When GTEST_ENABLE is defined, cmdData->rfcvalue, cmdData->command, and cmdData are no longer freed, introducing a guaranteed memory leak on the gtest path. Keep memory cleanup outside the #if !defined(GTEST_ENABLE) guard and only gate the systemctl calls.

[Copilot]

This strncpy call can produce a non-null-terminated string when truncated, and the computed bound (msgDataStringSize - strlen(RDM_PKG_SUFFIX)) can underflow if msgDataStringSize is smaller than strlen(RDM_PKG_SUFFIX). Switch to snprintf (or ensure the bound is validated and always explicitly null-terminate msgDataString).

Suggested change

	strncpy(msgDataString, paramString, (msgDataStringSize - strlen(RDM_PKG_SUFFIX)));
	{
	size_t paramLen = strlen(paramString);
	size_t suffixLen = strlen(RDM_PKG_SUFFIX);
	if (paramLen > suffixLen && msgDataStringSize > 0)
	{
	size_t baseLen = paramLen - suffixLen;
	if (baseLen >= msgDataStringSize)
	{
	baseLen = msgDataStringSize - 1;
	}
	/* Copy paramString without the RDM_PKG_SUFFIX, ensuring null termination */
	snprintf(msgDataString, msgDataStringSize, "%.*s", (int)baseLen, paramString);
	}
	else if (msgDataStringSize > 0)
	{
	msgDataString[0] = '\0';
	}
	}

[Copilot]

USTAR prefix/name splitting must occur on a directory separator boundary (and prefix should contain the directory portion). Splitting purely by character count can generate invalid headers that some tar implementations won’t extract correctly. Split on the last '/' that keeps prefix <= 155 and name <= 100, and fail cleanly if no valid split exists.

Suggested change

	if (name_len <= 100) {
	strncpy(hdr.name, name, sizeof(hdr.name) - 1);
	hdr.name[sizeof(hdr.name) - 1] = '\0';
	} else if (name_len <= 255) {
	/* split into prefix and name */
	size_t prefix_len = name_len - 100 - 1; /* leave room for null */
	if (prefix_len > sizeof(hdr.prefix)) {
	RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s] File name too long: %s (length %zu)\n",
	__FUNCTION__, name, name_len);
	return -1;
	}
	strncpy(hdr.prefix, name, prefix_len);
	hdr.prefix[prefix_len < sizeof(hdr.prefix) ? prefix_len : sizeof(hdr.prefix) - 1] = '\0';
	strncpy(hdr.name, name + prefix_len + 1, sizeof(hdr.name) - 1);
	if (name_len <= sizeof(hdr.name)) {
	strncpy(hdr.name, name, sizeof(hdr.name) - 1);
	hdr.name[sizeof(hdr.name) - 1] = '\0';
	} else if (name_len <= 255) {
	/* split into prefix and name on a directory separator boundary */
	size_t prefix_len = 0;
	const char *name_part = NULL;
	/* search for the last '/' that yields a valid prefix and name */
	for (size_t i = name_len; i > 0; --i) {
	if (name[i - 1] == '/') {
	size_t p_len = i - 1; /* directory portion, without trailing '/' */
	size_t n_len = name_len - i; /* name portion after '/' */
	if (p_len > 0 &&
	p_len <= sizeof(hdr.prefix) &&
	n_len > 0 &&
	n_len <= sizeof(hdr.name)) {
	prefix_len = p_len;
	name_part = name + i;
	break;
	}
	}
	}
	if (name_part == NULL) {
	RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s] File name too long: %s (length %zu)\n",
	__FUNCTION__, name, name_len);
	return -1;
	}
	/* copy prefix (directory part) */
	memcpy(hdr.prefix, name, prefix_len);
	hdr.prefix[prefix_len] = '\0';
	/* copy name (final path component) */
	strncpy(hdr.name, name_part, sizeof(hdr.name) - 1);

[Copilot]

Pull request overview

Copilot reviewed 63 out of 63 changed files in this pull request and generated 11 comments.

[Copilot]

This test module performs file I/O and mutates /etc/rrd/remote_debugger.json at import time. Pytest will execute this on collection, which can make test runs order-dependent and can affect other tests (and local environments) even if these tests are skipped/xfail. Move this JSON read/update/write into a dedicated test/fixture (e.g., setup function) and ensure the original file contents are restored in teardown/finalizer.

[Copilot]

SCRIPT_SUCCESS / SCRIPT_FAILURE constants are swapped: SCRIPT_SUCCESS is set to the "...upload Failed" message and SCRIPT_FAILURE to the "...upload Success" message, and the printed status strings are inverted as well. This will report success on failure and vice versa. Swap the constants (or adjust the condition/print statements) so the labels match the expected log lines.

[Copilot]

The CalledProcessError from running create_json.sh is caught and only printed, so the test will continue and potentially pass even when the script fails. Re-raise the exception or fail the test (e.g., assert False) in the except block so environment setup failures are not silently ignored.

[Copilot]

rrd_upload_cleanup_source_dir builds an "rm -rf" shell command with an unquoted dir_path and executes it via system(). Since dir_path is derived from issue/category values that ultimately come from external input, this is vulnerable to command injection and also breaks paths containing spaces/shell metacharacters. Prefer a safe recursive delete using nftw()/unlinkat() (or, at minimum, execv("rm", {"rm","-rf","--",dir_path,...}) without invoking a shell).

[Copilot]

In the GTEST_ENABLE build, this block skips freeing cmdData->rfcvalue/cmdData->command/cmdData entirely, which introduces a leak in unit tests and makes behavior differ between test and production builds. Consider guarding only the systemctl calls with GTEST_ENABLE, but keep the memory cleanup outside the conditional.

[Copilot]

The ustar name/prefix split logic for paths 101–255 chars is incorrect: it splits at an arbitrary offset (name_len - 101) rather than at the last '/' boundary. This can produce invalid tar headers and lose path information. Split the path at the last '/' such that prefix <= 155 and name <= 100 per ustar rules, or reject names that can’t be represented correctly.

Suggested change

	if (name_len <= 100) {
	strncpy(hdr.name, name, sizeof(hdr.name) - 1);
	hdr.name[sizeof(hdr.name) - 1] = '\0';
	} else if (name_len <= 255) {
	/* split into prefix and name */
	size_t prefix_len = name_len - 100 - 1; /* leave room for null */
	if (prefix_len > sizeof(hdr.prefix)) {
	RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s] File name too long: %s (length %zu)\n",
	__FUNCTION__, name, name_len);
	return -1;
	}
	strncpy(hdr.prefix, name, prefix_len);
	hdr.prefix[prefix_len < sizeof(hdr.prefix) ? prefix_len : sizeof(hdr.prefix) - 1] = '\0';
	strncpy(hdr.name, name + prefix_len + 1, sizeof(hdr.name) - 1);
	hdr.name[sizeof(hdr.name) - 1] = '\0';
	if (name_len <= sizeof(hdr.name)) {
	/* simple case: whole path fits in name field */
	strncpy(hdr.name, name, sizeof(hdr.name) - 1);
	hdr.name[sizeof(hdr.name) - 1] = '\0';
	} else if (name_len <= 255) {
	/* split into prefix and name according to POSIX ustar rules:
	* - prefix contains the directory path (no trailing '/')
	* - name contains the final path component
	* - both fields must fit in their respective header buffers
	*/
	size_t prefix_len = 0;
	size_t name_part_len = 0;
	int split_found = 0;
	/* search for last '/' that yields valid prefix and name lengths */
	for (size_t i = name_len; i > 0; --i) {
	if (name[i - 1] == '/') {
	prefix_len = i - 1; /* up to but not including this '/' */
	name_part_len = name_len - i; /* after '/' to end */
	if (prefix_len > 0 &&
	prefix_len <= sizeof(hdr.prefix) &&
	name_part_len > 0 &&
	name_part_len <= sizeof(hdr.name)) {
	split_found = 1;
	break;
	}
	}
	}
	if (!split_found) {
	RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG,
	"[%s] File name too long or invalid for ustar: %s (length %zu)\n",
	__FUNCTION__, name, name_len);
	return -1;
	}
	/* copy prefix (directory path) */
	memcpy(hdr.prefix, name, prefix_len);
	hdr.prefix[prefix_len] = '\0';
	/* copy final path component */
	memcpy(hdr.name, name + prefix_len + 1, name_part_len);
	hdr.name[name_part_len] = '\0';

[Copilot]

rrd_upload.h declares rrd_upload_invoke_logupload_api() and rrd_upload_cleanup(), but there are no implementations in the source tree. Since this header is installed as part of the public include set, this creates an API contract that will fail at link time for consumers. Either implement these functions or remove them from the public header until they exist.

[Copilot]

rrd_log.h declares rrd_log_init(), but there is no implementation in the repository. If this is intended as a public API, it should be implemented; otherwise, remove the declaration to avoid unresolved symbol/link-time failures for any consumer that includes this installed header.

[Copilot]

Typo in step text: "shopuld" should be "should".

[Copilot]

Typo in step text: "shopuld" should be "should".

[Copilot]

Pull request overview

Copilot reviewed 81 out of 81 changed files in this pull request and generated 5 comments.

[Copilot]

Pull request overview

Copilot reviewed 83 out of 84 changed files in this pull request and generated 6 comments.