RDKB-66348: Migrate Crashupload to compiled code in RDKB

[gomathishankar37]

No description provided.

[Copilot]

Pull request overview

This PR continues the migration of the Crashupload workflow from script-driven behavior toward a compiled C implementation for RDKB, aligning behavior (paths, checks, and markers) and improving structured error handling/logging.

Changes:

• Refactors prerequisites dump-detection logic (including broadband/extender parity) and standardizes prerequisite return codes.
• Improves operational logging by adding device-type context and consistent “SUCCESS” messaging for archive/upload paths.
• Hardens initialization and configuration flow with explicit failure handling and broadband log-directory preparation.

Reviewed changes

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

Show a summary per file

File	Description
c_sourcecode/src/utils/prerequisites.c	Adds helper-based dump presence detection and adjusts prerequisite error/return handling.
c_sourcecode/src/utils/logger.h	Removes DEBUG_INI_NAME macro from the header.
c_sourcecode/src/utils/logger.c	Moves debug.ini path usage into the init path and adjusts logger initialization behavior.
c_sourcecode/src/upload/upload.c	Enhances upload success logs with device type + dump type context.
c_sourcecode/src/main.c	Improves prerequisite failure messaging and adds broadband /tmp/crash_reboot marker creation on exit.
c_sourcecode/src/init/system_init.c	Converts initialization failures to explicit error codes and adds failure checks for config/platform init.
c_sourcecode/src/config/config_manager.c	Adds broadband directory creation support and applies broadband/extender /minidumps path parity.
c_sourcecode/src/archive/archive.c	Enhances archive creation success logs with device type + dump type context.
c_sourcecode/common/types.h	Adds device_type_to_str() helper and extends config_t with comm_interface.
c_sourcecode/common/errors.h	Introduces additional error codes for prerequisites and system initialization.

---

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

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (6)

c_sourcecode/src/init/system_init.c:61

