Skip to content

RDKB-62352 : Observed telemetry2_0 crash #247

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
adrojaankur wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

RDKB-62352 : Observed telemetry2_0 crash #247

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3a0a7a2
Telemetry crashe due to Open SSL engine
Dec 19, 2025
13c9f3d
Merge pull request #246 from rdkcentral/develop
adrojaankur Jan 27, 2026
fa28348
Update curlinterface.c
adrojaankur Jan 28, 2026
a5e4202
Update curlinterface.c
adrojaankur Jan 28, 2026
6657cfe
Update curlinterface.c
adrojaankur Jan 28, 2026
26176b5
Merge pull request #252 from rdkcentral/develop
adrojaankur Feb 3, 2026
9d1e4ed
RDKB-62352 : Observed telemetry2_0 crash
yogeswaransky Feb 3, 2026
6573670
Update curlinterface.c
adrojaankur Feb 3, 2026
d9e649e
RDKB-62352 : Observed telemetry2_0 crash
yogeswaransky Feb 3, 2026
af9d118
Merge pull request #255 from rdkcentral/develop
adrojaankur Feb 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion source/protocol/http/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ libhttp_la_SOURCES = curlinterface.c
libhttp_la_LDFLAGS = -shared -fPIC -lcurl
if IS_LIBRDKCERTSEL_ENABLED
libhttp_la_CFLAGS = $(LIBRDKCERTSEL_FLAG)
libhttp_la_LDFLAGS += -lRdkCertSelector
libhttp_la_LDFLAGS += -lRdkCertSelector -lcrypto
endif
libhttp_la_CPPFLAGS = -fPIC -I${PKG_CONFIG_SYSROOT_DIR}$(includedir)/dbus-1.0 \
-I${PKG_CONFIG_SYSROOT_DIR}$(libdir)/dbus-1.0/include \
Expand Down
37 changes: 37 additions & 0 deletions source/protocol/http/curlinterface.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@
#include <sys/wait.h>
#include <curl/curl.h>
#include <signal.h>
#ifdef LIBRDKCERTSEL_BUILD
#include <openssl/crypto.h>
Copy link
Contributor

@shibu-kv shibu-kv Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't be using openssl api's directly. The libcurl apis should be the only interface.

Copy link

@pdmooneypub pdmooneypub Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shibu, at one point we were hashing the passcode so we could see if it changed (without exposing it). We may want to keep this in here until we have resolved the problem with the passcode.

#endif

