Fix memory access issue in browser provider slots

[Model] COMMON
[BinType] AP
[Customer] OPEN

[Issue#] P170103-03175
[Request] PLM
[Occurrence Version] Latest

[Problem] Crash observed when we try to open youtube page in browser (Once crash)
[Cause & Measure] slot->client memory was accessed when it was getting free
[Checking Method] N/A

[Team] BrowserUI
[Developer] Vignesh Shanmuga Sunder (vgn.sunder)
[Solution company] Samsung
[Change Type] Apply Patch

Change-Id: I3384a1674f44417a76a34991c375e0a302894254
Signed-off-by: HyeKyoung Hwang <cookie@samsung.com>

diff --git a/provider/browser-provider-requests-manager.c b/provider/browser-provider-requests-manager.c
index 8b05fe2000f429fe45f916f5ddc3ad6b685aba95..8912c5ee9418902d1f67788ee7b0f082145e0a1a 100755 (executable)
--- a/provider/browser-provider-requests-manager.c
+++ b/provider/browser-provider-requests-manager.c
@@ -475,8 +475,7 @@ void *client_thread_idle(void *arg)
                FD_CLR(client->cmd_socket, &imask);
                FD_CLR(client->cmd_socket, &emask);
        }
-       bp_client_free(client);
-       slot->client = NULL;
+       bp_remove_client_from_slot(slot);
        return 0;
 }
 #endif
@@ -660,8 +659,7 @@ void bp_thread_requests_manager(bp_privates_defs *privates)
                                                        client->cmd_socket);
                                                if (client->cmd_socket >= 0)
                                                        FD_CLR(client->cmd_socket, &listen_fdset);
-                                               bp_client_free(client);
-                                               privates->slots[oldest_index].client = NULL;
+                                               bp_remove_client_from_slot(&privates->slots[oldest_index]);
                                                i = oldest_index;
                                        } else {
                                                TRACE_ERROR("[CRITICAL] No space in slots");
@@ -720,8 +718,7 @@ void bp_thread_requests_manager(bp_privates_defs *privates)
                                        TRACE_ERROR("client type");
                                        bp_ipc_send_errorcode(clientfd,
                                                BP_ERROR_PERMISSION_DENY);
-                                       bp_client_free(privates->slots[i].client);
-                                       privates->slots[i].client = NULL;
+                                       bp_remove_client_from_slot(&privates->slots[i]);
                                        continue;
                                } else if (!strcmp(privates->slots[i].client->privilege_label, "inhouse")) {
                                        // inhouse APIs (tab & scrap) have no privilege label.
@@ -734,8 +731,7 @@ void bp_thread_requests_manager(bp_privates_defs *privates)
                                                TRACE_ERROR("failed to create cynara client identification string");
                                                bp_ipc_send_errorcode(clientfd,
                                                        BP_ERROR_PERMISSION_DENY);
-                                               bp_client_free(privates->slots[i].client);
-                                               privates->slots[i].client = NULL;
+                                               bp_remove_client_from_slot(&privates->slots[i]);
                                                continue;
                                        }
                                        if(cynara_creds_socket_get_user(clientfd, USER_METHOD_UID, 
@@ -743,8 +739,7 @@ void bp_thread_requests_manager(bp_privates_defs *privates)
                                                TRACE_ERROR("failed to create cynara user identification string");
                                                bp_ipc_send_errorcode(clientfd,
                                                        BP_ERROR_PERMISSION_DENY);
-                                               bp_client_free(privates->slots[i].client);
-                                               privates->slots[i].client = NULL;
+                                               bp_remove_client_from_slot(&privates->slots[i]);
                                                continue;
                                        }
                                        pid_t cynara_pid;
@@ -752,16 +747,14 @@ void bp_thread_requests_manager(bp_privates_defs *privates)
                                                TRACE_ERROR("failed to create PID string for client session");
                                                bp_ipc_send_errorcode(clientfd,
                                                        BP_ERROR_PERMISSION_DENY);
-                                               bp_client_free(privates->slots[i].client);
-                                               privates->slots[i].client = NULL;
+                                               bp_remove_client_from_slot(&privates->slots[i]);
                                                continue;
                                        }
                                        if((privates->slots[i].client->cynara_session = cynara_session_from_pid(cynara_pid)) == NULL) {
                                                TRACE_ERROR("failed to create client session");
                                                bp_ipc_send_errorcode(clientfd,
                                                        BP_ERROR_PERMISSION_DENY);
-                                               bp_client_free(privates->slots[i].client);
-                                               privates->slots[i].client = NULL;
+                                               bp_remove_client_from_slot(&privates->slots[i]);
                                                continue;
                                        }
 
