RDK-60312: Remove fork calls for curl transactions #242
Conversation
Signed-off-by: Yogeswaran K <yogeswaran_k@comcast.com>
Signed-off-by: Yogeswaran K <yogeswaran_k@comcast.com>
There was a problem hiding this comment.
Pull request overview
This pull request removes fork() calls that were used for HTTP curl transactions to address OpenSSL memory management issues. The changes replace the fork-based approach with a connection pooling mechanism implemented in new files multicurlinterface.c and multicurlinterface.h.
Changes:
- Introduces a connection pool with pre-allocated CURL easy handles (MAX_POOL_SIZE=3) to eliminate the need for forking
- Removes all fork/pipe IPC code from
doHttpGet()in xconfclient.c andsendReportOverHTTP()in curlinterface.c - Adds connection pool initialization in
initTelemetry()and cleanup interminate()to manage the pool lifecycle
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 23 comments.
Show a summary per file
| File | Description |
|---|---|
| source/xconf-client/multicurlinterface.h | Defines the new connection pool API with GET/POST functions and request configuration structures |
| source/xconf-client/multicurlinterface.c | Implements connection pool with CURL handle management, mTLS certificate handling, and HTTP GET/POST operations |
| source/xconf-client/xconfclient.c | Replaces fork-based HTTP GET with http_pool_get() call; disables original callback function |
| source/protocol/http/curlinterface.c | Replaces fork-based HTTP POST with http_pool_post() call; disables original helper functions |
| source/telemetry2_0.c | Adds connection pool initialization and cleanup calls in telemetry lifecycle |
| source/xconf-client/Makefile.am | Adds multicurlinterface.c to build sources |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_FRESH_CONNECT, 0L); | ||
|
|
||
| // Set connection timeout to handle dead connections faster | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_CONNECTTIMEOUT, 10L); |
There was a problem hiding this comment.
CURLOPT_CONNECTTIMEOUT is set twice: once on line 176 to 10L and again on line 193 to 10L. Remove the duplicate assignment on line 193.
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_CONNECTTIMEOUT, 10L); | |
| /* Timeout already set above with CURLOPT_CONNECTTIMEOUT */ |
| T2ERROR init_connection_pool() | ||
| { | ||
| if(pool_initialized) | ||
| { | ||
| T2Info("Connection pool already initialized\n"); | ||
| return T2ERROR_SUCCESS; | ||
| } | ||
|
|
||
| CURLcode code = CURLE_OK; | ||
| T2Info("%s ++in\n", __FUNCTION__); | ||
| #if 0 | ||
| char *pCertFile = NULL; | ||
| char *pPasswd = NULL; | ||
| #endif | ||
| //pool.multi_handle = curl_multi_init(); | ||
|
|
||
| // Pre-allocate easy handles | ||
| for(int i = 0; i < MAX_POOL_SIZE; i++) | ||
| { | ||
| pool.easy_handles[i] = curl_easy_init(); | ||
| pool.handle_available[i] = true; | ||
|
|
||
| //Set common options once | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_TIMEOUT, 30L); | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_CONNECTTIMEOUT, 10L); | ||
|
|
||
| #if 1 | ||
| // More aggressive keepalive settings for your environment | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_TCP_KEEPALIVE, 1L); | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_TCP_KEEPIDLE, 50L); // 1 minute instead of 2 | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_TCP_KEEPINTVL, 30L); // 30 seconds instead of 60 | ||
|
|
||
| #ifdef CURLOPT_TCP_KEEPCNT | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_TCP_KEEPCNT, 15L); // Allow up to 15 probes | ||
| #endif | ||
|
|
||
| // Add connection reuse validation | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_FORBID_REUSE, 0L); | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_FRESH_CONNECT, 0L); | ||
|
|
||
| // Set connection timeout to handle dead connections faster | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_CONNECTTIMEOUT, 10L); | ||
|
|
||
| // Add low-level socket options for better connection health detection | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_NOSIGNAL, 1L); | ||
|
|
||
| // Add HTTP-level keep-alive headers for better server compatibility | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_HTTPHEADER, NULL); // Reset any existing headers first | ||
|
|
||
| // Add low-level socket options for better connection health detection | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_NOSIGNAL, 1L); | ||
|
|
||
| // Connection management options that work with older libcurl | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_PIPEWAIT, 0L); // Don't wait for pipelining | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_1_1); // Use HTTP/1.1 | ||
| #endif | ||
| // Enable connection pooling with limited cache size | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_MAXCONNECTS, 3L); // Only 1 connection per handle | ||
|
|
||
| code = curl_easy_setopt(pool.easy_handles[i], CURLOPT_SSLVERSION, CURL_SSLVERSION_TLSv1_2); | ||
| if(code != CURLE_OK) | ||
| { | ||
| T2Error("%s : Curl set opts failed with error %s \n", __FUNCTION__, curl_easy_strerror(code)); | ||
| } | ||
|
|
||
| // Certificate selector and SSL/TLS specific options from original code | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_SSL_VERIFYPEER, 1L); | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_SSLENGINE_DEFAULT, 1L); | ||
|
|
||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_VERBOSE, 1L); | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_DEBUGFUNCTION, curl_debug_callback_func); | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_DEBUGDATA, NULL); | ||
| } | ||
|
|
||
| #if 0 | ||
| if(T2ERROR_SUCCESS == getMtlsCerts(&pCertFile, &pPasswd)) | ||
| { | ||
| for(int i = 0; i < MAX_POOL_SIZE; i++) | ||
| { | ||
| code = curl_easy_setopt(pool.easy_handles[i], CURLOPT_SSLCERTTYPE, "P12"); | ||
| if(code != CURLE_OK) | ||
| { | ||
| T2Error("%s : Curl set opts failed with error %s \n", __FUNCTION__, curl_easy_strerror(code)); | ||
| } | ||
| code = curl_easy_setopt(pool.easy_handles[i], CURLOPT_SSLCERT, pCertFile); | ||
| if(code != CURLE_OK) | ||
| { | ||
| T2Error("%s : Curl set opts failed with error %s \n", __FUNCTION__, curl_easy_strerror(code)); | ||
| } | ||
| code = curl_easy_setopt(pool.easy_handles[i], CURLOPT_KEYPASSWD, pPasswd); | ||
| if(code != CURLE_OK) | ||
| { | ||
| T2Error("%s : Curl set opts failed with error %s \n", __FUNCTION__, curl_easy_strerror(code)); | ||
| } | ||
| /* disconnect if authentication fails */ | ||
| code = curl_easy_setopt(pool.easy_handles[i], CURLOPT_SSL_VERIFYPEER, 1L); | ||
| if(code != CURLE_OK) | ||
| { | ||
| T2Error("%s : Curl set opts failed with error %s \n", __FUNCTION__, curl_easy_strerror(code)); | ||
| } | ||
| } | ||
|
|
||
| if(pCertFile) | ||
| { | ||
| free(pCertFile); | ||
| pCertFile = NULL; | ||
| } | ||
| if(pPasswd) | ||
| { | ||
| free(pPasswd); | ||
| pPasswd = NULL; | ||
| } | ||
| } | ||
| #endif | ||
| pool.post_headers = curl_slist_append(NULL, "Accept: application/json"); |
There was a problem hiding this comment.
Race condition in pool initialization: The function checks pool_initialized (line 154) before acquiring any lock. If two threads call init_connection_pool concurrently, both could pass the check and proceed to initialize the pool, causing resource leaks and corrupted state. Additionally, pool_initialized is set to true at line 268 without holding the pool_mutex, creating a visibility issue across threads. Use pthread_once for initialization or protect the entire initialization sequence with appropriate locking.
| if (config->payload) | ||
| { | ||
| curl_easy_setopt(easy, CURLOPT_POSTFIELDS, config->payload); | ||
| curl_easy_setopt(easy, CURLOPT_POSTFIELDSIZE, strlen(config->payload)); |
There was a problem hiding this comment.
The http_pool_request_ex function attempts to use pool.multi_handle on line 1059, but this handle is never initialized. In init_connection_pool (line 166), the multi_handle initialization is commented out. This will cause a NULL pointer dereference and crash when http_pool_request_ex is called. Either uncomment the initialization or remove the multi_handle usage from http_pool_request_ex.
| T2Error("%s:%u , T2:memory realloc failed\n", __func__, __LINE__); | ||
| // FIX: Don't return 0 on realloc failure - this can cause curl to retry indefinitely | ||
| // Keep the original data intact |
There was a problem hiding this comment.
Returning incorrect value on realloc failure. On line 107, when realloc fails, the function returns 'realsize' pretending the data was processed. This is dangerous because: (1) the data is actually lost, leading to incomplete/corrupt responses; (2) curl will continue sending more data which will also fail; (3) the error is silently ignored. The function should return 0 to signal an error to curl, as done in the original code (xconfclient.c line 546).
Signed-off-by: Yogeswaran K <yogeswaransky@gmail.com>
Signed-off-by: Yogeswaran K <yogeswaransky@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 17 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Add low-level socket options for better connection health detection | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_NOSIGNAL, 1L); | ||
|
|
There was a problem hiding this comment.
CURLOPT_NOSIGNAL is set twice (lines 291 and 297). The second call is redundant and should be removed.
| // Add low-level socket options for better connection health detection | |
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_NOSIGNAL, 1L); | |
| pthread_mutex_init(&pool.pool_mutex, NULL); | ||
| pool_initialized = true; |
There was a problem hiding this comment.
Thread safety issue: pool_initialized is set to true at line 365 before the mutex is initialized at line 364. If another thread checks pool_initialized after line 365 but before line 364 completes, it could proceed without proper synchronization. The order should be: initialize mutex first, then set pool_initialized.
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_1_1); // Use HTTP/1.1 | ||
| #endif | ||
| // Enable connection pooling with limited cache size | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_MAXCONNECTS, 3L); // Only 1 connection per handle |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "curl_easy_setopt(pool.easy_handles[i], _curl_opt, 3L)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
Signed-off-by: Yogeswaran K <yogeswaransky@gmail.com>
Signed-off-by: Yogeswaran K <yogeswaransky@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 21 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Clean up certificates if not using certificate selector | ||
| #ifndef LIBRDKCERTSEL_BUILD | ||
| if(NULL != pCertFile) free(pCertFile); | ||
| if(NULL != pPasswd) free(pPasswd); |
There was a problem hiding this comment.
Missing LIBRDKCONFIG_BUILD handling for password cleanup in GET requests. When LIBRDKCERTSEL_BUILD is not defined but LIBRDKCONFIG_BUILD is defined, the password should be freed using rdkconfig_free instead of plain free() (similar to how it's done in the POST implementation at lines 880-888). This matches the old implementation that was removed. Add proper conditional compilation to handle rdkconfig_free when LIBRDKCONFIG_BUILD is defined.
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_SSL_VERIFYPEER, 1L); | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_SSLENGINE_DEFAULT, 1L); | ||
|
|
||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_VERBOSE, 1L); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "curl_easy_setopt(pool.easy_handles[i], _curl_opt, 1L)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_SSLENGINE_DEFAULT, 1L); | ||
|
|
||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_VERBOSE, 1L); | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_DEBUGFUNCTION, curl_debug_callback_func); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "curl_easy_setopt(pool.easy_handles[i], _curl_opt, curl_debug_callback_func)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_VERBOSE, 1L); | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_DEBUGFUNCTION, curl_debug_callback_func); | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_DEBUGDATA, NULL); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "curl_easy_setopt(pool.easy_handles[i], _curl_opt, NULL)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
Signed-off-by: Yogeswaran K <yogeswaransky@gmail.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_1_1); // Use HTTP/1.1 | ||
|
|
||
| // Enable connection pooling with limited cache size | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_MAXCONNECTS, 3L); // Only 1 connection per handle |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "curl_easy_setopt(pool.easy_handles[i], _curl_opt, 3L)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_SSL_VERIFYPEER, 1L); | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_SSLENGINE_DEFAULT, 1L); | ||
|
|
||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_VERBOSE, 1L); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "curl_easy_setopt(pool.easy_handles[i], _curl_opt, 1L)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_SSLENGINE_DEFAULT, 1L); | ||
|
|
||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_VERBOSE, 1L); | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_DEBUGFUNCTION, curl_debug_callback_func); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "curl_easy_setopt(pool.easy_handles[i], _curl_opt, curl_debug_callback_func)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_VERBOSE, 1L); | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_DEBUGFUNCTION, curl_debug_callback_func); | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_DEBUGDATA, NULL); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "curl_easy_setopt(pool.easy_handles[i], _curl_opt, NULL)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| //Set common options once | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_TIMEOUT, 30L); | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_CONNECTTIMEOUT, 10L); |
| { | ||
| if(pool.handle_available[i]) | ||
| { | ||
| T2Info("acquire_pool_handle ; Available handle = %d\n", i); |
There was a problem hiding this comment.
Please change this to debug
Signed-off-by: Yogeswaran K <yogeswaransky@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Add low-level socket options for better connection health detection | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_NOSIGNAL, 1L); | ||
|
|
There was a problem hiding this comment.
Duplicate curl option setting: CURLOPT_NOSIGNAL is set twice (lines 287 and 293) and CURLOPT_CONNECTTIMEOUT is set twice (lines 268 and 284). The second setting of CURLOPT_CONNECTTIMEOUT at line 284 with value 10L overwrites the first setting at line 268, making line 268 redundant. Remove the duplicate settings to avoid confusion.
| // Add low-level socket options for better connection health detection | |
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_NOSIGNAL, 1L); | |
| pool.easy_handles[i] = curl_easy_init(); | ||
| pool.handle_available[i] = true; | ||
|
|
||
| //Set common options once | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_TIMEOUT, 30L); | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_CONNECTTIMEOUT, 10L); | ||
|
|
||
| // More aggressive keepalive settings for your environment | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_TCP_KEEPALIVE, 1L); | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_TCP_KEEPIDLE, 50L); // 1 minute instead of 2 | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_TCP_KEEPINTVL, 30L); // 30 seconds instead of 60 | ||
|
|
||
| #ifdef CURLOPT_TCP_KEEPCNT | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_TCP_KEEPCNT, 15L); // Allow up to 15 probes | ||
| #endif | ||
|
|
||
| // Add connection reuse validation | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_FORBID_REUSE, 0L); | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_FRESH_CONNECT, 0L); | ||
|
|
||
| // Set connection timeout to handle dead connections faster | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_CONNECTTIMEOUT, 10L); | ||
|
|
||
| // Add low-level socket options for better connection health detection | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_NOSIGNAL, 1L); | ||
|
|
||
| // Add HTTP-level keep-alive headers for better server compatibility | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_HTTPHEADER, NULL); // Reset any existing headers first | ||
|
|
||
| // Add low-level socket options for better connection health detection | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_NOSIGNAL, 1L); | ||
|
|
||
| // Connection management options that work with older libcurl | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_PIPEWAIT, 0L); // Don't wait for pipelining | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_1_1); // Use HTTP/1.1 | ||
|
|
||
| // Enable connection pooling with limited cache size | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_MAXCONNECTS, 3L); // Only 1 connection per handle | ||
|
|
||
| code = curl_easy_setopt(pool.easy_handles[i], CURLOPT_SSLVERSION, CURL_SSLVERSION_TLSv1_2); | ||
| if(code != CURLE_OK) | ||
| { | ||
| T2Error("%s : Curl set opts failed with error %s \n", __FUNCTION__, curl_easy_strerror(code)); | ||
| } | ||
|
|
||
| // Certificate selector and SSL/TLS specific options from original code | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_SSL_VERIFYPEER, 1L); | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_SSLENGINE_DEFAULT, 1L); | ||
|
|
||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_VERBOSE, 1L); | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_DEBUGFUNCTION, curl_debug_callback_func); | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_DEBUGDATA, NULL); |
There was a problem hiding this comment.
Missing error handling: curl_easy_init can return NULL if initialization fails, but there's no check for this failure. If curl_easy_init fails for any handle, subsequent curl_easy_setopt calls will operate on NULL pointers, causing undefined behavior. Add null checks after each curl_easy_init call and handle initialization failures appropriately (e.g., cleanup already-initialized handles and return error).
| if (!pool_initialized) | ||
| { | ||
| T2ERROR ret = init_connection_pool(); | ||
| if(ret != T2ERROR_SUCCESS) | ||
| { | ||
| T2Error("Failed to initialize connection pool\n"); | ||
| return ret; | ||
| } | ||
| } |
There was a problem hiding this comment.
Thread-safety issue: The pool_initialized check at line 330 is performed without mutex protection. If init_connection_pool is being called by another thread concurrently, this thread could call init_connection_pool while it's already in progress, potentially causing race conditions. The check should be performed under mutex protection, or init_connection_pool should use pthread_once to ensure single initialization.
| T2ERROR http_pool_cleanup(void) | ||
| { | ||
| if (!pool_initialized) | ||
| { | ||
| T2Info("Pool not initialized, nothing to cleanup\n"); | ||
| return T2ERROR_SUCCESS; | ||
| } | ||
|
|
||
| T2Info("%s ++in\n", __FUNCTION__); | ||
|
|
||
| // Signal any waiting threads to wake up | ||
| pthread_mutex_lock(&pool.pool_mutex); | ||
| pthread_cond_broadcast(&pool.handle_available_cond); | ||
| pthread_mutex_unlock(&pool.pool_mutex); | ||
|
|
||
| // FIX: Free all header lists before cleanup | ||
| if(pool.post_headers) | ||
| { | ||
| curl_slist_free_all(pool.post_headers); | ||
| pool.post_headers = NULL; | ||
| } | ||
|
|
||
| // Cleanup all easy handles | ||
| for(int i = 0; i < MAX_POOL_SIZE; i++) | ||
| { | ||
| if(pool.easy_handles[i]) | ||
| { | ||
| curl_easy_cleanup(pool.easy_handles[i]); | ||
| pool.easy_handles[i] = NULL; | ||
| } | ||
| } | ||
|
|
||
| // Destroy condition variable and mutex | ||
| pthread_cond_destroy(&pool.handle_available_cond); | ||
| pthread_mutex_destroy(&pool.pool_mutex); | ||
|
|
||
| // Reset initialization flag | ||
| pool_initialized = false; | ||
|
|
||
| T2Info("%s ++out\n", __FUNCTION__); | ||
| return T2ERROR_SUCCESS; | ||
| } |
There was a problem hiding this comment.
Missing cleanup for certificate selectors: The xcCertSelector initialized in init_connection_pool (via xcCertSelectorInit at line 255) is never freed. The xcCertSelectorFree function exists but is not called from http_pool_cleanup or anywhere else. This will cause a memory leak. The http_pool_cleanup function should call xcCertSelectorFree to properly clean up the certificate selector resources before destroying the pool.
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_1_1); // Use HTTP/1.1 | ||
|
|
||
| // Enable connection pooling with limited cache size | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_MAXCONNECTS, 3L); // Only 1 connection per handle |
There was a problem hiding this comment.
Misleading comment: The comment says "Only 1 connection per handle" but the value is set to 3L. If the intent is to limit to 1 connection per handle, the value should be 1L. If 3 connections are intended, update the comment to reflect this.
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_MAXCONNECTS, 3L); // Only 1 connection per handle | |
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_MAXCONNECTS, 3L); // Allow up to 3 connections per handle |
| if(pool_initialized) | ||
| { | ||
| T2Info("Connection pool already initialized\n"); | ||
| return T2ERROR_SUCCESS; | ||
| } | ||
|
|
||
| CURLcode code = CURLE_OK; | ||
| T2Info("%s ++in\n", __FUNCTION__); | ||
|
|
||
| #ifdef LIBRDKCERTSEL_BUILD | ||
| // Initialize certificate selector before setting up connection pool | ||
| curlCertSelectorInit(); | ||
| xcCertSelectorInit(); | ||
| #endif | ||
|
|
||
| //pool.multi_handle = curl_multi_init(); | ||
|
|
||
| // Pre-allocate easy handles | ||
| for(int i = 0; i < MAX_POOL_SIZE; i++) | ||
| { | ||
| pool.easy_handles[i] = curl_easy_init(); | ||
| pool.handle_available[i] = true; | ||
|
|
||
| //Set common options once | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_TIMEOUT, 30L); | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_CONNECTTIMEOUT, 10L); | ||
|
|
||
| // More aggressive keepalive settings for your environment | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_TCP_KEEPALIVE, 1L); | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_TCP_KEEPIDLE, 50L); // 1 minute instead of 2 | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_TCP_KEEPINTVL, 30L); // 30 seconds instead of 60 | ||
|
|
||
| #ifdef CURLOPT_TCP_KEEPCNT | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_TCP_KEEPCNT, 15L); // Allow up to 15 probes | ||
| #endif | ||
|
|
||
| // Add connection reuse validation | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_FORBID_REUSE, 0L); | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_FRESH_CONNECT, 0L); | ||
|
|
||
| // Set connection timeout to handle dead connections faster | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_CONNECTTIMEOUT, 10L); | ||
|
|
||
| // Add low-level socket options for better connection health detection | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_NOSIGNAL, 1L); | ||
|
|
||
| // Add HTTP-level keep-alive headers for better server compatibility | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_HTTPHEADER, NULL); // Reset any existing headers first | ||
|
|
||
| // Add low-level socket options for better connection health detection | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_NOSIGNAL, 1L); | ||
|
|
||
| // Connection management options that work with older libcurl | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_PIPEWAIT, 0L); // Don't wait for pipelining | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_1_1); // Use HTTP/1.1 | ||
|
|
||
| // Enable connection pooling with limited cache size | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_MAXCONNECTS, 3L); // Only 1 connection per handle | ||
|
|
||
| code = curl_easy_setopt(pool.easy_handles[i], CURLOPT_SSLVERSION, CURL_SSLVERSION_TLSv1_2); | ||
| if(code != CURLE_OK) | ||
| { | ||
| T2Error("%s : Curl set opts failed with error %s \n", __FUNCTION__, curl_easy_strerror(code)); | ||
| } | ||
|
|
||
| // Certificate selector and SSL/TLS specific options from original code | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_SSL_VERIFYPEER, 1L); | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_SSLENGINE_DEFAULT, 1L); | ||
|
|
||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_VERBOSE, 1L); | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_DEBUGFUNCTION, curl_debug_callback_func); | ||
| curl_easy_setopt(pool.easy_handles[i], CURLOPT_DEBUGDATA, NULL); | ||
| } | ||
|
|
||
| pool.post_headers = curl_slist_append(NULL, "Accept: application/json"); | ||
| pool.post_headers = curl_slist_append(pool.post_headers, "Content-type: application/json"); | ||
|
|
||
| pthread_mutex_init(&pool.pool_mutex, NULL); | ||
| pthread_cond_init(&pool.handle_available_cond, NULL); // Initialize condition variable | ||
| pool_initialized = true; |
There was a problem hiding this comment.
Thread-safety issue: The pool_initialized check at line 243 and the initialization at lines 260-322 are not atomic. If multiple threads call init_connection_pool concurrently, both could pass the check at line 243 before pool_initialized is set to true at line 322, leading to multiple initializations of the same pool resources. This can cause resource leaks, double initialization of mutexes/condition variables, and other undefined behavior. Consider using pthread_once or adding mutex protection around the entire initialization block.
Signed-off-by: Yogeswaran K <yogeswaransky@gmail.com>
Signed-off-by: Yogeswaran K <yogeswaransky@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Destroy condition variable and mutex | ||
| pthread_cond_destroy(&pool.handle_available_cond); | ||
| pthread_mutex_destroy(&pool.pool_mutex); |
There was a problem hiding this comment.
Potential race condition in http_pool_cleanup. The function broadcasts to waiting threads at lines 864-866, but then immediately proceeds to cleanup handles and destroy synchronization primitives. If a thread was waiting on the condition variable and wakes up, it might try to access pool resources that are being destroyed. The cleanup should ensure all threads have exited the acquire_pool_handle function before destroying resources, or implement a shutdown flag that causes acquire_pool_handle to return an error instead of waiting.
| // Destroy condition variable and mutex | |
| pthread_cond_destroy(&pool.handle_available_cond); | |
| pthread_mutex_destroy(&pool.pool_mutex); | |
| /* | |
| * NOTE: | |
| * We intentionally do not destroy pool.handle_available_cond or pool.pool_mutex here. | |
| * Other threads may still be exiting functions that wait on or lock these primitives, | |
| * and destroying them could lead to a race condition or use-after-free. | |
| * Since these are process-lifetime resources, leaving them initialized is safe. | |
| */ |
| do | ||
| { | ||
| pCertFile = NULL; | ||
| pPasswd = NULL; | ||
| pCertURI = NULL; | ||
| xcGetCertStatus = rdkcertselector_getCert(xcCertSelector, &pCertURI, &pPasswd); | ||
| if(xcGetCertStatus != certselectorOk) | ||
| { | ||
| T2Error("%s, T2:Failed to retrieve the certificate.\n", __func__); | ||
| free(response.data); | ||
| release_pool_handle(idx); | ||
| return T2ERROR_FAILURE; | ||
| } | ||
| else | ||
| { | ||
| // skip past file scheme in URI | ||
| pCertFile = pCertURI; | ||
| if ( strncmp( pCertFile, FILESCHEME, sizeof(FILESCHEME) - 1 ) == 0 ) | ||
| { | ||
| pCertFile += (sizeof(FILESCHEME) - 1); | ||
| } | ||
|
|
||
| // Configure mTLS certificates | ||
| CURL_SETOPT_CHECK_STR(easy, CURLOPT_SSLCERTTYPE, "P12"); | ||
| CURL_SETOPT_CHECK_STR(easy, CURLOPT_SSLCERT, pCertFile); | ||
| CURL_SETOPT_CHECK_STR(easy, CURLOPT_KEYPASSWD, pPasswd); | ||
| CURL_SETOPT_CHECK(easy, CURLOPT_SSL_VERIFYPEER, 1L); | ||
| } | ||
| } | ||
| while(rdkcertselector_setCurlStatus(xcCertSelector, CURLE_OK, (const char*)url) == TRY_ANOTHER); | ||
| #else | ||
| // Fallback to getMtlsCerts if certificate selector not available | ||
| if(T2ERROR_SUCCESS == getMtlsCerts(&pCertFile, &pPasswd)) | ||
| { | ||
| // Configure mTLS certificates | ||
| CURL_SETOPT_CHECK_STR(easy, CURLOPT_SSLCERTTYPE, "P12"); | ||
| CURL_SETOPT_CHECK_STR(easy, CURLOPT_SSLCERT, pCertFile); | ||
| CURL_SETOPT_CHECK_STR(easy, CURLOPT_KEYPASSWD, pPasswd); | ||
| CURL_SETOPT_CHECK(easy, CURLOPT_SSL_VERIFYPEER, 1L); | ||
| } | ||
| else | ||
| { | ||
| T2Error("mTLS_get failure\n"); | ||
| free(response.data); | ||
| release_pool_handle(idx); | ||
| return T2ERROR_FAILURE; | ||
| } | ||
| #endif | ||
| } | ||
|
|
||
| // Execute the request directly | ||
| CURLcode curl_code = curl_easy_perform(easy); |
There was a problem hiding this comment.
The certificate selector logic in http_pool_get has a critical bug. The do-while loop always passes CURLE_OK to rdkcertselector_setCurlStatus regardless of whether the subsequent curl_easy_perform succeeds or fails. This means if the curl operation fails, the certificate selector won't know about it until after the loop exits. The curl operation should be executed inside the do-while loop, and the actual curl_code should be passed to rdkcertselector_setCurlStatus, similar to how it's done in the POST function.
| T2ERROR init_connection_pool() | ||
| { | ||
| if(pool_initialized) | ||
| { | ||
| T2Debug("Connection pool already initialized\n"); | ||
| return T2ERROR_SUCCESS; | ||
| } | ||
|
|
||
| T2Debug("%s ++in\n", __FUNCTION__); | ||
|
|
||
| #ifdef LIBRDKCERTSEL_BUILD | ||
| // Initialize certificate selector before setting up connection pool | ||
| curlCertSelectorInit(); | ||
| xcCertSelectorInit(); | ||
| #endif | ||
|
|
||
| // Pre-allocate easy handles | ||
| for(int i = 0; i < MAX_POOL_SIZE; i++) | ||
| { | ||
| pool.easy_handles[i] = curl_easy_init(); | ||
| pool.handle_available[i] = true; | ||
|
|
||
| //Set common options for each handle | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_TIMEOUT, 30L); | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_CONNECTTIMEOUT, 10L); | ||
|
|
||
| // More aggressive keepalive settings for your environment | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_TCP_KEEPALIVE, 1L); | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_TCP_KEEPIDLE, 50L); | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_TCP_KEEPINTVL, 30L); | ||
|
|
||
| #ifdef CURLOPT_TCP_KEEPCNT | ||
| // The option CURLOPT_TCP_KEEPCNT is not available in libcurl versions older than 7.25.0 | ||
| // Set number of keepalive probes before considering the connection dead | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_TCP_KEEPCNT, 15L); | ||
| #endif | ||
|
|
||
| // Add connection reuse validation | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_FORBID_REUSE, 0L); | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_FRESH_CONNECT, 0L); | ||
|
|
||
| // Add low-level socket options for better connection health detection | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_NOSIGNAL, 1L); | ||
|
|
||
| // Add HTTP-level keep-alive headers for better server compatibility | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_HTTPHEADER, NULL); | ||
|
|
||
| // Connection management options that work with older libcurl | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_PIPEWAIT, 0L); | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_1_1); | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_SSLVERSION, CURL_SSLVERSION_TLSv1_2); | ||
|
|
||
| // Certificate selector and SSL/TLS specific options from original code | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_SSL_VERIFYPEER, 1L); | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_SSLENGINE_DEFAULT, 1L); | ||
| } | ||
| pool.post_headers = curl_slist_append(NULL, "Accept: application/json"); | ||
| pool.post_headers = curl_slist_append(pool.post_headers, "Content-type: application/json"); | ||
|
|
||
| pthread_mutex_init(&pool.pool_mutex, NULL); | ||
| pthread_cond_init(&pool.handle_available_cond, NULL); // Initialize condition variable | ||
| pool_initialized = true; | ||
| T2Debug("%s ++out\n", __FUNCTION__); | ||
| return T2ERROR_SUCCESS; | ||
| } |
There was a problem hiding this comment.
The connection pool implementation does not call curl_global_init. According to libcurl documentation, curl_global_init should be called once per application before using any other libcurl functions. Since the code is creating a pool of curl easy handles and using them from multiple threads, curl_global_init should be called in init_connection_pool with the CURL_GLOBAL_ALL flag. Without this, the curl library may not be properly initialized for multi-threaded use.
Coverity Issue - Data race conditionAccessing "profile->grepSeekProfile->execCounter" without holding lock "plMutex". Elsewhere, "_GrepSeekProfile.execCounter" is written to with "plMutex" held 2 out of 2 times. Medium Impact, CWE-366 Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
Signed-off-by: Yogeswaran K <yogeswaransky@gmail.com>
Signed-off-by: Yogeswaran K <yogeswaransky@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| char *ptr = (char*) realloc(httpResponse->data, | ||
| httpResponse->size + realsize + 1); |
There was a problem hiding this comment.
There's a potential integer overflow when calculating the new buffer size (httpResponse->size + realsize + 1). If httpResponse->size and realsize are both large, their sum could overflow. Consider checking for overflow before the realloc call to prevent potential security issues and incorrect memory allocation.
| // Cleanup all easy handles | ||
| for(int i = 0; i < MAX_POOL_SIZE; i++) | ||
| { | ||
| if(pool.easy_handles[i]) | ||
| { | ||
| curl_easy_cleanup(pool.easy_handles[i]); | ||
| pool.easy_handles[i] = NULL; | ||
| } | ||
| } |
There was a problem hiding this comment.
The http_pool_cleanup function doesn't check if any handles are currently in use (handle_available[i] == false) before cleaning them up. If a handle is being used by an active request when cleanup is called, this will result in curl_easy_cleanup being called on a handle that's still in use, leading to undefined behavior or crashes. Consider adding a check to wait for all handles to be released before cleanup, or add logic to mark the pool as "shutting down" to prevent new acquisitions.
| T2ERROR init_connection_pool(); | ||
|
|
||
| // New dedicated APIs for better separation of concerns | ||
| T2ERROR http_pool_get(const char *url, char **response_data, bool enable_file_output); |
There was a problem hiding this comment.
The function http_pool_get uses the bool type in its parameter (bool enable_file_output), but the header file does not include stdbool.h. This could cause compilation errors if a source file includes this header before including stdbool.h. Add "#include <stdbool.h>" to the header file to ensure bool is properly defined.
| memset(waninterface, 0, sizeof(waninterface)); | ||
| snprintf(waninterface, sizeof(waninterface), "%s", IFINTERFACE); | ||
|
|
||
| if(T2ERROR_SUCCESS == getParameterValue(TR181_DEVICE_CURRENT_WAN_IFNAME, ¶mVal)) |
There was a problem hiding this comment.
The macro TR181_DEVICE_CURRENT_WAN_IFNAME is used but not defined in this file. This macro is defined in curlinterface.h, but that header is not included. Either include curlinterface.h or move the TR181_DEVICE_CURRENT_WAN_IFNAME definition to a shared header file that both curlinterface.c and multicurlinterface.c can include.
| #ifdef LIBRDKCERTSEL_BUILD | ||
| // Cleanup certificate selectors to prevent memory leak | ||
| curlCertSelectorFree(); | ||
| #endif | ||
|
|
There was a problem hiding this comment.
There is a potential double-free issue with certificate selectors. curlCertSelectorFree() is called both in ReportProfiles_uninit() (line 708 in reportprofiles.c) and in http_pool_cleanup() (line 846 in multicurlinterface.c). Since terminate() in telemetry2_0.c calls ReportProfiles_uninit() at line 131 followed by http_pool_cleanup() at line 132, the certificate selectors will be freed twice, which could cause a crash. Remove one of these calls - likely the one in ReportProfiles_uninit() since the certificate selectors are now owned by the connection pool.
| #ifdef LIBRDKCERTSEL_BUILD | |
| // Cleanup certificate selectors to prevent memory leak | |
| curlCertSelectorFree(); | |
| #endif | |
| memset(waninterface, 0, sizeof(waninterface)); | ||
| snprintf(waninterface, sizeof(waninterface), "%s", IFINTERFACE); | ||
|
|
||
| if(T2ERROR_SUCCESS == getParameterValue(TR181_DEVICE_CURRENT_WAN_IFNAME, ¶mVal)) |
There was a problem hiding this comment.
The macro TR181_DEVICE_CURRENT_WAN_IFNAME is used but not defined in this file. This macro is defined in curlinterface.h, but that header is not included. Either include curlinterface.h or move the TR181_DEVICE_CURRENT_WAN_IFNAME definition to a shared header file that both curlinterface.c and multicurlinterface.c can include.
| @@ -989,50 +987,3 @@ T2ERROR ProfileXConf_storeMarkerEvent(T2Event *eventInfo) | |||
| return T2ERROR_SUCCESS; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The function declaration for ProfileXConf_terminateReport() remains in the header file (source/bulkdata/profilexconf.h:83), but the implementation has been removed. This declaration should be removed from the header file as well to avoid undefined reference errors at link time.
| void ProfileXConf_terminateReport(void) | |
| { | |
| /* Implementation intentionally left empty; retained for interface compatibility. */ | |
| } |
| // Signal any waiting threads to wake up | ||
| pthread_mutex_lock(&pool.pool_mutex); | ||
| pthread_cond_broadcast(&pool.handle_available_cond); | ||
| pthread_mutex_unlock(&pool.pool_mutex); |
There was a problem hiding this comment.
There is a race condition in the cleanup sequence. The pthread_cond_broadcast at line 841 will wake up any threads waiting in acquire_pool_handle (line 276), but these threads will then proceed to check for available handles and potentially access pool data that is being destroyed. The pool_initialized flag is only set to false after all cleanup is done (line 871), so woken threads might still try to acquire handles. Consider setting pool_initialized to false before broadcasting, and modify acquire_pool_handle to check pool_initialized after waking from pthread_cond_wait to handle cleanup scenarios properly.
| #if defined(ENABLE_RDKB_SUPPORT) && !defined(RDKB_EXTENDER) | ||
| #if defined(WAN_FAILOVER_SUPPORTED) || defined(FEATURE_RDKB_CONFIGURABLE_WAN_INTERFACE) | ||
| char *paramVal = NULL; | ||
| char waninterface[256]; | ||
| memset(waninterface, 0, sizeof(waninterface)); | ||
| snprintf(waninterface, sizeof(waninterface), "%s", IFINTERFACE); | ||
|
|
||
| if(T2ERROR_SUCCESS == getParameterValue(TR181_DEVICE_CURRENT_WAN_IFNAME, ¶mVal)) | ||
| { | ||
| if(strlen(paramVal) > 0) | ||
| { | ||
| memset(waninterface, 0, sizeof(waninterface)); | ||
| snprintf(waninterface, sizeof(waninterface), "%s", paramVal); | ||
| T2Info("TR181_DEVICE_CURRENT_WAN_IFNAME -- %s\n", waninterface); | ||
| } | ||
| free(paramVal); | ||
| paramVal = NULL; | ||
| } | ||
| else | ||
| { | ||
| T2Error("Failed to get Value for %s\n", TR181_DEVICE_CURRENT_WAN_IFNAME); | ||
| } | ||
|
|
||
| CURL_SETOPT_CHECK_STR(easy, CURLOPT_INTERFACE, waninterface); | ||
| T2Info("TR181_DEVICE_CURRENT_WAN_IFNAME ---- %s\n", waninterface); | ||
| #else | ||
| CURL_SETOPT_CHECK(easy, CURLOPT_INTERFACE, "erouter0"); | ||
| #endif | ||
| #endif | ||
|
|
There was a problem hiding this comment.
The WAN interface configuration code (lines 339-367 in http_pool_get and lines 615-643 in http_pool_post) is duplicated. Consider extracting this into a shared helper function to reduce code duplication and improve maintainability.
| #if defined(ENABLE_RDKB_SUPPORT) && !defined(RDKB_EXTENDER) | |
| #if defined(WAN_FAILOVER_SUPPORTED) || defined(FEATURE_RDKB_CONFIGURABLE_WAN_INTERFACE) | |
| char *paramVal = NULL; | |
| char waninterface[256]; | |
| memset(waninterface, 0, sizeof(waninterface)); | |
| snprintf(waninterface, sizeof(waninterface), "%s", IFINTERFACE); | |
| if(T2ERROR_SUCCESS == getParameterValue(TR181_DEVICE_CURRENT_WAN_IFNAME, ¶mVal)) | |
| { | |
| if(strlen(paramVal) > 0) | |
| { | |
| memset(waninterface, 0, sizeof(waninterface)); | |
| snprintf(waninterface, sizeof(waninterface), "%s", paramVal); | |
| T2Info("TR181_DEVICE_CURRENT_WAN_IFNAME -- %s\n", waninterface); | |
| } | |
| free(paramVal); | |
| paramVal = NULL; | |
| } | |
| else | |
| { | |
| T2Error("Failed to get Value for %s\n", TR181_DEVICE_CURRENT_WAN_IFNAME); | |
| } | |
| CURL_SETOPT_CHECK_STR(easy, CURLOPT_INTERFACE, waninterface); | |
| T2Info("TR181_DEVICE_CURRENT_WAN_IFNAME ---- %s\n", waninterface); | |
| #else | |
| CURL_SETOPT_CHECK(easy, CURLOPT_INTERFACE, "erouter0"); | |
| #endif | |
| #endif | |
| configure_wan_interface(easy); | |
| lib_LTLIBRARIES = libhttp.la | ||
|
|
||
| libhttp_la_SOURCES = curlinterface.c | ||
| libhttp_la_SOURCES = curlinterface.c multicurlinterface.c |
There was a problem hiding this comment.
Adding multicurlinterface.c to libhttp_la_SOURCES causes the build to start linking and using the HTTP connection-pool implementation, which writes HTTP responses to fixed filenames in /tmp (e.g., /tmp/httpOutput.txt and /tmp/curlOutput.txt) using open(..., O_WRONLY | O_CREAT | O_TRUNC, ...). Because this service runs as root and /tmp is world-writable, a local attacker can plant symlinks at those paths and have privileged code overwrite arbitrary files with network-provided data, leading to integrity violations and potential escalation via other system components. Before enabling this module via the Makefile, harden the underlying implementation to use safe temporary-file handling (unique, non-predictable paths in a protected directory or open with anti-symlink semantics) instead of predictable paths in /tmp.
| libhttp_la_SOURCES = curlinterface.c multicurlinterface.c | |
| libhttp_la_SOURCES = curlinterface.c |
| CURL_SETOPT_CHECK_STR(easy, CURLOPT_URL, url); | ||
| CURL_SETOPT_CHECK(easy, CURLOPT_HTTPGET, 1L); | ||
| CURL_SETOPT_CHECK(easy, CURLOPT_WRITEFUNCTION, httpGetCallBack); | ||
| CURL_SETOPT_CHECK(easy, CURLOPT_WRITEDATA, (void *) &response); |
There was a problem hiding this comment.
Coverity Issue - Incorrect sizeof expression
Taking the size of "&response", which is the address of an object, is suspicious.
Medium Impact, CWE-467
BAD_SIZEOF
How to fix
Did you intend the size of "response" itself?
Signed-off-by: Yogeswaran K <yogeswaransky@gmail.com>
| CURL_SETOPT_CHECK_STR(easy, CURLOPT_URL, url); | ||
| CURL_SETOPT_CHECK(easy, CURLOPT_HTTPGET, 1L); | ||
| CURL_SETOPT_CHECK(easy, CURLOPT_WRITEFUNCTION, httpGetCallBack); | ||
| CURL_SETOPT_CHECK(easy, CURLOPT_WRITEDATA, (void *) &response); |
There was a problem hiding this comment.
Coverity Issue - Incorrect sizeof expression
Taking the size of "&response", which is the address of an object, is suspicious.
Medium Impact, CWE-467
BAD_SIZEOF
How to fix
Did you intend the size of "response" itself?
Signed-off-by: Yogeswaran K <yogeswaransky@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if(pPasswd != NULL) | ||
| { | ||
| free(pPasswd); | ||
| pPasswd = NULL; | ||
| } | ||
| pCertFile = NULL; | ||
|
|
||
| curlGetCertStatus = rdkcertselector_getCert(thisCertSel, &pCertURI, &pCertPC); |
There was a problem hiding this comment.
There is an inconsistency in variable usage in the certificate handling loop. The code declares pPasswd (line 702) and attempts to free it in the loop (lines 726-730), but rdkcertselector_getCert is called with &pCertPC (line 733) which is a different variable declared at line 692. This means pPasswd is never actually set by rdkcertselector_getCert, so the free operation is freeing uninitialized memory. Either use pCertPC consistently (and free it in the loop), or change line 733 to use &pPasswd to match the loop's free logic.
| pool.post_headers = curl_slist_append(NULL, "Accept: application/json"); | ||
| pool.post_headers = curl_slist_append(pool.post_headers, "Content-type: application/json"); |
There was a problem hiding this comment.
The second curl_slist_append call at line 246 does not check if the first append succeeded. If the first append fails and returns NULL, the second append will also fail. The return value of both curl_slist_append calls should be checked, and if either fails, the function should clean up and return T2ERROR_FAILURE to prevent using a NULL or incomplete header list.
| pool.post_headers = curl_slist_append(NULL, "Accept: application/json"); | |
| pool.post_headers = curl_slist_append(pool.post_headers, "Content-type: application/json"); | |
| pool.post_headers = curl_slist_append(NULL, "Accept: application/json"); | |
| if (pool.post_headers == NULL) { | |
| T2Error("%s : Failed to append HTTP header: Accept: application/json\n", __FUNCTION__); | |
| return T2ERROR_FAILURE; | |
| } | |
| pool.post_headers = curl_slist_append(pool.post_headers, "Content-type: application/json"); | |
| if (pool.post_headers == NULL) { | |
| T2Error("%s : Failed to append HTTP header: Content-type: application/json\n", __FUNCTION__); | |
| curl_slist_free_all(pool.post_headers); | |
| pool.post_headers = NULL; | |
| return T2ERROR_FAILURE; | |
| } |
| // Request types for different HTTP operations | ||
| typedef enum | ||
| { | ||
| HTTP_REQUEST_GET, // For HTTP GET | ||
| HTTP_REQUEST_POST // For sendReportOverHTTP | ||
| } http_request_type_t; | ||
|
|
There was a problem hiding this comment.
The enum type http_request_type_t is defined but never used anywhere in the codebase. Since the new implementation uses dedicated functions (http_pool_get and http_pool_post) instead of a generic function with a type parameter, this enum is unnecessary and should be removed to avoid confusion and reduce code clutter.
| // Request types for different HTTP operations | |
| typedef enum | |
| { | |
| HTTP_REQUEST_GET, // For HTTP GET | |
| HTTP_REQUEST_POST // For sendReportOverHTTP | |
| } http_request_type_t; | |
| for(int i = 0; i < MAX_POOL_SIZE; i++) | ||
| { | ||
| pool.easy_handles[i] = curl_easy_init(); | ||
| pool.handle_available[i] = true; | ||
|
|
||
| //Set common options for each handle | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_TIMEOUT, 30L); | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_CONNECTTIMEOUT, 10L); | ||
|
|
||
| // More aggressive keepalive settings for your environment | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_TCP_KEEPALIVE, 1L); | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_TCP_KEEPIDLE, 50L); | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_TCP_KEEPINTVL, 30L); | ||
|
|
||
| #ifdef CURLOPT_TCP_KEEPCNT | ||
| // The option CURLOPT_TCP_KEEPCNT is not available in libcurl versions older than 7.25.0 | ||
| // Set number of keepalive probes before considering the connection dead | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_TCP_KEEPCNT, 15L); | ||
| #endif | ||
|
|
||
| // Add connection reuse validation | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_FORBID_REUSE, 0L); | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_FRESH_CONNECT, 0L); | ||
|
|
||
| // Add low-level socket options for better connection health detection | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_NOSIGNAL, 1L); | ||
|
|
||
| // Add HTTP-level keep-alive headers for better server compatibility | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_HTTPHEADER, NULL); | ||
|
|
||
| // Connection management options that work with older libcurl | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_PIPEWAIT, 0L); | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_1_1); | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_SSLVERSION, CURL_SSLVERSION_TLSv1_2); | ||
|
|
||
| // Certificate selector and SSL/TLS specific options from original code | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_SSL_VERIFYPEER, 1L); | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_SSLENGINE_DEFAULT, 1L); | ||
| } |
There was a problem hiding this comment.
The initialization function does not check if curl_easy_init() succeeds before configuring the handle. If curl_easy_init() returns NULL (failure), subsequent CURL_SETOPT_CHECK calls will operate on a NULL handle, which could lead to undefined behavior. Add a check after line 208 to verify that pool.easy_handles[i] is not NULL, and if it is, clean up previously allocated handles and return T2ERROR_FAILURE.
| CURL_SETOPT_CHECK_STR(easy, CURLOPT_URL, url); | ||
| CURL_SETOPT_CHECK(easy, CURLOPT_HTTPGET, 1L); | ||
| CURL_SETOPT_CHECK(easy, CURLOPT_WRITEFUNCTION, httpGetCallBack); | ||
| CURL_SETOPT_CHECK(easy, CURLOPT_WRITEDATA, (void *) &response); |
There was a problem hiding this comment.
Coverity Issue - Incorrect sizeof expression
Taking the size of "&response", which is the address of an object, is suspicious.
Medium Impact, CWE-467
BAD_SIZEOF
How to fix
Did you intend the size of "response" itself?
Signed-off-by: Yogeswaran K <yogeswaransky@gmail.com>
| CURL_SETOPT_CHECK_STR(easy, CURLOPT_URL, url); | ||
| CURL_SETOPT_CHECK(easy, CURLOPT_HTTPGET, 1L); | ||
| CURL_SETOPT_CHECK(easy, CURLOPT_WRITEFUNCTION, httpGetCallBack); | ||
| CURL_SETOPT_CHECK(easy, CURLOPT_WRITEDATA, (void *) &response); |
There was a problem hiding this comment.
Coverity Issue - Incorrect sizeof expression
Taking the size of "&response", which is the address of an object, is suspicious.
Medium Impact, CWE-467
BAD_SIZEOF
How to fix
Did you intend the size of "response" itself?
Signed-off-by: Yogeswaran K <yogeswaransky@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Clean up certificates if not using certificate selector | ||
| #ifndef LIBRDKCERTSEL_BUILD | ||
| if(NULL != pCertFile) | ||
| { | ||
| free(pCertFile); | ||
| } | ||
| if(NULL != pPasswd) | ||
| { | ||
| free(pPasswd); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Memory leak when LIBRDKCERTSEL_BUILD is enabled: In the http_pool_get function, when using certificate selector, the pCertURI and pPasswd allocations from rdkcertselector_getCert are not freed after the do-while loop completes successfully. These should be freed at the end of the function similar to how they're freed in the non-LIBRDKCERTSEL_BUILD path.
| #endif | ||
| } | ||
| #endif | ||
|
|
There was a problem hiding this comment.
Memory leak when LIBRDKCERTSEL_BUILD is enabled: In the http_pool_post function, when using certificate selector, the pCertURI and pCertPC allocations from rdkcertselector_getCert are not freed after the do-while loop completes successfully. These should be freed at the end of the function similar to how they're freed in the non-LIBRDKCERTSEL_BUILD path.
| #ifdef LIBRDKCERTSEL_BUILD | |
| /* Clean up selector-provided certificate buffers to prevent leaks */ | |
| if (NULL != pCertURI) | |
| { | |
| free(pCertURI); | |
| } | |
| if (NULL != pCertPC) | |
| { | |
| free(pCertPC); | |
| } | |
| #endif |
| if(curlCertSelector == NULL || curlRcvryCertSelector == NULL) | ||
| { | ||
| T2Info("%s, T2:Cert selector memory free\n", __func__); | ||
| } | ||
| else | ||
| { | ||
| T2Info("%s, T2:Cert selector memory free failed\n", __func__); | ||
| } |
There was a problem hiding this comment.
The logic in this condition is incorrect. After calling rdkcertselector_free, both pointers should be NULL for successful cleanup. The condition should use AND (&&) instead of OR (||), or check each pointer separately. Currently, if either pointer is NULL after freeing, it logs success even if the other one failed to free.
| for(int i = 0; i < MAX_POOL_SIZE; i++) | ||
| { | ||
| pool.easy_handles[i] = curl_easy_init(); | ||
| pool.handle_available[i] = true; | ||
|
|
||
| //Set common options for each handle | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_TIMEOUT, 30L); | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_CONNECTTIMEOUT, 10L); | ||
|
|
||
| // More aggressive keepalive settings for your environment | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_TCP_KEEPALIVE, 1L); | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_TCP_KEEPIDLE, 50L); | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_TCP_KEEPINTVL, 30L); | ||
|
|
||
| #ifdef CURLOPT_TCP_KEEPCNT | ||
| // The option CURLOPT_TCP_KEEPCNT is not available in libcurl versions older than 7.25.0 | ||
| // Set number of keepalive probes before considering the connection dead | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_TCP_KEEPCNT, 15L); | ||
| #endif | ||
|
|
||
| // Add connection reuse validation | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_FORBID_REUSE, 0L); | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_FRESH_CONNECT, 0L); | ||
|
|
||
| // Add low-level socket options for better connection health detection | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_NOSIGNAL, 1L); | ||
|
|
||
| // Add HTTP-level keep-alive headers for better server compatibility | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_HTTPHEADER, NULL); | ||
|
|
||
| // Connection management options that work with older libcurl | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_PIPEWAIT, 0L); | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_1_1); | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_SSLVERSION, CURL_SSLVERSION_TLSv1_2); | ||
|
|
||
| // Certificate selector and SSL/TLS specific options from original code | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_SSL_VERIFYPEER, 1L); | ||
| CURL_SETOPT_CHECK(pool.easy_handles[i], CURLOPT_SSLENGINE_DEFAULT, 1L); | ||
| } |
There was a problem hiding this comment.
Missing error handling for curl_easy_init failure: The curl_easy_init call can return NULL if initialization fails. The code should check if pool.easy_handles[i] is NULL before setting options on it, and properly handle the failure case by cleaning up already-initialized handles and returning an error.
| -I${top_srcdir}/source/dcautil \ | ||
| -I${top_srcdir}/source/utils | ||
| -I${top_srcdir}/source/utils \ | ||
| -I${top_srcdir}/source/protocol/http |
There was a problem hiding this comment.
Inconsistent indentation: Line 41 uses tabs while the surrounding lines use spaces. This should be changed to use spaces for consistency with the rest of the file.
| -I${top_srcdir}/source/protocol/http | |
| -I${top_srcdir}/source/protocol/http |
| // Allocate response buffer locally for GET requests only | ||
| curlResponseData response; | ||
| response.data = (char*)malloc(RESPONSE_BUFFER_SIZE); | ||
| if (!response.data) | ||
| { | ||
| T2Error("Failed to allocate response buffer\n"); | ||
| release_pool_handle(idx); | ||
| return T2ERROR_FAILURE; | ||
| } | ||
| response.data[0] = '\0'; | ||
| response.size = 0; |
There was a problem hiding this comment.
Potential buffer size issue: The initial allocation uses RESPONSE_BUFFER_SIZE (8192 bytes), but the httpGetCallBack function uses realloc to grow the buffer as needed. However, if the initial malloc fails, the error is handled correctly. The concern is that the initial size might be unnecessarily large for small responses or too small for typical responses. Consider making this configurable or using a smaller initial size that grows dynamically.
| if (!pool_initialized) | ||
| { | ||
| T2ERROR ret = init_connection_pool(); | ||
| if(ret != T2ERROR_SUCCESS) | ||
| { | ||
| T2Error("Failed to initialize connection pool\n"); | ||
| return ret; | ||
| } | ||
| } | ||
|
|
||
| pthread_mutex_lock(&pool.pool_mutex); | ||
|
|
There was a problem hiding this comment.
Race condition in initialization check: The pool_initialized flag is checked without holding the pool_mutex at line 256. This creates a race condition where multiple threads could simultaneously detect that the pool is not initialized and attempt to initialize it concurrently. The initialization check and init_connection_pool call should be protected by the mutex, or use a thread-safe initialization mechanism like pthread_once.
| if (!pool_initialized) | |
| { | |
| T2ERROR ret = init_connection_pool(); | |
| if(ret != T2ERROR_SUCCESS) | |
| { | |
| T2Error("Failed to initialize connection pool\n"); | |
| return ret; | |
| } | |
| } | |
| pthread_mutex_lock(&pool.pool_mutex); | |
| pthread_mutex_lock(&pool.pool_mutex); | |
| if (!pool_initialized) | |
| { | |
| T2ERROR ret = init_connection_pool(); | |
| if(ret != T2ERROR_SUCCESS) | |
| { | |
| T2Error("Failed to initialize connection pool\n"); | |
| pthread_mutex_unlock(&pool.pool_mutex); | |
| return ret; | |
| } | |
| } | |
| CURL_SETOPT_CHECK_STR(easy, CURLOPT_URL, url); | ||
| CURL_SETOPT_CHECK(easy, CURLOPT_HTTPGET, 1L); | ||
| CURL_SETOPT_CHECK(easy, CURLOPT_WRITEFUNCTION, httpGetCallBack); | ||
| CURL_SETOPT_CHECK(easy, CURLOPT_WRITEDATA, (void *) &response); |
There was a problem hiding this comment.
Coverity Issue - Incorrect sizeof expression
Taking the size of "&response", which is the address of an object, is suspicious.
Medium Impact, CWE-467
BAD_SIZEOF
How to fix
Did you intend the size of "response" itself?
No description provided.