-
Notifications
You must be signed in to change notification settings - Fork 20
RDK-60001: [SPIKE] Decouple T2 Common Library From Rbus #236
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.
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
base: develop
Are you sure you want to change the base?
Conversation
Signed-off-by: Yogeswaran K <yogeswaran_k@comcast.com>
Signed-off-by: Yogeswaran K <yogeswaran_k@comcast.com>
Signed-off-by: Yogeswaran K <yogeswaran_k@comcast.com>
Signed-off-by: Yogeswaran K <yogeswaran_k@comcast.com>
| header->component_name[sizeof(header->component_name) - 1] = '\0'; | ||
|
|
||
| // Copy marker data | ||
| memcpy(message + sizeof(T2MQMessageHeader), marker_list, strlen(marker_list)); |
Check failure
Code scanning / CodeQL
Suspicious add with sizeof High
char *
Copilot Autofix
AI 23 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| header->component_name[sizeof(header->component_name) - 1] = '\0'; | ||
|
|
||
| // Copy marker data | ||
| memcpy(message + sizeof(T2MQMessageHeader), marker_list, strlen(marker_list)); |
Check failure
Code scanning / CodeQL
Suspicious add with sizeof High
char *
Copilot Autofix
AI 23 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| T2Info("T2_MQ_MSG_SUBSCRIBE\n"); | ||
| if (header->data_length > 0) | ||
| { | ||
| char* component_name = message + sizeof(T2MQMessageHeader); |
Check failure
Code scanning / CodeQL
Suspicious add with sizeof High
char *
Copilot Autofix
AI 23 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| // Handle event data from client | ||
| if (header->data_length > 0) | ||
| { | ||
| char* event_data = message + sizeof(T2MQMessageHeader); |
Check failure
Code scanning / CodeQL
Suspicious add with sizeof High
char *
Copilot Autofix
AI 23 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| FILE *logHandle = NULL ; | ||
|
|
||
| pthread_mutex_lock(&loggerMutex); | ||
| logHandle = fopen(SENDER_LOG_FILE, "a+"); |
Check failure
Code scanning / CodeQL
File created without restricting permissions High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 23 days ago
To fix the problem, the file must be created (or adjusted) with restrictive permissions, typically read/write for the owner only (0600). The cleanest, portable approach here—without altering behavior except for permissions—is to ensure that whenever the log file is newly created, its permissions are restricted. Because fopen doesn’t allow us to specify permissions directly, we can first open the file with O_CREAT and mode S_IRUSR | S_IWUSR, then immediately close it and proceed with fopen as before. This keeps the use of FILE * and buffered I/O unchanged while guaranteeing safe permissions at creation time.
Concretely, in EVENT_DEBUG in source/commonlib/t2_transport_interface.c, just before calling fopen(SENDER_LOG_FILE, "a+"), add a small block that does:
int fd = open(SENDER_LOG_FILE, O_WRONLY | O_CREAT, S_IRUSR | S_IWUSR);- Check for
fd >= 0and close it immediately. - Ignore
EEXISTsemantics since we only care about creation; if the file already exists,openwon’t change its mode, but that is acceptable for this minimal change (we avoid altering permissions of an existing file to prevent surprising behavior).
<sys/stat.h> and <fcntl.h> are already included at the top of the file, so no new imports are required. The logic and behavior of logging remain the same, except that a missing log file will now be created with permissions 0600 rather than being dependent on umask.
-
Copy modified lines R178-R183
| @@ -175,6 +175,12 @@ | ||
| FILE *logHandle = NULL ; | ||
|
|
||
| pthread_mutex_lock(&loggerMutex); | ||
| /* Ensure the log file is created with restrictive permissions (owner read/write only) */ | ||
| int fd = open(SENDER_LOG_FILE, O_WRONLY | O_CREAT, S_IRUSR | S_IWUSR); | ||
| if (fd >= 0) | ||
| { | ||
| close(fd); | ||
| } | ||
| logHandle = fopen(SENDER_LOG_FILE, "a+"); | ||
| if(logHandle) | ||
| { |
| return NULL; | ||
| } | ||
|
|
||
| FILE *fp = fopen(T2_CACHE_FILE, "a"); |
Check failure
Code scanning / CodeQL
File created without restricting permissions High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 23 days ago
In general, to avoid creating world‑writable files, do not rely on fopen’s default creation behavior. Instead, create the file using open with an explicit permission mask such as S_IRUSR | S_IWUSR (read/write for owner only), and then, if a FILE * is needed, wrap the resulting file descriptor with fdopen. This ensures that when the file is first created, it receives restrictive permissions independent of the process’ umask.
For this specific code, the best fix is to replace fopen(T2_CACHE_FILE, "a") with a two‑step sequence: call open(T2_CACHE_FILE, O_WRONLY | O_CREAT | O_APPEND, S_IRUSR | S_IWUSR) to either open or create the cache file safely, then call fdopen on the obtained descriptor with mode "a" to maintain the same append semantics that fopen would have provided. We should handle error cases carefully: if open fails, log and jump to the existing unlock label; if fdopen fails, close the descriptor, log, and go to unlock. This change is confined to the area around line 794 and uses existing headers (<fcntl.h>, <sys/stat.h>) that are already included at the top of the file, so no new imports are required and no external behavior changes occur other than more restrictive permissions on first creation.
-
Copy modified lines R794-R795 -
Copy modified lines R800-R808
| @@ -791,12 +791,21 @@ | ||
| return NULL; | ||
| } | ||
|
|
||
| FILE *fp = fopen(T2_CACHE_FILE, "a"); | ||
| if (fp == NULL) | ||
| int cache_fd = open(T2_CACHE_FILE, O_WRONLY | O_CREAT | O_APPEND, S_IRUSR | S_IWUSR); | ||
| if (cache_fd == -1) | ||
| { | ||
| EVENT_ERROR("%s: File open error %s\n", __FUNCTION__, T2_CACHE_FILE); | ||
| goto unlock; | ||
| } | ||
|
|
||
| FILE *fp = fdopen(cache_fd, "a"); | ||
| if (fp == NULL) | ||
| { | ||
| EVENT_ERROR("%s: fdopen error %s\n", __FUNCTION__, T2_CACHE_FILE); | ||
| close(cache_fd); | ||
| goto unlock; | ||
| } | ||
|
|
||
| fs = popen ("cat /tmp/t2_caching_file | wc -l", "r"); | ||
| if(fs != NULL) | ||
| { |
|
|
||
| if (is_for_us && header->data_length > 0) | ||
| { | ||
| char* marker_data = message + sizeof(T2MQMessageHeader); |
Check failure
Code scanning / CodeQL
Suspicious add with sizeof High
char *
Copilot Autofix
AI 23 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| EVENT_ERROR("Message too large: %zu bytes\n", sizeof(T2MQMessageHeader) + data_len); | ||
| return T2ERROR_FAILURE; | ||
| } | ||
| memcpy(message + sizeof(T2MQMessageHeader), data, data_len); |
Check failure
Code scanning / CodeQL
Suspicious add with sizeof High
char *
Copilot Autofix
AI 23 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
Signed-off-by: Yogeswaran K <yogeswaran_k@comcast.com>
Signed-off-by: Yogeswaran K <yogeswaran_k@comcast.com>
| T2Info("No markers found for component: %s\n", query_component); | ||
| strcpy(marker_response, ""); // Empty response | ||
| } | ||
| return strdup(marker_response); |
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.
Coverity Issue - Resource leak
Variable "eventMarkerList" going out of scope leaks the storage it points to.
High Impact, CWE-404
RESOURCE_LEAK
| { | ||
| T2Error("Failed to send marker response header to client %d\n", client_index); | ||
| t2_cleanup_tcp_client(client_index); | ||
| return; |
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.
Coverity issue no longer present as of: undefined
Show issue
Coverity Issue - Resource leak
Variable "eventMarkerList" going out of scope leaks the storage it points to.
High Impact, CWE-404
RESOURCE_LEAK
| client_index, component_name); | ||
|
|
||
| T2Debug("%s --out\n", __FUNCTION__); | ||
| } |
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.
Coverity issue no longer present as of: undefined
Show issue
Coverity Issue - Resource leak
Variable "eventMarkerList" going out of scope leaks the storage it points to.
High Impact, CWE-404
RESOURCE_LEAK
| { | ||
| pthread_mutex_lock(&clientMarkerMutex); | ||
| hash_map_destroy(clientEventMarkerMap, free); | ||
| clientEventMarkerMap = 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.
Coverity Issue - Check of thread-shared field evades lock acquisition
Thread1 sets "clientEventMarkerMap" to a new value. Now the two threads have an inconsistent view of "clientEventMarkerMap" and updates to fields of "clientEventMarkerMap" or fields correlated with "clientEventMarkerMap" may be lost.
High Impact, CWE-543
LOCK_EVASION
How to fix
Guard the modification of "clientEventMarkerMap" and the read used to decide whether to modify "clientEventMarkerMap" with the same set of locks.
| char* marker_data = NULL; | ||
| if (resp_header.data_length > 0) | ||
| { | ||
| marker_data = malloc(resp_header.data_length + 1); |
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.
Coverity Issue - Untrusted allocation size
Passing tainted expression "resp_header.data_length + 1U" to "malloc", which uses it as an allocation size.
Medium Impact, CWE-789
TAINTED_SCALAR
How to fix
Ensure that tainted values are properly sanitized, by checking that their values are within a permissible range.
|
|
||
| struct timeval timeout = {.tv_sec = 10, .tv_usec = 0}; | ||
| setsockopt(g_tcp_client_fd, SOL_SOCKET, SO_RCVTIMEO, &timeout, sizeof(timeout)); | ||
| setsockopt(g_tcp_client_fd, SOL_SOCKET, SO_SNDTIMEO, &timeout, sizeof(timeout)); |
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.
Coverity Issue - Unchecked return value from library
Calling "setsockopt(g_tcp_client_fd, 1, 21, &timeout, 16U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| header->component_name[sizeof(header->component_name) - 1] = '\0'; | ||
|
|
||
| // Copy marker data | ||
| memcpy(message + sizeof(T2MQMessageHeader), marker_list, strlen(marker_list)); |
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.
Coverity Issue - Copy into fixed size buffer
You might overrun the 4096-character fixed-size string "message + 152UL" by copying "marker_list" without checking the length.
Low Impact, CWE-120
STRING_OVERFLOW
| header->component_name[sizeof(header->component_name) - 1] = '\0'; | ||
|
|
||
| // Copy marker data | ||
| memcpy(message + sizeof(T2MQMessageHeader), marker_list, strlen(marker_list)); |
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.
Coverity Issue - Copy into fixed size buffer
You might overrun the 4096-character fixed-size string "message + 152UL" by copying "marker_list" without checking the length.
Low Impact, CWE-120
STRING_OVERFLOW
| break; | ||
|
|
||
| case T2_MSG_EVENT_DATA: | ||
| t2_handle_event_data(client_index, &req_header); |
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.
Coverity issue no longer present as of: undefined
Show issue
Coverity Issue - Untrusted allocation size
Passing tainted expression "req_header.data_length" to "t2_handle_event_data", which uses it as an allocation size.
Medium Impact, CWE-789
TAINTED_SCALAR
How to fix
Ensure that tainted values are properly sanitized, by checking that their values are within a permissible range.
| */ | ||
| static void t2_mq_check_marker_updates(void) | ||
| { | ||
| if (!g_mq_state.initialized || g_mq_state.broadcast_mq == -1) |
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.
Coverity issue no longer present as of: undefined
Show issue
Coverity Issue - Data race condition
Accessing "g_mq_state.broadcast_mq" without holding lock "g_mq_mutex". Elsewhere, "g_mq_state.broadcast_mq" is written to with "g_mq_mutex" held 2 out of 2 times.
Medium Impact, CWE-366
MISSING_LOCK
Signed-off-by: Yogeswaran K <yogeswaran_k@comcast.com>
Signed-off-by: Yogeswaran K <yogeswaran_k@comcast.com>
Signed-off-by: Yogeswaran K <yogeswaran_k@comcast.com>
Signed-off-by: Yogeswaran K <yogeswaran_k@comcast.com>
Signed-off-by: Yogeswaran K <yogeswaran_k@comcast.com>
Signed-off-by: Yogeswaran K <yogeswaran_k@comcast.com>
| sev.sigev_signo = SIGUSR1; | ||
| sev.sigev_value.sival_ptr = NULL; | ||
|
|
||
| if (mq_notify(g_mq_state.broadcast_mq, &sev) == -1) { |
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.
Coverity Issue - Uninitialized scalar variable
Using uninitialized value "sev". Field "sev._sigev_un" is uninitialized when calling "mq_notify".
High Impact, CWE-457
UNINIT
Signed-off-by: Yogeswaran K <yogeswaran_k@comcast.com>
| sev.sigev_signo = SIGUSR1; | ||
| sev.sigev_value.sival_ptr = &g_daemon_mq_state; | ||
|
|
||
| if (mq_notify(g_daemon_mq_state.daemon_mq, &sev) == -1) |
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.
Coverity Issue - Data race condition
Accessing "g_daemon_mq_state.daemon_mq" without holding lock "g_daemon_mq_mutex". Elsewhere, "g_daemon_mq_state.daemon_mq" is written to with "g_daemon_mq_mutex" held 2 out of 2 times.
Medium Impact, CWE-366
MISSING_LOCK
| char message[T2_MQ_MAX_MSG_SIZE]; | ||
| ssize_t msg_size; | ||
|
|
||
| if (!g_daemon_mq_state.initialized || g_daemon_mq_state.daemon_mq == -1) |
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.
Coverity Issue - Data race condition
Accessing "g_daemon_mq_state.daemon_mq" without holding lock "g_daemon_mq_mutex". Elsewhere, "g_daemon_mq_state.daemon_mq" is written to with "g_daemon_mq_mutex" held 2 out of 2 times.
Medium Impact, CWE-366
MISSING_LOCK
| sev.sigev_signo = SIGUSR1; | ||
| sev.sigev_value.sival_ptr = &g_daemon_mq_state; | ||
|
|
||
| if (mq_notify(g_daemon_mq_state.daemon_mq, &sev) == -1) |
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.
Coverity Issue - Uninitialized scalar variable
Using uninitialized value "sev". Field "sev._sigev_un" is uninitialized when calling "mq_notify".
High Impact, CWE-457
UNINIT
No description provided.