ipc: Refactor code, check for NULL pointers and prevent memory leaks
authorPaul Kocialkowski <contact@paulk.fr>
Sun, 31 Mar 2013 10:53:15 +0000 (12:53 +0200)
committerPaul Kocialkowski <contact@paulk.fr>
Sun, 31 Mar 2013 10:53:15 +0000 (12:53 +0200)
Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
ipc.c
samsung-ril.h

diff --git a/ipc.c b/ipc.c
index fe6c79f..78bf687 100644 (file)
--- a/ipc.c
+++ b/ipc.c
@@ -40,19 +40,18 @@ void ipc_log_handler(const char *message, void *user_data)
 
 void ipc_fmt_send(const unsigned short command, const char type, unsigned char *data, const int length, unsigned char mseq)
 {
+       struct ipc_client_data *ipc_client_data;
        struct ipc_client *ipc_client;
 
-       if (ril_data.ipc_fmt_client == NULL) {
-               LOGE("ipc_fmt_client is null, aborting!");
+       if (ril_data.ipc_fmt_client == NULL || ril_data.ipc_fmt_client->data == NULL)
                return;
-       }
 
-       if (ril_data.ipc_fmt_client->data == NULL) {
-               LOGE("ipc_fmt_client data is null, aborting!");
+       ipc_client_data = (struct ipc_client_data *) ril_data.ipc_fmt_client->data;
+
+       if (ipc_client_data->ipc_client == NULL)
                return;
-       }
 
-       ipc_client = ((struct ipc_client_data *) ril_data.ipc_fmt_client->data)->ipc_client;
+       ipc_client = ipc_client_data->ipc_client;
 
        RIL_CLIENT_LOCK(ril_data.ipc_fmt_client);
        ipc_client_send(ipc_client, command, type, data, length, mseq);
@@ -61,42 +60,42 @@ void ipc_fmt_send(const unsigned short command, const char type, unsigned char *
 
 int ipc_fmt_read_loop(struct ril_client *client)
 {
-       struct ipc_message_info info;
+       struct ipc_client_data *ipc_client_data;
        struct ipc_client *ipc_client;
+
+       struct ipc_message_info info;
        int ipc_client_fd;
        fd_set fds;
 
-       if (client == NULL) {
-               LOGE("client is NULL, aborting!");
-               return -1;
-       }
+       if (client == NULL || client->data == NULL)
+               return -EINVAL;
 
-       if (client->data == NULL) {
-               LOGE("client data is NULL, aborting!");
-               return -1;
-       }
+       ipc_client_data = (struct ipc_client_data *) client->data;
 
-       ipc_client = ((struct ipc_client_data *) client->data)->ipc_client;
-       ipc_client_fd = ((struct ipc_client_data *) client->data)->ipc_client_fd;
+       if (ipc_client_data->ipc_client == NULL)
+               return -EINVAL;
 
-       FD_ZERO(&fds);
-       FD_SET(ipc_client_fd, &fds);
+       ipc_client = ipc_client_data->ipc_client;
+       ipc_client_fd = ipc_client_data->ipc_client_fd;
 
        while (1) {
-               memset(&info, 0, sizeof(info));
-
                if (ipc_client_fd < 0) {
-                       LOGE("IPC FMT client fd is negative, aborting!");
+                       LOGE("IPC FMT client fd is negative, aborting");
                        return -1;
                }
 
+               FD_ZERO(&fds);
+               FD_SET(ipc_client_fd, &fds);
+
                select(FD_SETSIZE, &fds, NULL, NULL, NULL);
 
                if (FD_ISSET(ipc_client_fd, &fds)) {
+                       memset(&info, 0, sizeof(info));
+
                        RIL_CLIENT_LOCK(client);
                        if (ipc_client_recv(ipc_client, &info) < 0) {
                                RIL_CLIENT_UNLOCK(client);
-                               LOGE("IPC FMT recv failed, aborting!");
+                               LOGE("IPC FMT recv failed, aborting");
                                return -1;
                        }
                        RIL_CLIENT_UNLOCK(client);
@@ -113,116 +112,140 @@ int ipc_fmt_read_loop(struct ril_client *client)
 
 int ipc_fmt_create(struct ril_client *client)
 {
-       struct ipc_client_data *client_data;
+       struct ipc_client_data *ipc_client_data;
        struct ipc_client *ipc_client;
+
        int ipc_client_fd;
        int rc;
 
-       client_data = malloc(sizeof(struct ipc_client_data));
-       memset(client_data, 0, sizeof(struct ipc_client_data));
-       client_data->ipc_client_fd = -1;
+       if (client == NULL)
+               return -EINVAL;
 
-       client->data = client_data;
+       LOGD("Creating new FMT client");
 
-       ipc_client = (struct ipc_client *) client_data->ipc_client;
+       ipc_client_data = (struct ipc_client_data *) calloc(1, sizeof(struct ipc_client_data));
+       ipc_client_data->ipc_client_fd = -1;
 
-       LOGD("Creating new FMT client");
-       ipc_client = ipc_client_new(IPC_CLIENT_TYPE_FMT);
+       client->data = (void *) ipc_client_data;
 
+       ipc_client = ipc_client_new(IPC_CLIENT_TYPE_FMT);
        if (ipc_client == NULL) {
-               LOGE("FMT client creation failed!");
-               return -1;
+               LOGE("FMT client creation failed");
+               goto error_client_create;
        }
 
-       client_data->ipc_client = ipc_client;
+       ipc_client_data->ipc_client = ipc_client;
 
        LOGD("Setting log handler");
-       rc = ipc_client_set_log_handler(ipc_client, ipc_log_handler, NULL);
 
+       rc = ipc_client_set_log_handler(ipc_client, ipc_log_handler, NULL);
        if (rc < 0) {
-               LOGE("Setting log handler failed!");
-               return -1;
+               LOGE("Setting log handler failed");
+               goto error_log_handler;
        }
 
        // ipc_client_set_handlers
 
        LOGD("Creating handlers common data");
-       rc = ipc_client_create_handlers_common_data(ipc_client);
 
+       rc = ipc_client_create_handlers_common_data(ipc_client);
        if (rc < 0) {
-               LOGE("Creating handlers common data failed!");
-               return -1;
+               LOGE("Creating handlers common data failed");
+               goto error_handlers_create;
        }
 
        LOGD("Starting modem bootstrap");
-       rc = ipc_client_bootstrap_modem(ipc_client);
 
+       rc = ipc_client_bootstrap_modem(ipc_client);
        if (rc < 0) {
-               LOGE("Modem bootstrap failed!");
-               return -1;
+               LOGE("Modem bootstrap failed");
+               goto error_bootstrap;
        }
 
        LOGD("Client open...");
-       rc = ipc_client_open(ipc_client);
 
+       rc = ipc_client_open(ipc_client);
        if (rc < 0) {
-               LOGE("%s: failed to open ipc client", __FUNCTION__);
-               return -1;
+               LOGE("%s: failed to open ipc client", __func__);
+               goto error_open;
        }
 
        LOGD("Obtaining ipc_client_fd");
-       ipc_client_fd = ipc_client_get_handlers_common_data_fd(ipc_client);
-       client_data->ipc_client_fd = ipc_client_fd;
 
+       ipc_client_fd = ipc_client_get_handlers_common_data_fd(ipc_client);
        if (ipc_client_fd < 0) {
-               LOGE("%s: client_fmt_fd is negative, aborting", __FUNCTION__);
-               return -1;
+               LOGE("%s: client_fmt_fd is negative, aborting", __func__);
+               goto error_get_fd;
        }
 
+       ipc_client_data->ipc_client_fd = ipc_client_fd;
+
        LOGD("Client power on...");
-       if (ipc_client_power_on(ipc_client)) {
-               LOGE("%s: failed to power on ipc client", __FUNCTION__);
-               return -1;
+
+       rc = ipc_client_power_on(ipc_client);
+       if (rc < 0) {
+               LOGE("%s: failed to power on ipc client", __func__);
+               goto error_power_on;
        }
 
        LOGD("IPC FMT client done");
 
        return 0;
+
+error:
+       ipc_client_power_off(ipc_client);
+
+error_power_on:
+error_get_fd:
+       ipc_client_close(ipc_client);
+
+error_open:
+error_bootstrap:
+       ipc_client_destroy_handlers_common_data(ipc_client);
+
+error_handlers_create:
+error_log_handler:
+       ipc_client_free(ipc_client);
+
+error_client_create:
+       ipc_client_data->ipc_client = NULL;
+       ipc_client_data->ipc_client_fd = -1;
+
+       free(ipc_client_data);
+       client->data = NULL;
+
+       return -1;
 }
 
 int ipc_fmt_destroy(struct ril_client *client)
 {
+       struct ipc_client_data *ipc_client_data;
        struct ipc_client *ipc_client;
-       int ipc_client_fd;
-       int rc;
-
-       LOGD("Destrying ipc fmt client");
 
-       if (client == NULL) {
-               LOGE("client was already destroyed");
-               return 0;
-       }
+       int rc;
 
-       if (client->data == NULL) {
-               LOGE("client data was already destroyed");
+       if (client == NULL || client->data == NULL) {
+               LOGE("Client was already destroyed");
                return 0;
        }
 
-       ipc_client_fd = ((struct ipc_client_data *) client->data)->ipc_client_fd;
-
-       if (ipc_client_fd)
-               close(ipc_client_fd);
+       ipc_client_data = (struct ipc_client_data *) client->data;
+       ipc_client = ipc_client_data->ipc_client;
 
-       ipc_client = ((struct ipc_client_data *) client->data)->ipc_client;
+       LOGD("Destroying ipc fmt client");
 
        if (ipc_client != NULL) {
-               ipc_client_destroy_handlers_common_data(ipc_client);
                ipc_client_power_off(ipc_client);
                ipc_client_close(ipc_client);
+               ipc_client_destroy_handlers_common_data(ipc_client);
                ipc_client_free(ipc_client);
        }
 
-       free(client->data);
+       ipc_client_data->ipc_client = NULL;
+       ipc_client_data->ipc_client_fd = -1;
+
+       free(ipc_client_data);
+       client->data = NULL;
 
        return 0;
 }
@@ -233,19 +256,18 @@ int ipc_fmt_destroy(struct ril_client *client)
 
 void ipc_rfs_send(const unsigned short command, unsigned char *data, const int length, unsigned char mseq)
 {
+       struct ipc_client_data *ipc_client_data;
        struct ipc_client *ipc_client;
 
-       if (ril_data.ipc_rfs_client == NULL) {
-               LOGE("ipc_rfs_client is null, aborting!");
+       if (ril_data.ipc_rfs_client == NULL || ril_data.ipc_rfs_client->data == NULL)
                return;
-       }
 
-       if (ril_data.ipc_rfs_client->data == NULL) {
-               LOGE("ipc_rfs_client data is null, aborting!");
+       ipc_client_data = (struct ipc_client_data *) ril_data.ipc_rfs_client->data;
+
+       if (ipc_client_data->ipc_client == NULL)
                return;
-       }
 
-       ipc_client = ((struct ipc_client_data *) ril_data.ipc_rfs_client->data)->ipc_client;
+       ipc_client = ipc_client_data->ipc_client;
 
        RIL_CLIENT_LOCK(ril_data.ipc_rfs_client);
        ipc_client_send(ipc_client, command, 0, data, length, mseq);
@@ -254,47 +276,49 @@ void ipc_rfs_send(const unsigned short command, unsigned char *data, const int l
 
 int ipc_rfs_read_loop(struct ril_client *client)
 {
-       struct ipc_message_info info;
+       struct ipc_client_data *ipc_client_data;
        struct ipc_client *ipc_client;
+
+       struct ipc_message_info info;
        int ipc_client_fd;
        fd_set fds;
 
-       if (client == NULL) {
-               LOGE("client is NULL, aborting!");
-               return -1;
-       }
+       if (client == NULL || client->data == NULL)
+               return -EINVAL;
 
-       if (client->data == NULL) {
-               LOGE("client data is NULL, aborting!");
-               return -1;
-       }
+       ipc_client_data = (struct ipc_client_data *) client->data;
 
-       ipc_client = ((struct ipc_client_data *) client->data)->ipc_client;
-       ipc_client_fd = ((struct ipc_client_data *) client->data)->ipc_client_fd;
+       if (ipc_client_data->ipc_client == NULL)
+               return -EINVAL;
 
-       FD_ZERO(&fds);
-       FD_SET(ipc_client_fd, &fds);
+       ipc_client = ipc_client_data->ipc_client;
+       ipc_client_fd = ipc_client_data->ipc_client_fd;
 
        while (1) {
                if (ipc_client_fd < 0) {
-                       LOGE("IPC RFS client fd is negative, aborting!");
+                       LOGE("IPC RFS client fd is negative, aborting");
                        return -1;
                }
 
+               FD_ZERO(&fds);
+               FD_SET(ipc_client_fd, &fds);
+
                select(FD_SETSIZE, &fds, NULL, NULL, NULL);
 
                if (FD_ISSET(ipc_client_fd, &fds)) {
+                       memset(&info, 0, sizeof(info));
+
                        RIL_CLIENT_LOCK(client);
                        if (ipc_client_recv(ipc_client, &info) < 0) {
                                RIL_CLIENT_UNLOCK(client);
-                               LOGE("IPC RFS recv failed, aborting!");
+                               LOGE("IPC RFS recv failed, aborting");
                                return -1;
                        }
                        RIL_CLIENT_UNLOCK(client);
 
                        ipc_rfs_dispatch(&info);
 
-                       if (info.data != NULL)
+                       if (info.data != NULL && info.length > 0)
                                free(info.data);
                }
        }
@@ -304,106 +328,128 @@ int ipc_rfs_read_loop(struct ril_client *client)
 
 int ipc_rfs_create(struct ril_client *client)
 {
-       struct ipc_client_data *client_data;
+       struct ipc_client_data *ipc_client_data;
        struct ipc_client *ipc_client;
+
        int ipc_client_fd;
        int rc;
 
-       client_data = malloc(sizeof(struct ipc_client_data));
-       memset(client_data, 0, sizeof(struct ipc_client_data));
-       client_data->ipc_client_fd = -1;
+       if (client == NULL)
+               return -EINVAL;
 
-       client->data = client_data;
+       LOGD("Creating new RFS client");
 
-       ipc_client = (struct ipc_client *) client_data->ipc_client;
+       ipc_client_data = (struct ipc_client_data *) calloc(1, sizeof(struct ipc_client_data));
+       ipc_client_data->ipc_client_fd = -1;
 
-       LOGD("Creating new RFS client");
-       ipc_client = ipc_client_new(IPC_CLIENT_TYPE_RFS);
+       client->data = (void *) ipc_client_data;
 
+       ipc_client = ipc_client_new(IPC_CLIENT_TYPE_RFS);
        if (ipc_client == NULL) {
-               LOGE("RFS client creation failed!");
-               return -1;
+               LOGE("RFS client creation failed");
+               goto error_client_create;
        }
 
-       client_data->ipc_client = ipc_client;
+       ipc_client_data->ipc_client = ipc_client;
 
        LOGD("Setting log handler");
-       rc = ipc_client_set_log_handler(ipc_client, ipc_log_handler, NULL);
 
+       rc = ipc_client_set_log_handler(ipc_client, ipc_log_handler, NULL);
        if (rc < 0) {
-               LOGE("Setting log handler failed!");
-               return -1;
+               LOGE("Setting log handler failed");
+               goto error_log_handler;
        }
 
        // ipc_client_set_handlers
 
        LOGD("Creating handlers common data");
-       rc = ipc_client_create_handlers_common_data(ipc_client);
 
+       rc = ipc_client_create_handlers_common_data(ipc_client);
        if (rc < 0) {
-               LOGE("Creating handlers common data failed!");
-               return -1;
+               LOGE("Creating handlers common data failed");
+               goto error_handlers_create;
        }
 
        LOGD("Client open...");
-       rc = ipc_client_open(ipc_client);
 
+       rc = ipc_client_open(ipc_client);
        if (rc < 0) {
-               LOGE("%s: failed to open ipc client", __FUNCTION__);
-               return -1;
+               LOGE("%s: failed to open ipc client", __func__);
+               goto error_open;
        }
 
        LOGD("Obtaining ipc_client_fd");
-       ipc_client_fd = ipc_client_get_handlers_common_data_fd(ipc_client);
-       client_data->ipc_client_fd = ipc_client_fd;
 
+       ipc_client_fd = ipc_client_get_handlers_common_data_fd(ipc_client);
        if (ipc_client_fd < 0) {
-               LOGE("%s: client_rfs_fd is negative, aborting", __FUNCTION__);
-               return -1;
+               LOGE("%s: client_rfs_fd is negative, aborting", __func__);
+               goto error_get_fd;
        }
 
+       ipc_client_data->ipc_client_fd = ipc_client_fd;
+
        LOGD("IPC RFS client done");
 
        return 0;
+
+error:
+error_get_fd:
+       ipc_client_close(ipc_client);
+
+error_open:
+       ipc_client_destroy_handlers_common_data(ipc_client);
+
+error_handlers_create:
+error_log_handler:
+       ipc_client_free(ipc_client);
+
+error_client_create:
+       ipc_client_data->ipc_client = NULL;
+       ipc_client_data->ipc_client_fd = -1;
+
+       free(ipc_client_data);
+       client->data = NULL;
+
+       return -1;
 }
 
 
 int ipc_rfs_destroy(struct ril_client *client)
 {
+       struct ipc_client_data *ipc_client_data;
        struct ipc_client *ipc_client;
-       int ipc_client_fd;
-       int rc;
 
-       LOGD("Destrying ipc rfs client");
-
-       if (client == NULL) {
-               LOGE("client was already destroyed");
-               return 0;
-       }
+       int rc;
 
-       if (client->data == NULL) {
-               LOGE("client data was already destroyed");
+       if (client == NULL || client->data == NULL) {
+               LOGE("Client was already destroyed");
                return 0;
        }
 
-       ipc_client_fd = ((struct ipc_client_data *) client->data)->ipc_client_fd;
-
-       if (ipc_client_fd)
-               close(ipc_client_fd);
+       ipc_client_data = (struct ipc_client_data *) client->data;
+       ipc_client = ipc_client_data->ipc_client;
 
-       ipc_client = ((struct ipc_client_data *) client->data)->ipc_client;
+       LOGD("Destroying ipc rfs client");
 
        if (ipc_client != NULL) {
-               ipc_client_destroy_handlers_common_data(ipc_client);
                ipc_client_close(ipc_client);
+               ipc_client_destroy_handlers_common_data(ipc_client);
                ipc_client_free(ipc_client);
        }
 
-       free(client->data);
+       ipc_client_data->ipc_client = NULL;
+       ipc_client_data->ipc_client_fd = -1;
+
+       free(ipc_client_data);
+       client->data = NULL;
 
        return 0;
 }
 
+/*
+ * IPC clients structures
+ */
+
 struct ril_client_funcs ipc_fmt_client_funcs = {
        .create = ipc_fmt_create,
        .destroy = ipc_fmt_destroy,
index 60aca04..0b14972 100644 (file)
@@ -22,6 +22,8 @@
 #ifndef _SAMSUNG_RIL_H_
 #define _SAMSUNG_RIL_H_
 
+#include <errno.h>
+#include <string.h>
 #include <pthread.h>
 
 #include <utils/Log.h>