RDKB-64112: Account ID check removal from IDM by veeraputhiran-thangavel · Pull Request #7 · rdkcentral/secure-upnp

veeraputhiran-thangavel · 2026-03-26T12:19:14Z

Reason for change: Remove all accountId-based pairing restrictions so devices pair irrespective of accountId
Test Procedure:
Risks: Low
Coverage:

Signed-off-by:Veeraputhiran_Thangavel@comcast.com

Remove all accountId-based pairing restrictions so devices pair irrespective of accountId value when both run new code, and work correctly with valid accountId in mixed scenarios. Changes: - Remove while(1) blocking loop in start_discovery that waited for RFC accountId to become non-Unknown (XLE could never start discovery when RFC was not yet provisioned) - Remove digit-only validation that rejected accountIds containing non-digit characters (rejected 'Unknown') - Remove accountId match check that required peer accountId to equal own accountId (prevented XLE from being added when it returned Unknown) - Remove now-unused static accountId[] global declaration idm_server.c is unchanged. Coverage: XLE new + XB new : works for valid and Unknown accountId XLE new + XB old : works when XLE RFC accountId is valid XLE old + XB new : works when XLE RFC accountId is valid

Copilot

Pull request overview

This PR aims to remove accountId-based pairing restrictions in the IDM client so discovered devices can be associated regardless of accountId value/content.

Changes:

Removed the local accountId retrieval/wait loop during discovery startup.
Simplified device association logic to no longer validate/compare accountId before inserting devices into the discovered list.

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

