RDK-60261:Implement software update service layer library#177
RDK-60261:Implement software update service layer library#177mkadinti wants to merge 325 commits into
Conversation
…tecture - fixing seg fault in Makefile.am
…tecture - fixing seg fault in Makefile.am
…tecture - fixing seg fault in Makefile.am
…tecture - fixing seg fault in Makefile.am
…tecture - fixing seg fault in Makefile.am
…tecture - fixing seg fault
…tecture - fixing seg fault
…tecture - fixing seg fault
…tecture - fixing seg fault
…tecture - fixing seg fault
…tecture- PR cleanup
…tecture- PR cleanup
…itecture- UT fix- flashImage update
…itecture- UT fix - compilation fixed for first two test binaries
…tecture- UT fix- failed test fix
…tecture- UT fix- failed test fix
…itecture- UT fix- partial fix for rdkfw_main_gtest test bin
…tecture- UT fix- partial fix for rdkfw_main_gtest test bin - 2
…tecture- UT fix- partial fix for rdkfw_main_gtest test bin - 3
…tecture- UT fix- partial fix for rdkfw_main_gtest test bin - 4
…tecture- UT fix- fix for rdkfw_main_gtest test bin
…itecture- L2 partial fix
…-Driven Firmware Updates- UpdateFirmware API Implementation- Coverity issue fix
There was a problem hiding this comment.
Pull request overview
This PR implements a comprehensive software update service layer library with extensive unit testing infrastructure. The implementation uses a sophisticated testing approach including fake D-Bus implementations via link-time symbol interposition, comprehensive mocks, and integration tests.
Changes:
- Added fake D-Bus implementation for unit testing without requiring a real D-Bus daemon
- Implemented comprehensive unit tests for firmware update handlers and main flow
- Created extensive mock infrastructure for external dependencies
- Enhanced logging macros and test utilities
Reviewed changes
Copilot reviewed 48 out of 50 changed files in this pull request and generated 24 comments.
Show a summary per file
| File | Description |
|---|---|
| unittest/test_dbus_fake.h | Header for fake D-Bus implementation using link-time symbol interposition |
| unittest/test_dbus_fake.cpp | Fake D-Bus implementation with 407 lines of test infrastructure |
| unittest/rdkfwupdatemgr_main_flow_gtest.cpp | Main flow unit tests (666 lines) covering trigger types, signal handling, state transitions |
| unittest/rdkFwupdateMgr_handlers_gtest.cpp | Handler tests (2711 lines) covering cache operations, download/update APIs, integration |
| unittest/dbus_handlers.cpp | D-Bus handlers tests (2293 lines) with signal emission tests |
| unittest/mocks/rdkFwupdateMgr_mock.h/cpp | Mock interface for firmware update manager dependencies |
| unittest/mocks/dbus_handlers_gmock.h/cpp | GMock classes for D-Bus handler dependencies |
| unittest/miscellaneous_mock.cpp | Updated mock implementations with new stubs |
| unittest/rdkv_cdl_log_wrapper.h | Added SWLOG_DEBUG/WARN/FATAL macros |
| unittest/device_status_helper_gtest.cpp | Added test for daemon binary process name |
| unittest/miscellaneous.h | Added file utility function declarations |
| unittest/configure.ac | Updated autoconf configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| g_task_return_boolean(task, TRUE); | ||
| // Free monitor context | ||
| if (monitor_ctx != NULL) { | ||
| g_free(monitor_ctx); // or free(monitor_ctx) depending on allocation |
There was a problem hiding this comment.
Coverity Issue - Logically dead code
Execution cannot reach this statement: "g_free(monitor_ctx);".
Medium Impact, CWE-561
DEADCODE
| "false" | ||
| ) | ||
|
|
||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| "false" | ||
| ) | ||
|
|
||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| "false" | ||
| ) | ||
|
|
||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| "false" | ||
| ) | ||
|
|
||
| update_result1 = str(result1[0] if isinstance(result1, tuple) else result1[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result1, tuple)" has identical then and else expressions: "result1[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| * The thread will free flash_ctx when it completes. Do NOT set to NULL to avoid | ||
| * false positive "resource leak" warnings from Coverity on the NULL assignment. */ | ||
| // flash_ctx is now owned by the worker thread and will be freed there | ||
| } |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Variable "flash_ctx" going out of scope leaks the storage it points to.
High Impact, CWE-404
RESOURCE_LEAK
| /* coverity[leaked_storage] - False positive: g_thread_join() already freed the GThread. | ||
| * Setting to NULL is defensive programming to prevent double-join. GLib documentation | ||
| * confirms the thread handle is consumed by g_thread_join(). */ | ||
| monitor_thread = NULL; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Overwriting "monitor_thread" in "monitor_thread = NULL" leaks the storage that "monitor_thread" points to.
High Impact, CWE-404
RESOURCE_LEAK
|
|
||
| // CRITICAL FIX: monitor_ctx is cleaned up by the thread itself in its cleanup section | ||
| // We MUST set it to NULL here to prevent double-free in error paths below | ||
| monitor_ctx = NULL; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Overwriting "monitor_ctx" in "monitor_ctx = NULL" leaks the storage that "monitor_ctx" points to.
High Impact, CWE-404
RESOURCE_LEAK
| /* coverity[leaked_storage] - False positive: g_thread_join() already freed the GThread. | ||
| * Setting to NULL is defensive programming to prevent double-join. GLib documentation | ||
| * confirms the thread handle is consumed by g_thread_join(). */ | ||
| monitor_thread = NULL; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Overwriting "monitor_thread" in "monitor_thread = NULL" leaks the storage that "monitor_thread" points to.
High Impact, CWE-404
RESOURCE_LEAK
| /* coverity[leaked_storage] - False positive: monitor_thread was already cleaned up | ||
| * at line 1303 via g_thread_join(). All paths reaching this return have already | ||
| * stopped and freed the monitor thread. */ | ||
| return result; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Variable "monitor_ctx" going out of scope leaks the storage it points to.
High Impact, CWE-404
RESOURCE_LEAK
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 52 out of 54 changed files in this pull request and generated 15 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| start_time = time.time() | ||
|
|
||
| # Provide URL explicitly (daemon reads delay from cache, but URL still required) | ||
| api.DownloadFirmware( |
There was a problem hiding this comment.
Variable result is not used.
| # Unresolvable hostname - will timeout | ||
| unresolvable_url = "https://unmockxconf:50052/featureControl/firmware.bin" | ||
|
|
||
| api.DownloadFirmware( |
There was a problem hiding this comment.
Variable result is not used.
| # The key validation is D-Bus API acceptance above | ||
| if wait_for_file("/opt/CDL/ABCD_PDRI_test.bin", timeout=15): | ||
| print("[PASS] PDRI firmware file created: /opt/CDL/ABCD_PDRI_test.bin") | ||
| else: |
There was a problem hiding this comment.
Variable file_created is not used.
| print("[INFO] File not created within timeout (may be expected with cert selector)") | ||
| print("[INFO] D-Bus API correctly accepted PDRI type - primary test objective met") | ||
|
|
||
| # Verify status file updated (if not skipped by disableStatsUpdate) |
There was a problem hiding this comment.
Variable file_created is not used.
| print("[PASS] PERIPHERAL firmware downloaded to /opt/CDL") | ||
| peripheral_found = True | ||
| elif os.path.exists("/tmp/peripheral_fw.bin"): | ||
| print("[PASS] PERIPHERAL firmware downloaded to /tmp") |
There was a problem hiding this comment.
Variable peripheral_found is not used.
| import time | ||
| import os | ||
| import json | ||
| import pytest |
There was a problem hiding this comment.
Import of 'pytest' is not used.
| print(f"[DEBUG] Content of {flash_file}: {content[:200]}") | ||
| except Exception: | ||
| print(f"[DEBUG] {flash_file} exists but cannot read (may be empty)") | ||
|
|
There was a problem hiding this comment.
Except block directly handles BaseException.
| print(f"[INFO] Progress content: {progress_content[:100]}") | ||
| except Exception: | ||
| pass | ||
| else: |
There was a problem hiding this comment.
Except block directly handles BaseException.
| print(f"[INFO] Progress content: {progress_content[:100]}") | ||
| except Exception: | ||
| pass | ||
| else: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| <policy context="default"> | ||
| <allow send_destination="org.rdkfwupdater.Service"/> | ||
| <allow send_interface="org.rdkfwupdater.Interface"/> | ||
| <allow receive_sender="org.rdkfwupdater.Service"/> | ||
| <allow receive_interface="org.rdkfwupdater.Interface"/> | ||
| </policy> |
There was a problem hiding this comment.
This D-Bus system-bus policy grants any user in the default context permission to send method calls to the org.rdkfwupdater.Service/org.rdkfwupdater.Interface service, which runs as root, without any further authentication or user/group restriction. On a multi-user or partially untrusted system, any local process can invoke privileged firmware update operations (e.g., download/update methods) on the root-owned daemon, enabling unauthorized firmware actions or device tampering. Tighten the D-Bus policy so that only trusted users/groups or a polkit-based authorization path can invoke these interfaces, rather than allowing access from the entire default context.
| "false" | ||
| ) | ||
|
|
||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| "false" | ||
| ) | ||
|
|
||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| "false" | ||
| ) | ||
|
|
||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| ) | ||
|
|
||
| # Parse result | ||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
|
|
||
| # Parse result | ||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) | ||
| update_status = str(result[1] if isinstance(result, tuple) and len(result) > 1 else result[1]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple) && (len(result) > 1)" has identical then and else expressions: "result[1]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| * The thread will free flash_ctx when it completes. Do NOT set to NULL to avoid | ||
| * false positive "resource leak" warnings from Coverity on the NULL assignment. */ | ||
| // flash_ctx is now owned by the worker thread and will be freed there | ||
| } |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Variable "flash_ctx" going out of scope leaks the storage it points to.
High Impact, CWE-404
RESOURCE_LEAK
| /* coverity[leaked_storage] - False positive: g_thread_join() already freed the GThread. | ||
| * Setting to NULL is defensive programming to prevent double-join. GLib documentation | ||
| * confirms the thread handle is consumed by g_thread_join(). */ | ||
| monitor_thread = NULL; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Overwriting "monitor_thread" in "monitor_thread = NULL" leaks the storage that "monitor_thread" points to.
High Impact, CWE-404
RESOURCE_LEAK
|
|
||
| // CRITICAL FIX: monitor_ctx is cleaned up by the thread itself in its cleanup section | ||
| // We MUST set it to NULL here to prevent double-free in error paths below | ||
| monitor_ctx = NULL; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Overwriting "monitor_ctx" in "monitor_ctx = NULL" leaks the storage that "monitor_ctx" points to.
High Impact, CWE-404
RESOURCE_LEAK
| /* coverity[leaked_storage] - False positive: g_thread_join() already freed the GThread. | ||
| * Setting to NULL is defensive programming to prevent double-join. GLib documentation | ||
| * confirms the thread handle is consumed by g_thread_join(). */ | ||
| monitor_thread = NULL; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Overwriting "monitor_thread" in "monitor_thread = NULL" leaks the storage that "monitor_thread" points to.
High Impact, CWE-404
RESOURCE_LEAK
| /* coverity[leaked_storage] - False positive: monitor_thread was already cleaned up | ||
| * at line 1303 via g_thread_join(). All paths reaching this return have already | ||
| * stopped and freed the monitor thread. */ | ||
| return result; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Variable "monitor_ctx" going out of scope leaks the storage it points to.
High Impact, CWE-404
RESOURCE_LEAK
| "false" | ||
| ) | ||
|
|
||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| "false" | ||
| ) | ||
|
|
||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| "false" | ||
| ) | ||
|
|
||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| ) | ||
|
|
||
| # Parse result | ||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
|
|
||
| # Parse result | ||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) | ||
| update_status = str(result[1] if isinstance(result, tuple) and len(result) > 1 else result[1]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple) && (len(result) > 1)" has identical then and else expressions: "result[1]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| * The thread will free flash_ctx when it completes. Do NOT set to NULL to avoid | ||
| * false positive "resource leak" warnings from Coverity on the NULL assignment. */ | ||
| // flash_ctx is now owned by the worker thread and will be freed there | ||
| } |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Variable "flash_ctx" going out of scope leaks the storage it points to.
High Impact, CWE-404
RESOURCE_LEAK
| /* coverity[leaked_storage] - False positive: g_thread_join() already freed the GThread. | ||
| * Setting to NULL is defensive programming to prevent double-join. GLib documentation | ||
| * confirms the thread handle is consumed by g_thread_join(). */ | ||
| monitor_thread = NULL; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Overwriting "monitor_thread" in "monitor_thread = NULL" leaks the storage that "monitor_thread" points to.
High Impact, CWE-404
RESOURCE_LEAK
|
|
||
| // CRITICAL FIX: monitor_ctx is cleaned up by the thread itself in its cleanup section | ||
| // We MUST set it to NULL here to prevent double-free in error paths below | ||
| monitor_ctx = NULL; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Overwriting "monitor_ctx" in "monitor_ctx = NULL" leaks the storage that "monitor_ctx" points to.
High Impact, CWE-404
RESOURCE_LEAK
| /* coverity[leaked_storage] - False positive: g_thread_join() already freed the GThread. | ||
| * Setting to NULL is defensive programming to prevent double-join. GLib documentation | ||
| * confirms the thread handle is consumed by g_thread_join(). */ | ||
| monitor_thread = NULL; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Overwriting "monitor_thread" in "monitor_thread = NULL" leaks the storage that "monitor_thread" points to.
High Impact, CWE-404
RESOURCE_LEAK
| /* coverity[leaked_storage] - False positive: monitor_thread was already cleaned up | ||
| * at line 1303 via g_thread_join(). All paths reaching this return have already | ||
| * stopped and freed the monitor thread. */ | ||
| return result; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Variable "monitor_ctx" going out of scope leaks the storage it points to.
High Impact, CWE-404
RESOURCE_LEAK
| "false" | ||
| ) | ||
|
|
||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| "false" | ||
| ) | ||
|
|
||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| "false" | ||
| ) | ||
|
|
||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| ) | ||
|
|
||
| # Parse result | ||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
|
|
||
| # Parse result | ||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) | ||
| update_status = str(result[1] if isinstance(result, tuple) and len(result) > 1 else result[1]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple) && (len(result) > 1)" has identical then and else expressions: "result[1]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| * The thread will free flash_ctx when it completes. Do NOT set to NULL to avoid | ||
| * false positive "resource leak" warnings from Coverity on the NULL assignment. */ | ||
| // flash_ctx is now owned by the worker thread and will be freed there | ||
| } |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Variable "flash_ctx" going out of scope leaks the storage it points to.
High Impact, CWE-404
RESOURCE_LEAK
| /* coverity[leaked_storage] - False positive: g_thread_join() already freed the GThread. | ||
| * Setting to NULL is defensive programming to prevent double-join. GLib documentation | ||
| * confirms the thread handle is consumed by g_thread_join(). */ | ||
| monitor_thread = NULL; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Overwriting "monitor_thread" in "monitor_thread = NULL" leaks the storage that "monitor_thread" points to.
High Impact, CWE-404
RESOURCE_LEAK
|
|
||
| // CRITICAL FIX: monitor_ctx is cleaned up by the thread itself in its cleanup section | ||
| // We MUST set it to NULL here to prevent double-free in error paths below | ||
| monitor_ctx = NULL; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Overwriting "monitor_ctx" in "monitor_ctx = NULL" leaks the storage that "monitor_ctx" points to.
High Impact, CWE-404
RESOURCE_LEAK
| /* coverity[leaked_storage] - False positive: g_thread_join() already freed the GThread. | ||
| * Setting to NULL is defensive programming to prevent double-join. GLib documentation | ||
| * confirms the thread handle is consumed by g_thread_join(). */ | ||
| monitor_thread = NULL; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Overwriting "monitor_thread" in "monitor_thread = NULL" leaks the storage that "monitor_thread" points to.
High Impact, CWE-404
RESOURCE_LEAK
| /* coverity[leaked_storage] - False positive: monitor_thread was already cleaned up | ||
| * at line 1303 via g_thread_join(). All paths reaching this return have already | ||
| * stopped and freed the monitor thread. */ | ||
| return result; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Variable "monitor_ctx" going out of scope leaks the storage it points to.
High Impact, CWE-404
RESOURCE_LEAK
| "false" | ||
| ) | ||
|
|
||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| "false" | ||
| ) | ||
|
|
||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| "false" | ||
| ) | ||
|
|
||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| ) | ||
|
|
||
| # Parse result | ||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
|
|
||
| # Parse result | ||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) | ||
| update_status = str(result[1] if isinstance(result, tuple) and len(result) > 1 else result[1]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple) && (len(result) > 1)" has identical then and else expressions: "result[1]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| * The thread will free flash_ctx when it completes. Do NOT set to NULL to avoid | ||
| * false positive "resource leak" warnings from Coverity on the NULL assignment. */ | ||
| // flash_ctx is now owned by the worker thread and will be freed there | ||
| } |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Variable "flash_ctx" going out of scope leaks the storage it points to.
High Impact, CWE-404
RESOURCE_LEAK
| /* coverity[leaked_storage] - False positive: g_thread_join() already freed the GThread. | ||
| * Setting to NULL is defensive programming to prevent double-join. GLib documentation | ||
| * confirms the thread handle is consumed by g_thread_join(). */ | ||
| monitor_thread = NULL; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Overwriting "monitor_thread" in "monitor_thread = NULL" leaks the storage that "monitor_thread" points to.
High Impact, CWE-404
RESOURCE_LEAK
|
|
||
| // CRITICAL FIX: monitor_ctx is cleaned up by the thread itself in its cleanup section | ||
| // We MUST set it to NULL here to prevent double-free in error paths below | ||
| monitor_ctx = NULL; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Overwriting "monitor_ctx" in "monitor_ctx = NULL" leaks the storage that "monitor_ctx" points to.
High Impact, CWE-404
RESOURCE_LEAK
| /* coverity[leaked_storage] - False positive: g_thread_join() already freed the GThread. | ||
| * Setting to NULL is defensive programming to prevent double-join. GLib documentation | ||
| * confirms the thread handle is consumed by g_thread_join(). */ | ||
| monitor_thread = NULL; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Overwriting "monitor_thread" in "monitor_thread = NULL" leaks the storage that "monitor_thread" points to.
High Impact, CWE-404
RESOURCE_LEAK
| /* coverity[leaked_storage] - False positive: monitor_thread was already cleaned up | ||
| * at line 1303 via g_thread_join(). All paths reaching this return have already | ||
| * stopped and freed the monitor thread. */ | ||
| return result; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Variable "monitor_ctx" going out of scope leaks the storage it points to.
High Impact, CWE-404
RESOURCE_LEAK
| "false" | ||
| ) | ||
|
|
||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| "false" | ||
| ) | ||
|
|
||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| "false" | ||
| ) | ||
|
|
||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| ) | ||
|
|
||
| # Parse result | ||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
|
|
||
| # Parse result | ||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) | ||
| update_status = str(result[1] if isinstance(result, tuple) and len(result) > 1 else result[1]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple) && (len(result) > 1)" has identical then and else expressions: "result[1]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| * The thread will free flash_ctx when it completes. Do NOT set to NULL to avoid | ||
| * false positive "resource leak" warnings from Coverity on the NULL assignment. */ | ||
| // flash_ctx is now owned by the worker thread and will be freed there | ||
| } |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Variable "flash_ctx" going out of scope leaks the storage it points to.
High Impact, CWE-404
RESOURCE_LEAK
| /* coverity[leaked_storage] - False positive: g_thread_join() already freed the GThread. | ||
| * Setting to NULL is defensive programming to prevent double-join. GLib documentation | ||
| * confirms the thread handle is consumed by g_thread_join(). */ | ||
| monitor_thread = NULL; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Overwriting "monitor_thread" in "monitor_thread = NULL" leaks the storage that "monitor_thread" points to.
High Impact, CWE-404
RESOURCE_LEAK
|
|
||
| // CRITICAL FIX: monitor_ctx is cleaned up by the thread itself in its cleanup section | ||
| // We MUST set it to NULL here to prevent double-free in error paths below | ||
| monitor_ctx = NULL; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Overwriting "monitor_ctx" in "monitor_ctx = NULL" leaks the storage that "monitor_ctx" points to.
High Impact, CWE-404
RESOURCE_LEAK
| /* coverity[leaked_storage] - False positive: g_thread_join() already freed the GThread. | ||
| * Setting to NULL is defensive programming to prevent double-join. GLib documentation | ||
| * confirms the thread handle is consumed by g_thread_join(). */ | ||
| monitor_thread = NULL; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Overwriting "monitor_thread" in "monitor_thread = NULL" leaks the storage that "monitor_thread" points to.
High Impact, CWE-404
RESOURCE_LEAK
| /* coverity[leaked_storage] - False positive: monitor_thread was already cleaned up | ||
| * at line 1303 via g_thread_join(). All paths reaching this return have already | ||
| * stopped and freed the monitor thread. */ | ||
| return result; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Variable "monitor_ctx" going out of scope leaks the storage it points to.
High Impact, CWE-404
RESOURCE_LEAK
| "false" | ||
| ) | ||
|
|
||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| "false" | ||
| ) | ||
|
|
||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| "false" | ||
| ) | ||
|
|
||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| ) | ||
|
|
||
| # Parse result | ||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
|
|
||
| # Parse result | ||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) | ||
| update_status = str(result[1] if isinstance(result, tuple) and len(result) > 1 else result[1]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple) && (len(result) > 1)" has identical then and else expressions: "result[1]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| * The thread will free flash_ctx when it completes. Do NOT set to NULL to avoid | ||
| * false positive "resource leak" warnings from Coverity on the NULL assignment. */ | ||
| // flash_ctx is now owned by the worker thread and will be freed there | ||
| } |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Variable "flash_ctx" going out of scope leaks the storage it points to.
High Impact, CWE-404
RESOURCE_LEAK
| /* coverity[leaked_storage] - False positive: g_thread_join() already freed the GThread. | ||
| * Setting to NULL is defensive programming to prevent double-join. GLib documentation | ||
| * confirms the thread handle is consumed by g_thread_join(). */ | ||
| monitor_thread = NULL; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Overwriting "monitor_thread" in "monitor_thread = NULL" leaks the storage that "monitor_thread" points to.
High Impact, CWE-404
RESOURCE_LEAK
|
|
||
| // CRITICAL FIX: monitor_ctx is cleaned up by the thread itself in its cleanup section | ||
| // We MUST set it to NULL here to prevent double-free in error paths below | ||
| monitor_ctx = NULL; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Overwriting "monitor_ctx" in "monitor_ctx = NULL" leaks the storage that "monitor_ctx" points to.
High Impact, CWE-404
RESOURCE_LEAK
| /* coverity[leaked_storage] - False positive: g_thread_join() already freed the GThread. | ||
| * Setting to NULL is defensive programming to prevent double-join. GLib documentation | ||
| * confirms the thread handle is consumed by g_thread_join(). */ | ||
| monitor_thread = NULL; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Overwriting "monitor_thread" in "monitor_thread = NULL" leaks the storage that "monitor_thread" points to.
High Impact, CWE-404
RESOURCE_LEAK
| /* coverity[leaked_storage] - False positive: monitor_thread was already cleaned up | ||
| * at line 1303 via g_thread_join(). All paths reaching this return have already | ||
| * stopped and freed the monitor thread. */ | ||
| return result; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Variable "monitor_ctx" going out of scope leaks the storage it points to.
High Impact, CWE-404
RESOURCE_LEAK
| "false" | ||
| ) | ||
|
|
||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| "false" | ||
| ) | ||
|
|
||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| "false" | ||
| ) | ||
|
|
||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| ) | ||
|
|
||
| # Parse result | ||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
|
|
||
| # Parse result | ||
| update_result = str(result[0] if isinstance(result, tuple) else result[0]) | ||
| update_status = str(result[1] if isinstance(result, tuple) and len(result) > 1 else result[1]) |
There was a problem hiding this comment.
Coverity Issue - Identical code for different branches
Ternary expression on condition "isinstance(result, tuple) && (len(result) > 1)" has identical then and else expressions: "result[1]". Should one of the expressions be modified, or the entire ternary expression replaced?
Medium Impact, CWE-398
IDENTICAL_BRANCHES
| * The thread will free flash_ctx when it completes. Do NOT set to NULL to avoid | ||
| * false positive "resource leak" warnings from Coverity on the NULL assignment. */ | ||
| // flash_ctx is now owned by the worker thread and will be freed there | ||
| } |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Variable "flash_ctx" going out of scope leaks the storage it points to.
High Impact, CWE-404
RESOURCE_LEAK
| /* coverity[leaked_storage] - False positive: g_thread_join() already freed the GThread. | ||
| * Setting to NULL is defensive programming to prevent double-join. GLib documentation | ||
| * confirms the thread handle is consumed by g_thread_join(). */ | ||
| monitor_thread = NULL; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Overwriting "monitor_thread" in "monitor_thread = NULL" leaks the storage that "monitor_thread" points to.
High Impact, CWE-404
RESOURCE_LEAK
|
|
||
| // CRITICAL FIX: monitor_ctx is cleaned up by the thread itself in its cleanup section | ||
| // We MUST set it to NULL here to prevent double-free in error paths below | ||
| monitor_ctx = NULL; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Overwriting "monitor_ctx" in "monitor_ctx = NULL" leaks the storage that "monitor_ctx" points to.
High Impact, CWE-404
RESOURCE_LEAK
| /* coverity[leaked_storage] - False positive: g_thread_join() already freed the GThread. | ||
| * Setting to NULL is defensive programming to prevent double-join. GLib documentation | ||
| * confirms the thread handle is consumed by g_thread_join(). */ | ||
| monitor_thread = NULL; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Overwriting "monitor_thread" in "monitor_thread = NULL" leaks the storage that "monitor_thread" points to.
High Impact, CWE-404
RESOURCE_LEAK
| /* coverity[leaked_storage] - False positive: monitor_thread was already cleaned up | ||
| * at line 1303 via g_thread_join(). All paths reaching this return have already | ||
| * stopped and freed the monitor thread. */ | ||
| return result; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Variable "monitor_ctx" going out of scope leaks the storage it points to.
High Impact, CWE-404
RESOURCE_LEAK
…updater into topic/RDK-60261
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 60 out of 67 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| MOCK_METHOD(int, getImageDetails, (ImageDetails_t*), ()); | ||
| MOCK_METHOD(int, createDir, (const char*), ()); | ||
| MOCK_METHOD(void, createFile, (const char*), ()); // Uncommented for common_utilities mock | ||
| MOCK_METHOD(int, createFile, (const char*), ()); // Fixed return type to int |
There was a problem hiding this comment.
The return type was changed from void to int, but the implementation in lines 182-193 still uses return values inconsistently. The function should consistently return 0 on success and -1 on failure, which it now does, but verify all call sites expect an int return.
…RegisterProcess- rdk logger fix applied
… RegisterProcess- adding rdkFwupdateMgr_client.h header
No description provided.