From 64738a0f15ffb1c3c1ebf10dfb2bb6dd6dbcfc70 Mon Sep 17 00:00:00 2001 From: Paul Kocialkowski Date: Sun, 31 Mar 2013 12:53:15 +0200 Subject: ipc: Refactor code, check for NULL pointers and prevent memory leaks Signed-off-by: Paul Kocialkowski --- ipc.c | 316 +++++++++++++++++++++++++++++++++------------------------- samsung-ril.h | 2 + 2 files changed, 183 insertions(+), 135 deletions(-) diff --git a/ipc.c b/ipc.c index fe6c79f..78bf687 100644 --- 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, diff --git a/samsung-ril.h b/samsung-ril.h index 60aca04..0b14972 100644 --- a/samsung-ril.h +++ b/samsung-ril.h @@ -22,6 +22,8 @@ #ifndef _SAMSUNG_RIL_H_ #define _SAMSUNG_RIL_H_ +#include +#include #include #include -- cgit v1.1