Copilot · 2026-03-26T12:22:31Z

                    if ( processStringRequest((GUPnPServiceProxy *)gwydata->sproxy_i, "GetAccountId","AccountId", &temp, FALSE))
                    {
-                        int valid_account=1,loop=0;
                        g_message("Discovered device sent accountId as %s",temp);
-                        for(loop=0;loop<(int)(strlen(temp));loop++)
-                        {
-                            if(temp[loop] < '0' || temp[loop] > '9')
-                            {
-                                g_message("not a valid account due to %c presence",temp[loop]);
-                                valid_account=0;
-                                break;
-                            }
-                        }
-                        if(valid_account==1)
-                        {
-                            g_message("Discovered device AccountId is valid");
-                            if(g_strcmp0(g_strstrip(accountId),temp)==0)
-                            {
-                                g_mutex_lock(mutex);
-                                xdevlist = g_list_insert_sorted_with_data(xdevlist, gwydata,(GCompareDataFunc)g_list_compare_sno, NULL);
-                                g_mutex_unlock(mutex);
-                                g_message("Inserted new/updated device %s in the list as accountId %s is same", sno,temp);
-                                callback(&di,1,1);
-                            }
-                            else
-                            {
-                                g_message("Not adding to the list as accountId %s is different",temp);
-                            }
-                        }
-                        else
-                        {
-                            g_message("Its not valid accountID so accountId %s not adding to the list",temp);
-                        }
+                        g_mutex_lock(mutex);
+                        xdevlist = g_list_insert_sorted_with_data(xdevlist, gwydata,(GCompareDataFunc)g_list_compare_sno, NULL);


Device insertion/callback is still gated by the success of the "GetAccountId" action (the list insertion and callback happen only inside the processStringRequest("GetAccountId", ...) block). This means devices that return an error / don't implement GetAccountId will never be associated, which conflicts with the PR goal of removing accountId-based pairing restrictions. Consider inserting the device (and firing the callback) regardless of GetAccountId success, and treat accountId as optional metadata (log it when available).

Copilot · 2026-03-26T12:22:32Z

                    if ( processStringRequest((GUPnPServiceProxy *)gwydata->sproxy_i, "GetAccountId","AccountId", &temp, FALSE))
                    {
-                        int valid_account=1,loop=0;
                        g_message("Discovered device sent accountId as %s",temp);
-                        for(loop=0;loop<(int)(strlen(temp));loop++)
-                        {
-                            if(temp[loop] < '0' || temp[loop] > '9')
-                            {
-                                g_message("not a valid account due to %c presence",temp[loop]);
-                                valid_account=0;
-                                break;
-                            }
-                        }
-                        if(valid_account==1)
-                        {
-                            g_message("Discovered device AccountId is valid");
-                            if(g_strcmp0(g_strstrip(accountId),temp)==0)
-                            {
-                                g_mutex_lock(mutex);
-                                xdevlist = g_list_insert_sorted_with_data(xdevlist, gwydata,(GCompareDataFunc)g_list_compare_sno, NULL);
-                                g_mutex_unlock(mutex);
-                                g_message("Inserted new/updated device %s in the list as accountId %s is same", sno,temp);
-                                callback(&di,1,1);
-                            }
-                            else
-                            {
-                                g_message("Not adding to the list as accountId %s is different",temp);
-                            }
-                        }
-                        else
-                        {
-                            g_message("Its not valid accountID so accountId %s not adding to the list",temp);
-                        }
+                        g_mutex_lock(mutex);
+                        xdevlist = g_list_insert_sorted_with_data(xdevlist, gwydata,(GCompareDataFunc)g_list_compare_sno, NULL);
+                        g_mutex_unlock(mutex);
+                        g_message("Associating device %s accountId=%s", sno,temp);
+                        callback(&di,1,1);
                        g_free(temp);
                    }


If processStringRequest("GetAccountId", ...) returns FALSE, the newly allocated gwydata is neither inserted into xdevlist nor freed, which leaks memory. When restructuring the GetAccountId handling, also ensure gwydata is freed on all failure paths (including missing UDN/receiverid).

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

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

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

Device list insertion and callback now happen regardless of whether GetAccountId succeeds, so devices that don't implement the action or return an error are still associated. When GetAccountId succeeds the accountId is logged as metadata. When it fails a diagnostic message is logged and pairing continues. This fully removes accountId-based pairing restrictions from the client side.

- New idm_log.h / idm_log.c: shared logging module for idm_client and idm_server. Writes timestamped lines to /rdklogs/logs/InterDeviceManager.txt.0 with RDK log format: YYMMDD-HH:MM:SS.uuuuuu [mod=INTERDEVICEMANAGER, lvl=<LEVEL>] [tid=NNN] func line - message - Persistent file descriptor with inode-check reopen on rdklogger rotation - Thread-safe via pthread_mutex_t; _GNU_SOURCE guard for localtime_r/syscall - IDM_LOG_INFO(fmt, ...) macro for INFO-level logging - IDM_LOG_ERR(fmt, ...) macro for ERROR-level logging - Format attribute __attribute__((format(printf, 4, 5))) for compile-time format-string validation - T2 telemetry: t2_event_s("IDM_DISCOVERY_STARTED_split", ...) and t2_event_s("IDM_DEVICE_ASSOCIATED_split", ...) at key events - Makefile.am: idm_log.c added to libupnpidm_la_SOURCES

Copilot

Pull request overview

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

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

Copilot · 2026-03-27T09:22:03Z

+    if (!idm_log_fp)
+        idm_log_fp = fopen(IDM_LOG_FILE, "a");
+    return idm_log_fp;
+}
+
+/**
+ * idm_consolelog — write one timestamped log line.
+ * Called via the IDM_LOG_INFO() macro which supplies __func__ and __LINE__.
+ */
+void idm_consolelog(const char *func, int line, const char *level,
+                    const char *fmt, ...)
+{
+    pthread_mutex_lock(&idm_log_mutex);
+    FILE *out = idm_log_reopen();
+    if (!out)
+    {
+        pthread_mutex_unlock(&idm_log_mutex);
+        return;
+    }


If fopen(IDM_LOG_FILE, "a") fails, idm_consolelog returns without emitting anything, which can silently drop important diagnostics. Consider logging the failure (including errno) via g_message/stdio fallback, and/or retrying later with backoff.

@copilot apply changes based on this feedback

Suggested change

if (!idm_log_fp)

idm_log_fp = fopen(IDM_LOG_FILE, "a");

return idm_log_fp;

}

/**

* idm_consolelog — write one timestamped log line.

* Called via the IDM_LOG_INFO() macro which supplies __func__ and __LINE__.

*/

void idm_consolelog(const char *func, int line, const char *level,

const char *fmt, ...)

{

pthread_mutex_lock(&idm_log_mutex);

FILE *out = idm_log_reopen();

if (!out)

{

pthread_mutex_unlock(&idm_log_mutex);

return;

}

if (!idm_log_fp)

idm_log_fp = fopen(IDM_LOG_FILE, "a");

return idm_log_fp;

}