@@ -779,8 +772,7 @@ void bp_thread_requests_manager(bp_privates_defs *privates)
                                        TRACE_ERROR("client have no privilege permission");
                                        bp_ipc_send_errorcode(clientfd,
                                                BP_ERROR_PERMISSION_DENY);
-                                       bp_client_free(privates->slots[i].client);
-                                       privates->slots[i].client = NULL;
+                                       bp_remove_client_from_slot(&privates->slots[i]);
                                        continue;
                                }
 #endif
@@ -797,8 +789,7 @@ void bp_thread_requests_manager(bp_privates_defs *privates)
                                if (privates->slots[i].client->notify < 0) {
                                        TRACE_ERROR("failed to open fifo slot:%d", clientfd);
                                        bp_ipc_send_errorcode(clientfd, BP_ERROR_IO_ERROR);
-                                       bp_client_free(privates->slots[i].client);
-                                       privates->slots[i].client = NULL;
+                                       bp_remove_client_from_slot(&privates->slots[i]);
                                        continue;
                                }
 
@@ -817,8 +808,7 @@ void bp_thread_requests_manager(bp_privates_defs *privates)
                                        TRACE_ERROR("[ERROR][main_thread] create\n");
                                        bp_ipc_send_errorcode(clientfd,
                                                BP_ERROR_OUT_OF_MEMORY);
-                                       bp_client_free(privates->slots[i].client);
-                                       privates->slots[i].client = NULL;
+                                       bp_remove_client_from_slot(&privates->slots[i]);
                                        continue;
                                }
                                pthread_detach(privates->slots[i].client->tid);
@@ -842,8 +832,7 @@ void bp_thread_requests_manager(bp_privates_defs *privates)
                                continue;
                        if (privates->slots[i].client->cmd_socket < 0){
                                TRACE_ERROR("[CRITICAL] [i] Found Bad socket", i);
-                               bp_client_free(privates->slots[i].client);
-                               privates->slots[i].client = NULL;
+                               bp_remove_client_from_slot(&privates->slots[i]);
                                continue;
                        }
 
@@ -855,10 +844,9 @@ void bp_thread_requests_manager(bp_privates_defs *privates)
                                if (__handle_client_request
                                        (privates->slots[i].client) == BP_ERROR_IO_ERROR) {
                                        TRACE_ERROR("disconnect client slot:%d sock:%d", i,
-                                               client->cmd_socket);
-                                       FD_CLR(client->cmd_socket, &listen_fdset);
-                                       bp_client_free(client);
-                                       privates->slots[i].client = NULL;
+                                                       privates->slots[i].client->cmd_socket);
+                                       FD_CLR(privates->slots[i].client->cmd_socket, &listen_fdset);
+                                       bp_remove_client_from_slot(&privates->slots[i]);
                                }
                        } // FD_ISSET
                } // BP_MAX_CLIENT

diff --git a/provider/browser-provider-slots.c b/provider/browser-provider-slots.c
index bf095709badff8f81f01d3e6d325c3a0d8800d26..8427405fd3d74ec4778c277b30efd1d2f2986db3 100755 (executable)
--- a/provider/browser-provider-slots.c
+++ b/provider/browser-provider-slots.c
@@ -107,14 +107,24 @@ int bp_client_free(bp_client_defs *client)
        return 0;
 }
 
+int bp_remove_client_from_slot(bp_client_slots_defs *slot)
+{
+       if (slot == NULL || slot->client == NULL)
+               return -1;
+
+       // First set NULL and then free client
+       bp_client_defs *_client = slot->client;
+       slot->client = NULL;
+       return bp_client_free(_client);
+}
+
 void bp_client_slots_free(bp_client_slots_defs *slots, int size)
 {
        int i = 0;
        if (slots != NULL) {
                for (; i < size; i++) {
-                       if (slots[i].client == NULL)
-                               bp_client_free(slots[i].client);
-                       slots[i].client = NULL;
+                       if (slots[i].client != NULL)
+                               bp_remove_client_from_slot(&slots[i]);
                        pthread_mutex_destroy(&slots[i].mutex);
                }
                free(slots);

diff --git a/provider/include/browser-provider-slots.h b/provider/include/browser-provider-slots.h
index dda8dc82965e886f90b89108e6da82093861e404..ea610619bca350b364f9b9a47771e28761e82b73 100755 (executable)
--- a/provider/include/browser-provider-slots.h
+++ b/provider/include/browser-provider-slots.h
@@ -56,6 +56,7 @@ typedef struct {
 int bp_create_unique_id(void);
 bp_client_slots_defs *bp_client_slots_new(int size);
 int bp_client_free(bp_client_defs *client);
+int bp_remove_client_from_slot(bp_client_slots_defs *slot);
 void bp_client_slots_free(bp_client_slots_defs *slots, int size);
 
 #endif