#include "curlinterface.h"
#include "reportprofiles.h"
Expand Down Expand Up @@ -277,6 +280,11 @@ static void curlCertSelectorInit()
#endif
if (state_red_enable && curlRcvryCertSelector == NULL )
{
if(curlCertSelector != NULL)
{
rdkcertselector_free(&curlCertSelector);
curlCertSelector = NULL;
}
curlRcvryCertSelector = rdkcertselector_new( NULL, NULL, "RCVRY" );
if (curlRcvryCertSelector == NULL)
{
Expand Down Expand Up @@ -319,6 +327,7 @@ T2ERROR sendReportOverHTTP(char *httpUrl, char *payload, pid_t* outForkedPid)
rdkcertselectorStatus_t curlGetCertStatus;
char *pCertURI = NULL;
bool state_red_enable = false;
char *pEngine = NULL;
#endif
char *pCertFile = NULL;
char *pCertPC = NULL;
Expand Down Expand Up @@ -440,6 +449,12 @@ T2ERROR sendReportOverHTTP(char *httpUrl, char *payload, pid_t* outForkedPid)
*/
if(childPid == 0)
{
#ifdef LIBRDKCERTSEL_BUILD
if(OPENSSL_init_crypto(OPENSSL_INIT_NO_LOAD_CONFIG, NULL) != 1)
Copy link
Contributor

@shibu-kv shibu-kv Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to be the right thing to do.

Copy link
Contributor Author

@adrojaankur adrojaankur Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curl_easy_init directly will not initialize OPENSSL_init_crypto.
OPENSSL_init_crypto is not strictly mandatory, but
OPENSSL_init_crypto is strongly recommended in forked child processes.

{
fprintf(stderr, "T2:OPENSSL_init_crypto failed\n"); // avoiding T2 logger in the child.
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the child process, if OPENSSL_init_crypto(OPENSSL_INIT_NO_LOAD_CONFIG, NULL) returns a failure status, the code only prints to stderr but continues to use OpenSSL/cURL as if initialization had succeeded. If OPENSSL_init_crypto ever fails, subsequent crypto/TLS operations are undefined; consider treating this as a hard failure (e.g., set an error in childCurlResponse and jump to child_cleanReturn) so callers get a clear error signal instead of potentially crashing or misbehaving.

Suggested change
fprintf(stderr, "T2:OPENSSL_init_crypto failed\n"); // avoiding T2 logger in the child.
fprintf(stderr, "T2:OPENSSL_init_crypto failed\n"); // avoiding T2 logger in the child.
childCurlResponse.curlStatus = false;
childCurlResponse.curlResponse = CURLE_FAILED_INIT;
childCurlResponse.lineNumber = __LINE__;
goto child_cleanReturn;

Copilot uses AI. Check for mistakes.
}
#endif
curl = curl_easy_init();
if(curl)
{
Expand All @@ -455,6 +470,28 @@ T2ERROR sendReportOverHTTP(char *httpUrl, char *payload, pid_t* outForkedPid)
goto child_cleanReturn;
}
#ifdef LIBRDKCERTSEL_BUILD
pEngine = rdkcertselector_getEngine(thisCertSel);
if(pEngine != NULL )
{
code = curl_easy_setopt(curl, CURLOPT_SSLENGINE, pEngine);
if(code != CURLE_OK)
{
childCurlResponse.lineNumber = __LINE__;
curl_easy_cleanup(curl);
goto child_cleanReturn;
Comment on lines +477 to +481
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error handling in the new SSL engine configuration code doesn't set childCurlResponse.curlSetopCode before jumping to child_cleanReturn. This is inconsistent with other error paths in this function (see lines 240, 247 for comparison). Consider setting childCurlResponse.curlSetopCode = code for consistency with existing error handling patterns.

Copilot uses AI. Check for mistakes.
}
}
else
{
code = curl_easy_setopt(curl, CURLOPT_SSLENGINE_DEFAULT, 1L);
if(code != CURLE_OK )
{
Comment on lines +473 to +488
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When CURLOPT_SSLENGINE / CURLOPT_SSLENGINE_DEFAULT fail, childCurlResponse.curlSetopCode is not updated before you set lineNumber and jump to child_cleanReturn. This means the parent-side logging will report the last successful setopt result instead of the actual failing CURLcode, making diagnosis harder; consider assigning childCurlResponse.curlSetopCode = code here, consistent with other curl_easy_setopt error paths in this file.

Copilot uses AI. Check for mistakes.
childCurlResponse.lineNumber = __LINE__;
curl_easy_cleanup(curl);
goto child_cleanReturn;
Comment on lines +487 to +491
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error handling in the new SSL engine configuration code doesn't set childCurlResponse.curlSetopCode before jumping to child_cleanReturn. This is inconsistent with other error paths in this function (see lines 240, 247 for comparison). Consider setting childCurlResponse.curlSetopCode = code for consistency with existing error handling patterns.

Copilot uses AI. Check for mistakes.
Comment on lines +473 to +491
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new error paths at lines 477-481 and 487-491 jump to child_cleanReturn without freeing the headerList, which was allocated by setHeader at line 462. While the child process exits shortly after (line 595), proper resource cleanup is a best practice. Note that this is an existing issue in other error paths (lines 464-465, 469-470, 505, 519-520), but the new code adds more paths that skip the cleanup at line 560. Consider adding curl_slist_free_all(headerList) before goto child_cleanReturn in all these error paths, or ensure headerList is freed in the child_cleanReturn section.

Copilot uses AI. Check for mistakes.
}
}
Comment on lines 452 to +493
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new SSL engine configuration code (lines 473-493) and OPENSSL_init_crypto initialization (lines 453-456) are not covered by existing tests. Since source/test/protocol/ProtocolTest.cpp has comprehensive test coverage for sendReportOverHTTP, consider adding tests for these new code paths to ensure the SSL engine configuration and OpenSSL initialization work correctly under LIBRDKCERTSEL_BUILD.

Copilot uses AI. Check for mistakes.

Copy link

@pdmooneypub pdmooneypub Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copilot noticed that there were no tests added for the new code paths. can we add unit tests?

Copy link
Contributor Author

@adrojaankur adrojaankur Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the OpenSSL engine logic is not part of the core telemetry functionality, the telemetry unit test NOT handles the curl request. Based on my understanding, this scenario should be covered under L2/L3 testing rather than as a unit or core telemetry test

do
{
pCertFile = NULL;
Expand Down
Loading