/**

* idm_consolelog — write one timestamped log line.

* Called via the IDM_LOG_INFO() macro which supplies __func__ and __LINE__.

*/

void idm_consolelog(const char *func, int line, const char *level,

const char *fmt, ...)

{

pthread_mutex_lock(&idm_log_mutex);

FILE *out = idm_log_reopen();

if (!out)

{

pthread_mutex_unlock(&idm_log_mutex);

return;

}

Change IDM_LOG_FILE from /rdklogs/logs/InterDeviceManager.txt.0 to /rdklogs/logs/Consolelog.txt.0 so IDM log output is consolidated into the standard console log file.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

…sites Agent-Logs-Url: https://github.com/rdkcentral/secure-upnp/sessions/a0e5b975-4a9e-4407-8e68-8d5db314bb62 Co-authored-by: veeraputhiran-thangavel <224542127+veeraputhiran-thangavel@users.noreply.github.com>

Copilot

Pull request overview

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

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

Add IDM_LOG_ERR() alongside every existing g_message/g_critical error print so errors appear in both the GLib log (g_message) and the RDK console log (Consolelog.txt.0 via idm_log.c). Errors covered (30 call sites): - SE cert passcode retrieval failure (rdkconfig_get) - g_tls_certificate_new_from_file_with_password failure - HW cert fallback path: NULL certFile/keyFile, file not found, g_tls_certificate_new_from_files failure - Normal cert path: same three cases - GUPnP service proxy action error (processStringRequest) - gssdp_resource_browser_rescan failures (cp and cp_bgw variants) - xupnp stuck >5 minutes — all four code paths (IDM_DEBUG and production, inactive and null-count variants) - NULL cp/dproxy received in available callbacks (broadband + gateway) - Gateway receiver id NULL, UDN NULL - Memory allocation failure for GwyDeviceData - sproxy NULL on device available - Device receiver id NULL, device UDN NULL (non-broadband path) - Missing mandatory start_discovery parameters - p12 cert file creation failure - SE HW certificate not accessible - idm_server_start failure - pthread_create failure for EventHandler thread - Mandatory TLS cert files absent - GUPnP context creation failure g_message/g_critical calls are preserved unchanged.

Remove the 6 IDM_LOG_ERR calls inside the verify_devices() polling loop that fire every ~30s when gssdp is broken: - gssdp_resource_browser_rescan failed (cp and cp_bgw) - xupnp stuck >5min cp/cp_bgw inactive count threshold - xupnp stuck >5min cp/cp_bgw null count threshold These will be re-added with rate-limiting in a follow-up commit. The existing g_message calls in the same paths are left untouched. The remaining 24 IDM_LOG_ERR calls (startup / event-driven paths) carry no flooding risk and are retained.

- idm_log.c: when fopen(IDM_LOG_FILE) fails, emit a rate-limited diagnostic via stderr (once per 60 s) with errno string instead of silently dropping the message. Add <errno.h> and <string.h> includes. - idm_client.c: remove trailing \n from all 24 IDM_LOG_ERR/INFO fmt strings; idm_consolelog already appends a newline via fputc, so the extra \n was producing blank lines between log entries. Addresses Copilot review comments #1-#4 on idm_log.c/idm_log.h.

Copilot

Pull request overview

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

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

Copilot · 2026-04-14T05:43:28Z

+ * All writes go to /rdklogs/logs/Consolelog.txt.0.
+ * Log rotation is handled externally by rdklogger; the open file descriptor
+ * is transparently reopened after rotation via inode comparison.


The header comment says “All writes go to /rdklogs/logs/Consolelog.txt.0.”, but the implementation can also emit rate-limited diagnostics to stderr when fopen() fails (and otherwise drops the log line). Consider updating this comment to describe the stderr fallback / drop behavior so expectations match reality.

Suggested change

* All writes go to /rdklogs/logs/Consolelog.txt.0.

* Log rotation is handled externally by rdklogger; the open file descriptor

* is transparently reopened after rotation via inode comparison.

* Log lines are normally written to /rdklogs/logs/Consolelog.txt.0.

* Log rotation is handled externally by rdklogger; the open file descriptor

* is transparently reopened after rotation via inode comparison. If the log

* file cannot be opened, the implementation may emit rate-limited diagnostics

* to stderr; otherwise the attempted log line is dropped.

The three IDM_LOG_INFO prints: - start_discovery called for serial %s (line 927) - device associated sno=%s (BGW path, line 732) - device associated sno=%s (XB path, line 839) are redundant with CcspTraceInfo prints in rdk-interdevicemanager: - discovery start: CcspTraceInfo line 770 logs base MAC + start - pairing: CcspTraceInfo line 378 in discovery_cb logs IPv4/IPv6/MAC/ discovery_status/authentication_status for every callback() invocation Removing avoids duplicate entries in Consolelog.txt.0 and eliminates the noisy self-association entries seen in field logs (MWN2983C0544 appearing as 'device associated' alongside the real XB serial).

Copilot

Pull request overview

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

                    g_message("In available_cb_bgw gateway device receiver id is NULL");
+                    IDM_LOG_ERR("gateway device receiver id is NULL sno=%s", sno);
                }


        {
            g_message("Gateway UDN is NULL");
+            IDM_LOG_ERR("gateway UDN is NULL sno=%s", sno);
        }


    gwydata->sproxy = gupnp_device_info_get_service(GUPNP_DEVICE_INFO (dproxy), IDM_SERVICE);
    if(!gwydata->sproxy)
    {
        deviceAddNo--;
        g_message("Unable to get the services, sproxy null. returning");
+        IDM_LOG_ERR("unable to get services sproxy null sno=%s", sno ? sno : "NULL");
        return;


                else
+                {
                    g_message("Device receiver id is NULL");
+                    IDM_LOG_ERR("device receiver id is NULL sno=%s", sno);
+                }


        else
+        {
            g_message("Device UDN is NULL");
+            IDM_LOG_ERR("device UDN is NULL sno=%s", sno);
+        }


johnsonsebastiar · 2026-05-11T11:21:01Z

+#ifndef IDM_LOG_H
+#define IDM_LOG_H
+
+#define IDM_LOG_FILE "/rdklogs/logs/Consolelog.txt.0"


Instead of logging into Consolelog.txt why don't we use the existing IDM's default logfile ? So that all IDM related logs will be in a single location. Moreover, there are many modules will be logging into Consolelog.txt as well and it grows and rotates really faster.

johnsonsebastiar · 2026-05-11T11:27:17Z

-        sleep(5);
-    }
-    g_message("%s:Account ID complete.AccountId = %s", __FUNCTION__, accountId);
+    g_message("TELEMETRY_IDM_DISCOVERY_STARTED:%s", ownSerialNo->str);