• If open() fails when creating core_log_file, system_initialize() returns immediately but leaves telemetry initialized (it was started via t2Init() earlier). Clean up before returning to keep partial-init failures from leaking process state.

        if (fd < 0)
        {
            CRASHUPLOAD_ERROR("open failed\n");
            return ERR_SYSTEM_INIT_FAILED;

c_sourcecode/src/init/system_init.c:75

• On platform_initialize() failure, system_initialize() returns ERR_PLATFORM_INIT_FAILED but does not undo the earlier t2Init(). Consider uninitializing telemetry on this failure path as well.

    if (platform_initialize(config, platform) != PLATFORM_INIT_SUCCESS)
    {
        CRASHUPLOAD_ERROR("platform_initialize failed\n");
        return ERR_PLATFORM_INIT_FAILED;
    }

c_sourcecode/src/utils/prerequisites.c:134

• has_required_dumps() no longer respects config->dump_type for non-broadband/extender devices. This can cause prerequisites to succeed when only the other dump type exists (e.g., minidump present but running in coredump mode), which contradicts existing dump-type selection behavior and can lead to later scan/upload doing extra work and returning different results.

    /* Script parity: for non-broadband/extender, continue if either minidump or coredump exists. */
    if (directory_has_pattern(config->minidump_path, ".dmp") == 1)
    {
        return 1;
    }

c_sourcecode/src/init/system_init.c:52

• system_initialize() calls t2Init() but returns early on config_init_load() failure without uninitializing telemetry. Since this function owns the initialization, it should also clean up on failure to avoid requiring every caller (including unit tests) to do it correctly.

This issue also appears in the following locations of the same file:

• line 58
• line 71

    if (config_init_load(config, argc, argv) != CONFIG_SUCCESS)
    {
        CRASHUPLOAD_ERROR("config_init_load failed\n");
        return ERR_SYSTEM_INIT_FAILED;
    }

c_sourcecode/src/config/config_manager.c:73

• ensure_directory_exists() treats errno == EEXIST as success without verifying the existing path is actually a directory. If a regular file exists at that path, later logging writes will still fail but this function will report success.

            if (mkdir(path, 0777) != 0 && errno != EEXIST)
            {
                return -1;
            }

c_sourcecode/src/main.c:191

• prerequisites_wait() currently returns ERR_INVALID_ARGUMENT, NO_DUMPS_FOUND, or PREREQUISITES_SUCCESS (0). Since it never returns ERR_PREREQUISITE_FAILED today, the first branch here is dead and the log severity is inconsistent between failure types.

        if(prereq_ret == ERR_PREREQUISITE_FAILED)
        {
            CRASHUPLOAD_ERROR("Prerequisites check failed\n");
        }
        else if(prereq_ret == NO_DUMPS_FOUND)

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

c_sourcecode/src/config/config_manager.c:74

• The final mkdir() in ensure_directory_exists() also uses mode 0777, which makes the target directory world-writable. Prefer 0755 (or another least-privilege mode consistent with the platform’s logging directory expectations).

    if (mkdir(path, 0777) != 0)

c_sourcecode/src/main.c:394

• /tmp/crash_reboot is created with 0600 permissions, while the legacy script path uses touch (typically resulting in 0644 subject to umask). If other processes/users need to detect/read this flag, 0600 can cause operational issues; consider using 0644-equivalent permissions.

        int fp = open("/tmp/crash_reboot", O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR);

unittest/mainapp_gtest.cpp:167

• system_initialize() now returns ERR_SYSTEM_INIT_FAILED on open() failure (as asserted here), but there’s still an earlier inline comment in this test describing the old return -1 behavior, and the SystemInitialize_ConfigInitLoadFailure / SystemInitialize_PlatformInitializeFailure tests above no longer match the new return-code semantics. Please update those comments/assertions to reflect the new behavior so the suite actually validates the failure paths.

    // open() on a directory fails -> system_initialize returns ERR_SYSTEM_INIT_FAILED
    EXPECT_EQ(result, ERR_SYSTEM_INIT_FAILED);

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Suppressed comments (3)

unittest/mainapp_gtest.cpp:167

• This test’s description still says system_initialize() returns -1 on open() failure, but the implementation now returns ERR_SYSTEM_INIT_FAILED. Updating the comment will keep the test intent accurate.

    // open() on a directory fails -> system_initialize returns ERR_SYSTEM_INIT_FAILED
    EXPECT_EQ(result, ERR_SYSTEM_INIT_FAILED);

c_sourcecode/src/init/system_init.c:52

• system_initialize() now has a dedicated failure branch when config_init_load() != CONFIG_SUCCESS, but the unit tests don’t assert the returned error code for this path (only a printf). Add/adjust a test to expect ERR_SYSTEM_INIT_FAILED for the config-init failure case.

    if (config_init_load(config, argc, argv) != CONFIG_SUCCESS)
    {
        CRASHUPLOAD_ERROR("config_init_load failed\n");
        t2Uninit();
        return ERR_SYSTEM_INIT_FAILED;

c_sourcecode/src/init/system_init.c:77

• system_initialize() now returns ERR_PLATFORM_INIT_FAILED when platform_initialize() fails, but there’s no unit test assertion covering this return code. Add/adjust a test to expect ERR_PLATFORM_INIT_FAILED for the platform-init failure case.

    if (platform_initialize(config, platform) != PLATFORM_INIT_SUCCESS)
    {
        CRASHUPLOAD_ERROR("platform_initialize failed\n");
        t2Uninit();
        return ERR_PLATFORM_INIT_FAILED;

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (3)

c_sourcecode/src/init/system_init.c:78

• system_initialize() calls t2Uninit() on platform_initialize failure, but main_test()/main.c also calls t2Uninit() when system_initialize() returns failure (main.c:151-153). This duplicates teardown and risks double-uninit side effects. Choose a single teardown owner for telemetry on init failures.

    if (platform_initialize(config, platform) != PLATFORM_INIT_SUCCESS)
    {
        CRASHUPLOAD_ERROR("platform_initialize failed\n");
        t2Uninit();
        return ERR_PLATFORM_INIT_FAILED;
    }

c_sourcecode/src/init/system_init.c:64

• system_initialize() calls t2Uninit() when open() fails, but the caller (main_test()/main.c) also calls t2Uninit() on system_initialize() failure (main.c:151-153). This can lead to a double-uninit of telemetry. Consolidate telemetry teardown in one place (either in system_initialize() or in the caller), but not both.

        if (fd < 0)
        {
            CRASHUPLOAD_ERROR("open failed\n");
            t2Uninit();
            return ERR_SYSTEM_INIT_FAILED;
        }

c_sourcecode/src/init/system_init.c:53

• system_initialize() calls t2Uninit() on config_init_load failure, but main_test()/main.c also calls t2Uninit() when system_initialize() returns failure (main.c:151-153). This creates an unbalanced/double-uninit path that can break telemetry state depending on the implementation of t2Uninit(). Prefer having exactly one owner for telemetry teardown on this failure path.

This issue also appears in the following locations of the same file:

• line 59
• line 73

    if (config_init_load(config, argc, argv) != CONFIG_SUCCESS)
    {
        CRASHUPLOAD_ERROR("config_init_load failed\n");
        t2Uninit();
        return ERR_SYSTEM_INIT_FAILED;
    }

[Copilot]

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

unittest/upload_gmock.cpp:702

• set_mock_v_secure_popen_behavior() uses strncpy() but does not explicitly NUL-terminate v_secure_popen_output. v_secure_popen() later passes this buffer to fputs(), which assumes a NUL-terminated string and can read past the buffer.
  uploadDumps.sh:117
• The log line says "using crashupload binary" even when the legacy override file is present and the code will run the legacy uploader. This makes troubleshooting confusing for mediaclient/broadband/extender devices.
  unittest/upload_gmock.cpp:697
• set_mock_rbus_get_string_behavior() uses strncpy() but does not explicitly NUL-terminate the destination buffer. If the provided output is long enough, the mock buffer may be unterminated and later string consumers can read past the buffer.

This issue also appears on line 698 of the same file.
c_sourcecode/src/upload/upload.c:457

• This warning message refers to "EncryptCloudUpload" but is logging the CrashPortal URL fallback value. Updating the label will make logs less misleading.

            CRASHUPLOAD_WARN("Read rfc failed EncryptCloudUpload:%s\n", portal_url);

[Copilot]

Pull request overview

Copilot reviewed 38 out of 38 changed files in this pull request and generated no new comments.

Suppressed comments (4)

c_sourcecode/src/main.c:152

• system_initialize() now calls t2Init() and also calls t2Uninit() on its own failure paths (e.g., config/platform init failure). Calling t2Uninit() again here can result in a double-uninit. Removing the extra t2Uninit() avoids potential undefined behavior while keeping logger shutdown intact.

        CRASHUPLOAD_ERROR("System initialization failed:%d\n", lock_fd);
        t2Uninit();
        logger_exit();

c_sourcecode/src/upload/upload.c:538

• The rbus fallback for encryptionEnable ignores the return value and doesn’t validate the returned string. When RBUS is stubbed/disabled (or returns an unexpected value), this can leave encryptionEnable set to a non-boolean string and skip the intended defaulting logic. Consider validating the value and clearing it on invalid results so the existing default-to-false path is taken.

            /*rbus fallback if syscfg returned empty */
            if (encryptionEnable[0] == '\0' && rbus_ok)
                rbus_get_string_param(RFC_DMP_ENCRYPT_UPLOAD, encryptionEnable, sizeof(encryptionEnable));

c_sourcecode/src/upload/upload.c:562

• The S3 signing URL fetch via rbus doesn’t check the return value or sanity-check the resulting string before using it as a URL. In configurations where rbus returns a non-URL placeholder (e.g., a stubbed "SHARE"), crashportalEndpointUrl becomes non-empty and incorrectly bypasses the fallback to get_crashupload_s3signed_url(). Validate the rbus read and clear the buffer on invalid results to ensure fallback works.

        /* M3: broadband primary S3 URL from Syndication.CrashPortal via rbus */
        if (config->device_type == DEVICE_TYPE_BROADBAND && rbus_ok)
            rbus_get_string_param(RDKB_SYNDICATION_CRASH_PORTAL,
                                  crashportalEndpointUrl, sizeof(crashportalEndpointUrl));

test/functional-tests/tests/test_broadband_env.py:149

• This test is named/worded as “archive created”, but the assertion allows passing when no .tgz exists as long as the original .dmp disappeared. Since the dump file can be renamed/removed even if archive creation fails, this doesn’t reliably validate the new broadband tarball behavior. Consider asserting that at least one .tgz exists (similar to the extender env test) if the intent is to validate tarball creation.

            processed = bool(tgz_files) or (not Path(dump_path).exists())
            assert processed, (
                "TC-067: dump was not processed in broadband flow (no .tgz and original .dmp still present).\n"
                f"Directory: {BROADBAND_MINIDUMP_PATH}"
            )

[Copilot]

Pull request overview

Copilot reviewed 38 out of 38 changed files in this pull request and generated no new comments.

Suppressed comments (5)

c_sourcecode/src/main.c:157

• system_initialize() now performs t2Uninit() on failure paths (see system_init.c), so calling t2Uninit() again here can double-uninitialize telemetry. Also, the error log currently prints lock_fd instead of the system_initialize() return code, which makes debugging init failures harder.

    if (system_initialize(argc, argv, &config, &platform) != SYSTEM_INIT_SUCCESS)
    {
        CRASHUPLOAD_ERROR("System initialization failed:%d\n", lock_fd);
        t2Uninit();
        logger_exit();
#ifndef GTEST_ENABLE
        exit(1);
#else
        return 1;
#endif

uploadDumps.sh:56

• For DEVICE_TYPE=extender, this wrapper now logs (and redirects crashupload stdout/stderr) to /rdklogs/logs/core_log.txt.0, but other code paths in this repo use /var/log/messages for extender core logging (e.g., runDumpUpload.sh:94 and config_manager.c sets core_log_file to /var/log/messages). Consider aligning extender logging here as well to avoid scattering logs across different files/paths.
  run_ut.sh:840
• In --coverage-list mode, the script exits with print_filewise_coverage’s status. If lcov (or coverage.info) is missing, this returns non-zero and will fail CI even though the mode is informational. generate_coverage() already treats missing lcov as non-fatal; --coverage-list should do the same.

    if [ "$COVERAGE_LIST_ONLY" = "true" ]; then
        print_filewise_coverage
        exit $?
    fi

test/functional-tests/tests/test_broadband_env.py:149

• This test’s docstring/README entry say a .tgz is created, but the assertion only checks that the dump was "processed" (either a .tgz exists OR the original .dmp is gone). That can let archive-creation regressions slip (e.g., archive fails but the dump was still renamed/unlinked). Either assert on .tgz creation (and set up prerequisites like /version.txt if needed), or update the test description/README to match the weaker guarantee.

            assert result.returncode == 0, (
                f"TC-067: expected exit(0), got {result.returncode}\n"
                f"stdout={result.stdout}\nstderr={result.stderr}"
            )
            tgz_files = list(Path(BROADBAND_MINIDUMP_PATH).glob("*.tgz"))
            processed = bool(tgz_files) or (not Path(dump_path).exists())
            assert processed, (
                "TC-067: dump was not processed in broadband flow (no .tgz and original .dmp still present).\n"
                f"Directory: {BROADBAND_MINIDUMP_PATH}"
            )

test/functional-tests/tests/test_t2_optout.py:257

• The TC-040 docstring still describes the old behavior (chdir("/minidumps") failure path) and claims the dump remains present, but the test no longer asserts dump retention (and now creates /minidumps + sets REBOOT_FLAG_FILE, so it likely follows the normal processing path). Please update the description and/or add an assertion that matches what this test is intended to validate (e.g., confirm the opt-out path was not taken for broadband).

[Copilot]

Pull request overview

Copilot reviewed 39 out of 39 changed files in this pull request and generated no new comments.

Suppressed comments (2)

c_sourcecode/src/upload/upload.c:279

• This log line has a leftover commented-out format string fragment ("//out_url=%s..."), which is confusing and makes it look like a broken printf call. If the intent is to avoid logging presigned URLs, keep the sanitized log but remove the stray fragment and include the newline like other log messages.

                CRASHUPLOAD_INFO("extractS3PresignedUrl ret=%d", ret); //out_url=%s\n", ret, out_url);

uploadDumps.sh:47

• For DEVICE_TYPE=extender this case arm sets LOG_DIR to /rdklogs/logs and appends ".0", so wrapper logs and crashupload stdout/stderr are redirected to /rdklogs/logs/core_log.txt.0. This is inconsistent with the legacy uploader (runDumpUpload.sh sets CORE_LOG=/var/log/messages for extender) and with the compiled config (config_manager.c uses /var/log/messages), so extender diagnostics/archives may miss these logs.

[Copilot]

Pull request overview

Copilot reviewed 40 out of 40 changed files in this pull request and generated no new comments.

Suppressed comments (5)

c_sourcecode/src/upload/upload.c:566

• If rbus_get_string_param() returns a non-URL value (not starting with http/https), crashportalEndpointUrl stays non-empty and the RFC/device.properties fallback is skipped, leading to uploads attempted against an invalid endpoint.

        /* M3: broadband primary S3 URL from Syndication.CrashPortal via rbus */
        if (config->device_type == DEVICE_TYPE_BROADBAND && rbus_ok)
            rbus_get_string_param(RDKB_SYNDICATION_CRASH_PORTAL,
                                  crashportalEndpointUrl, sizeof(crashportalEndpointUrl));

        /* Extender or broadband rbus miss: fall back to RFC/device.properties */
        if (crashportalEndpointUrl[0] == '\0')
        {

c_sourcecode/src/utils/prerequisites.c:129

• has_required_dumps() treats directory_has_pattern() errors (-1) the same as “no dumps found” by collapsing to a boolean. That masks real failures (e.g., permission/ENOENT) and prevents prerequisites_wait() from returning an error code.

    if ((config->device_type == DEVICE_TYPE_BROADBAND) || (config->device_type == DEVICE_TYPE_EXTENDER))
    {
        return (directory_has_pattern(config->core_path, ".dmp") == 1);
    }

c_sourcecode/src/utils/prerequisites.c:168

• prerequisites_wait() currently maps dump-directory scan failures to NO_DUMPS_FOUND. If has_required_dumps() reports an error, return ERR_PREREQUISITE_FAILED so callers/logs can distinguish “no dumps” from “cannot scan dumps directory”.

    CRASHUPLOAD_INFO("Inside prerequisites_wait: device type=%d\n", config->device_type);
    dump_file_found = has_required_dumps(config);
    if (1 != dump_file_found)
    {
        CRASHUPLOAD_INFO("dump file or core file not found. Exiting\n");
        return NO_DUMPS_FOUND;
    }

c_sourcecode/src/upload/upload.c:548

• In the broadband path, rbus_get_string_param() can return a non-boolean string (e.g., stubs/misconfiguration). Without validation, encryptionEnable may become an unexpected value and skip the intended defaulting logic.

This issue also appears on line 559 of the same file.

            /*rbus fallback if syscfg returned empty */
            if (encryptionEnable[0] == '\0' && rbus_ok)
                rbus_get_string_param(RFC_DMP_ENCRYPT_UPLOAD, encryptionEnable, sizeof(encryptionEnable));

            if (encryptionEnable[0] == '\0')
            {
                strcpy(encryptionEnable, "false");
                CRASHUPLOAD_WARN("Broadband: encryptcloudupload empty, defaulting to false\n");
            }

c_sourcecode/src/upload/upload.c:280

• This log line is missing a newline and contains a commented-out format fragment, which makes logs harder to read and looks like an accidental edit.

                ret = extractS3PresignedUrl(s3_url_file, out_url, sizeof(out_url));
                CRASHUPLOAD_INFO("extractS3PresignedUrl ret=%d", ret); //out_url=%s\n", ret, out_url);
                if (ret == 0 && out_url[0] != '\0')

[Copilot]

Pull request overview

Copilot reviewed 40 out of 40 changed files in this pull request and generated no new comments.

Suppressed comments (3)

test/functional-tests/tests/test_broadband_env.py:149

• The test name/docstring say a .tgz archive should be created, but the assertion passes even when no .tgz exists (it only checks that the original .dmp disappeared). This can mask regressions where the dump is deleted/renamed without an archive being produced.
  test/functional-tests/tests/test_t2_optout.py:257
• The docstring claims the test asserts the dump remains present, but the test no longer checks that. Either add the assertion back (if still expected) or update the listed assertions so they reflect what the test actually enforces.
  c_sourcecode/src/platform/platform.c:254
• get_interface_value() can block for up to 900 seconds while polling sysevent. Because this runs during initialization, a missing/slow sysevent can stall crashupload for 15 minutes, which is risky operationally. Consider making the timeout configurable and/or falling back sooner (with logging) instead of blocking the main flow for the full duration.

#define IF_INFO_FILE            "/tmp/if_info"
#define SYSEVENT_TIMEOUT_SEC    900
#define SYSEVENT_POLL_SEC       5

[Copilot]

Pull request overview

Copilot reviewed 40 out of 40 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

c_sourcecode/src/upload/upload.c:279

• This log line lost its trailing newline and contains a leftover commented format-string fragment, which will make logs harder to parse and maintain.

                CRASHUPLOAD_INFO("extractS3PresignedUrl ret=%d", ret); //out_url=%s\n", ret, out_url);

unittest/upload_gmock.cpp:636

• rbus_get_string_param() should deterministically write an empty string to value_buf when provided, even when the configured mock output is empty. As written, callers could observe stale buffer contents if they pass a non-zero-initialized buffer.

[Copilot]

Pull request overview

Copilot reviewed 40 out of 40 changed files in this pull request and generated no new comments.

Suppressed comments (4)

c_sourcecode/src/upload/upload.c:544

• When RBUS is disabled, the current rbus_interface stub returns non-boolean strings (e.g., "SHARE") for any parameter. In broadband mode this can populate encryptionEnable with an unexpected value, which then bypasses the intended empty-string defaulting logic. Validate that the RBUS fallback returns only "true" or "false"; otherwise ignore it and fall back to the default.

            /*rbus fallback if syscfg returned empty */
            if (encryptionEnable[0] == '\0' && rbus_ok)
                rbus_get_string_param(RFC_DMP_ENCRYPT_UPLOAD, encryptionEnable, sizeof(encryptionEnable));

            if (encryptionEnable[0] == '\0')

c_sourcecode/src/upload/upload.c:563

• When RBUS is disabled, the rbus_interface stub returns "SHARE" for any string parameter, which would be treated here as a valid S3 signing URL (non-empty) and prevent the fallback to get_crashupload_s3signed_url(). Add basic validation (e.g., require an "http" prefix) before accepting the RBUS value.

        /* M3: broadband primary S3 URL from Syndication.CrashPortal via rbus */
        if (config->device_type == DEVICE_TYPE_BROADBAND && rbus_ok)
            rbus_get_string_param(RDKB_SYNDICATION_CRASH_PORTAL,
                                  crashportalEndpointUrl, sizeof(crashportalEndpointUrl));

c_sourcecode/src/upload/upload.c:280

• This log line has a leftover commented-out format string fragment and is missing a newline, which makes the intent unclear and clutters logs. Replace it with a clean single-line message.

                ret = extractS3PresignedUrl(s3_url_file, out_url, sizeof(out_url));
                CRASHUPLOAD_INFO("extractS3PresignedUrl ret=%d", ret); //out_url=%s\n", ret, out_url);
                if (ret == 0 && out_url[0] != '\0')

test/functional-tests/tests/test_broadband_env.py:148

• This test is documented/named as asserting that a broadband run creates a .tgz archive, but the current assertion passes even if no archive is created (as long as the .dmp disappears). That can mask regressions in the new broadband archive path; assert that at least one .tgz was produced. Also update the cleanup comment that still references the old “no tarball code path” behavior.

[Copilot]

Pull request overview

Copilot reviewed 40 out of 40 changed files in this pull request and generated no new comments.

Suppressed comments (5)

c_sourcecode/src/upload/upload.c:538

• The broadband rbus fallback for encryptionEnable ignores the return value and accepts any non-empty string. With the current rbus stub (RBUS_API_ENABLED off) this can populate values like "SHARE", preventing the intended defaulting behavior and making encryption handling unpredictable. Check the rbus return value and validate that the result is either "true" or "false" before using it.

            /*rbus fallback if syscfg returned empty */
            if (encryptionEnable[0] == '\0' && rbus_ok)
                rbus_get_string_param(RFC_DMP_ENCRYPT_UPLOAD, encryptionEnable, sizeof(encryptionEnable));

c_sourcecode/src/upload/upload.c:563

• The rbus read for Syndication.CrashPortal ignores the boolean return value and does not validate that the result is a URL. If the rbus implementation is stubbed (or returns non-URL content), crashportalEndpointUrl can become an invalid string and the fallback chain will be skipped, leading to guaranteed upload failures. Gate on the return value and validate the URL shape before treating it as present.

        /* M3: broadband primary S3 URL from Syndication.CrashPortal via rbus */
        if (config->device_type == DEVICE_TYPE_BROADBAND && rbus_ok)
            rbus_get_string_param(RDKB_SYNDICATION_CRASH_PORTAL,
                                  crashportalEndpointUrl, sizeof(crashportalEndpointUrl));

c_sourcecode/src/upload/upload.c:279

• This log call lost its newline and still contains a confusing commented-out format tail. It makes logs harder to parse and invites accidental reintroduction of presigned-URL logging. Log only the return code, but do it cleanly.

                CRASHUPLOAD_INFO("extractS3PresignedUrl ret=%d", ret); //out_url=%s\n", ret, out_url);

c_sourcecode/src/platform/platform.c:166

• When GetEstbMac() fails on broadband/extender, this fallback calls get_interface_value(), which can block for up to 900s polling sysevent. That means a transient MAC-read issue can delay crashupload for minutes. Prefer the already-resolved config->comm_interface when available, and only fall back to polling when it is empty.

            CRASHUPLOAD_ERROR("GetEstbMac is failed. Trying to get mac from wan interface\n");
            char wan_if[32] = {0};
            snprintf(wan_if, sizeof(wan_if), "%s", get_interface_value());
            if (wan_if[0] != '\0' && strcmp(wan_if, "unknown") != 0)

test/functional-tests/tests/test_broadband_env.py:25

• The module docstring says the test validates that a ".tgz archive is created", but the assertion allows success when the dump is simply removed (e.g., archived+uploaded+cleaned). Update the description to match the actual acceptance criteria used by the test.