RDKB-62352 : Observed telemetry2_0 crash #247
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,9 @@ | |
| #include <sys/wait.h> | ||
| #include <curl/curl.h> | ||
| #include <signal.h> | ||
| #ifdef LIBRDKCERTSEL_BUILD | ||
| #include <openssl/crypto.h> | ||
| #endif | ||
|
|
||
| #include "curlinterface.h" | ||
| #include "reportprofiles.h" | ||
|
|
@@ -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) | ||
| { | ||
|
|
@@ -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; | ||
|
|
@@ -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) | ||
|
Contributor
There was a problem hiding this comment. This doesn't seem to be the right thing to do.
Contributor
Author
There was a problem hiding this comment. curl_easy_init directly will not initialize OPENSSL_init_crypto. |
||
| { | ||
| fprintf(stderr, "T2:OPENSSL_init_crypto failed\n"); // avoiding T2 logger in the child. | ||
| } | ||
| #endif | ||
| curl = curl_easy_init(); | ||
| if(curl) | ||
| { | ||
|
|
@@ -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
There was a problem hiding this comment. 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. |
||
| } | ||
| } | ||
| else | ||
| { | ||
| code = curl_easy_setopt(curl, CURLOPT_SSLENGINE_DEFAULT, 1L); | ||
| if(code != CURLE_OK ) | ||
| { | ||
|
Comment on lines
+473
to
+488
There was a problem hiding this comment. When CURLOPT_SSLENGINE / CURLOPT_SSLENGINE_DEFAULT fail, |
||
| childCurlResponse.lineNumber = __LINE__; | ||
| curl_easy_cleanup(curl); | ||
| goto child_cleanReturn; | ||
|
Comment on lines
+487
to
+491
There was a problem hiding this comment. 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.
Comment on lines
+473
to
+491
There was a problem hiding this comment. 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. |
||
| } | ||
| } | ||
|
Comment on lines
452
to
493
There was a problem hiding this comment. 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. |
||
|
|
||
|
There was a problem hiding this comment. copilot noticed that there were no tests added for the new code paths. can we add unit tests?
Contributor
Author
There was a problem hiding this comment. 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; | ||
|
|
||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.