Can we do a NULL check before using ownSerialNo->str?

johnsonsebastiar · 2026-05-11T11:29:40Z

            if(access(se_cert_p12, F_OK ) == -1)
            {
                g_message("Cannot create p12 cert file");
+                IDM_LOG_ERR("cannot create p12 cert file from %s", certFile);


Please remove the certFile. Don't print the file names.

johnsonsebastiar · 2026-05-11T11:35:35Z

    if(cert == NULL) {

        g_message("Certificate creation failed from cert and key files");
+        IDM_LOG_ERR("g_tls_certificate_new_from_files failed certFile=%s keyFile=%s", certFile, keyFile);


Please don't print file names.

johnsonsebastiar · 2026-05-11T11:37:04Z

+        IDM_LOG_ERR("g_tls_certificate_new_from_files failed certFile=%s keyFile=%s", certFile, keyFile);

        if(xupnp_error != NULL) {
            g_message("Failure reason for certificate creation: %s\n", xupnp_error->message );


Don't we need this error message in the log file?

1. Restore IDM_LOG_FILE to /rdklogs/logs/InterDeviceManager.txt.0 (reverts commit f4755b1 which changed it to Consolelog.txt.0). UPNP/IDM log output now lands in the same file used by the rdk-interdevicemanager component — InterDeviceManager.txt.0 — consistent with the log4crc appender for LOG.RDK.INTERDEVICEMANAGER. All rotation/permissions behaviour is unchanged (rdklogger still owns the file; idm_log_reopen() detects inode change on rotation). 2. Sanitize sensitive file-path logging in idm_client.c (5 call sites introduced by veeraputhiran commit b94fc4a): - 'certFile or keyFile not found certFile=%s keyFile=%s' (×2) - 'g_tls_certificate_new_from_files failed certFile=%s keyFile=%s' (×2) - 'cannot create p12 cert file from %s' Each now logs only the high-level failure without the actual path. 3. Remove absolute-path exposure in idm_log.c stderr fallback (introduced by commit 3971804): the rate-limited fopen-failure message no longer emits IDM_LOG_FILE in the format string. No new log files created. No functional behaviour changed.

veeraputhiran-thangavel requested a review from a team as a code owner March 26, 2026 12:19

Copilot AI review requested due to automatic review settings March 26, 2026 12:19

veeraputhiran-thangavel requested a review from a team as a code owner March 26, 2026 12:19

Copilot started reviewing on behalf of veeraputhiran-thangavel March 26, 2026 12:19 View session

Copilot AI reviewed Mar 26, 2026

View reviewed changes

Apply suggestion from @Copilot

6d07c43

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings March 26, 2026 13:34

Copilot started reviewing on behalf of veeraputhiran-thangavel March 26, 2026 13:35 View session

Copilot AI reviewed Mar 26, 2026

View reviewed changes

Comment thread src/idm_client.c Outdated

veeraputhiran-thangavel added 2 commits March 27, 2026 00:52

Copilot AI review requested due to automatic review settings March 27, 2026 09:16

Copilot started reviewing on behalf of veeraputhiran-thangavel March 27, 2026 09:16 View session

Copilot AI reviewed Mar 27, 2026

View reviewed changes

veeraputhiran-thangavel and others added 2 commits April 13, 2026 05:13

idm: redirect IDM logs to Consolelog.txt.0

f4755b1

Change IDM_LOG_FILE from /rdklogs/logs/InterDeviceManager.txt.0 to /rdklogs/logs/Consolelog.txt.0 so IDM log output is consolidated into the standard console log file.

Update src/idm_log.c

45c2ccf

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings April 13, 2026 05:17

Copilot started work on behalf of veeraputhiran-thangavel April 13, 2026 05:18 View session

Copilot started reviewing on behalf of veeraputhiran-thangavel April 13, 2026 05:18 View session

idm_log: append newline automatically in idm_consolelog; update call …

2bdb3a3

…sites Agent-Logs-Url: https://github.com/rdkcentral/secure-upnp/sessions/a0e5b975-4a9e-4407-8e68-8d5db314bb62 Co-authored-by: veeraputhiran-thangavel <224542127+veeraputhiran-thangavel@users.noreply.github.com>

Copilot finished work on behalf of veeraputhiran-thangavel April 13, 2026 05:21

Copilot AI reviewed Apr 13, 2026

View reviewed changes

Comment thread src/idm_log.c

Comment thread src/idm_log.c

Comment thread src/idm_client.c

veeraputhiran-thangavel added 3 commits April 13, 2026 11:44

Copilot AI review requested due to automatic review settings April 14, 2026 05:39

Copilot started reviewing on behalf of veeraputhiran-thangavel April 14, 2026 05:39 View session

Copilot AI reviewed Apr 14, 2026

View reviewed changes

snayak002c previously approved these changes Apr 17, 2026

View reviewed changes

Merge branch 'develop' into remove_xle_accountid_check_v3

aa90f70

Copilot AI review requested due to automatic review settings May 8, 2026 16:21

Copilot started reviewing on behalf of snayak002c May 8, 2026 16:21 View session

Copilot AI reviewed May 8, 2026

View reviewed changes

johnsonsebastiar reviewed May 11, 2026

View reviewed changes

veeraputhiran-thangavel dismissed snayak002c’s stale review via 407ce53 May 12, 2026 08:38

Conversation

veeraputhiran-thangavel commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

veeraputhiran-thangavel Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

johnsonsebastiar May 11, 2026

Choose a reason for hiding this comment

Uh oh!

johnsonsebastiar May 11, 2026

Choose a reason for hiding this comment

Uh oh!

johnsonsebastiar May 11, 2026

Choose a reason for hiding this comment

Uh oh!

johnsonsebastiar May 11, 2026

Choose a reason for hiding this comment

Uh oh!

johnsonsebastiar May 11, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

veeraputhiran-thangavel commented Mar 26, 2026 •

edited

Loading