From 97745865085b025efd6009a47c69c476d5c9a3cb Mon Sep 17 00:00:00 2001 From: Dengfeng Liu Date: Mon, 17 Jul 2023 21:33:00 +0800 Subject: [PATCH] fix some memory leak Signed-off-by: Dengfeng Liu --- src/auth.c | 46 +++++++++++++++++++-------------- src/centralserver.c | 62 ++++++++++++++++++++++++++++++++++++--------- src/client_list.c | 2 +- src/firewall.c | 13 +++++++--- src/fw_iptables.c | 1 + src/http.c | 31 ++++++++++++++++------- src/http.h | 2 +- src/wd_util.c | 4 +-- 8 files changed, 114 insertions(+), 47 deletions(-) diff --git a/src/auth.c b/src/auth.c index e71ca32b9..27d7d1631 100644 --- a/src/auth.c +++ b/src/auth.c @@ -71,10 +71,6 @@ client_timeout_check_cb(evutil_socket_t fd, short event, void *arg) { /** * @brief Logout a client and report to auth server. * - * This function assumes it is being called with the client lock held! This - * function remove the client from the client list and free its memory, so - * client is no longer valid when this method returns. - * * @param context Points to request context * @param client Points to the client to be logged out; the client will be free * in this function @@ -84,21 +80,22 @@ void ev_logout_client(struct wd_request_context *context, t_client *client) { assert(client); - - fw_deny(client); - - LOCK_CLIENT_LIST(); - client_list_remove(client); - UNLOCK_CLIENT_LIST(); char *uri = get_auth_uri(REQUEST_TYPE_LOGOUT, ONLINE_CLIENT, client); - client_free_node(client); - if (!uri) return; - + if (!uri) { + return; + } + struct evhttp_connection *evcon = NULL; - struct evhttp_request *req = NULL; - wd_make_request(context, &evcon, &req, process_auth_server_logout); - evhttp_make_request(evcon, req, EVHTTP_REQ_GET, uri); + struct evhttp_request *req = NULL; + assert(context->data == NULL); + context->data = client; + if (!wd_make_request(context, &evcon, &req, process_auth_server_logout)) + evhttp_make_request(evcon, req, EVHTTP_REQ_GET, uri); + else { + debug(LOG_ERR, "wd_make_request failed"); + context->data = NULL; + } free(uri); } @@ -117,16 +114,27 @@ ev_authenticate_client(struct evhttp_request *req, struct wd_request_context *context, t_client *client) { char *uri = get_auth_uri(REQUEST_TYPE_LOGIN, ONLINE_CLIENT, client); - if (!uri) return; + if (!uri) { + debug(LOG_ERR, "get_auth_uri failed"); + evhttp_send_error(req, HTTP_INTERNAL, "Internal Server Error"); + safe_client_list_delete(client); + return; + } debug(LOG_DEBUG, "client login request [%s]", uri); struct evhttp_connection *wd_evcon = NULL; struct evhttp_request *wd_req = NULL; + assert(context->data == NULL); context->data = client; context->clt_req = req; - wd_make_request(context, &wd_evcon, &wd_req, process_auth_server_login); - evhttp_make_request(wd_evcon, wd_req, EVHTTP_REQ_GET, uri); + if (!wd_make_request(context, &wd_evcon, &wd_req, process_auth_server_login)) { + evhttp_make_request(wd_evcon, wd_req, EVHTTP_REQ_GET, uri); + } else { + debug(LOG_ERR, "wd_make_request failed"); + evhttp_send_error(req, HTTP_INTERNAL, "Internal Server Error"); + safe_client_list_delete(client); + } free(uri); } diff --git a/src/centralserver.c b/src/centralserver.c index f698be7c9..e6d92f3eb 100755 --- a/src/centralserver.c +++ b/src/centralserver.c @@ -139,14 +139,13 @@ make_roam_request(struct wd_request_context *context, struct roam_req_info *roam } static void -process_auth_server_login2(struct evhttp_request *req, void *ctx) +process_auth_server_login_v2(struct evhttp_request *req, void *ctx) { auth_req_info *auth = ((struct wd_request_context *)ctx)->data; debug(LOG_DEBUG, "process auth server login2 response"); char buffer[MAX_BUF] = {0}; - if (evbuffer_remove(evhttp_request_get_input_buffer(req), buffer, MAX_BUF-1) > 0 ) { - } else { + if (evbuffer_remove(evhttp_request_get_input_buffer(req), buffer, MAX_BUF-1) <= 0 ) { free(auth); return; } @@ -177,7 +176,7 @@ process_auth_server_login2(struct evhttp_request *req, void *ctx) * */ static char * -get_login2_request_uri(s_config *config, t_auth_serv *auth_server, const auth_req_info *auth) +get_login_v2_request_uri(s_config *config, t_auth_serv *auth_server, const auth_req_info *auth) { char *login2_uri = NULL; safe_asprintf(&login2_uri, "%slogin2?gw_id=%s&gw_address=%s&gw_port=%d&mac=%s&channel_path=%s&cltIp=%s", @@ -200,14 +199,21 @@ get_login2_request_uri(s_config *config, t_auth_serv *auth_server, const auth_re void make_auth_request(struct wd_request_context *context, auth_req_info *auth) { - char *uri = get_login2_request_uri(config_get_config(), get_auth_server(), auth); + char *uri = get_login_v2_request_uri(config_get_config(), get_auth_server(), auth); + if (!uri) { + free(auth); + return; + } debug(LOG_DEBUG, "login2 request uri [%s]", uri); struct evhttp_connection *evcon = NULL; struct evhttp_request *req = NULL; - context->data = auth; - wd_make_request(context, &evcon, &req, process_auth_server_login2); - evhttp_make_request(evcon, req, EVHTTP_REQ_GET, uri); + context->data = auth; + if (!wd_make_request(context, &evcon, &req, process_auth_server_login_v2)) { + evhttp_make_request(evcon, req, EVHTTP_REQ_GET, uri); + } else { + free(auth); + } free(uri); } @@ -351,7 +357,18 @@ process_auth_server_logout(struct evhttp_request *req, void *ctx) { t_authresponse authresponse; memset(&authresponse, 0, sizeof(t_authresponse)); - parse_auth_server_response(&authresponse, req); + if (parse_auth_server_response(&authresponse, req)) { + if (authresponse.authcode == 0) { + // get client from ctx + t_client *client = (t_client *)((struct wd_request_context *)ctx)->data; + fw_deny(client); + debug(LOG_INFO, "Client %s logged out successfully", client->ip); + safe_client_list_delete(client); + } + } else { + debug(LOG_ERR, "parse_auth_server_response failed"); + } + ((struct wd_request_context *)ctx)->data = NULL; } /** @@ -367,10 +384,16 @@ client_login_request_reply(t_authresponse *authresponse, struct evhttp_request *req, struct wd_request_context *context) { t_client *client = (t_client *)context->data; + context->data = NULL; t_auth_serv *auth_server = get_auth_server(); char *url_fragment = NULL; - if (!req) return; + if (!req) { + debug(LOG_ERR, "Got NULL request in client_login_request_reply"); + evhttp_send_error(req, 200, "Internal Error, We can not validate your request at this time"); + safe_client_list_delete(client); + return; + } switch (authresponse->authcode) { case AUTH_ERROR: @@ -464,6 +487,14 @@ process_auth_server_login(struct evhttp_request *req, void *ctx) memset(&authresponse, 0, sizeof(t_authresponse)); if (parse_auth_server_response(&authresponse, req)) client_login_request_reply(&authresponse, ((struct wd_request_context *)ctx)->clt_req, ctx); + else { + // free client in ctx + t_client *p1 = (t_client *)((struct wd_request_context *)ctx)->data; + debug(LOG_ERR, "parse_auth_server_response failed, free client %s", p1->ip); + safe_client_list_delete(p1); + evhttp_send_error(((struct wd_request_context *)ctx)->clt_req, 200, + "Internal Error, We can not validate your request at this time"); + } } /** @@ -476,6 +507,7 @@ client_counter_request_reply(t_authresponse *authresponse, { s_config *config = config_get_config(); t_client *p1 = (t_client *)context->data; + context->data = NULL; time_t current_time = time(NULL); debug(LOG_DEBUG, @@ -520,6 +552,13 @@ process_auth_server_counter(struct evhttp_request *req, void *ctx) memset(&authresponse, 0, sizeof(t_authresponse)); if (parse_auth_server_response(&authresponse, req)) client_counter_request_reply(&authresponse, req, ctx); + else { + // free client in ctx + t_client *p1 = (t_client *)((struct wd_request_context *)ctx)->data; + debug(LOG_ERR, "parse_auth_server_response failed, free client %s", p1->ip); + client_free_node(p1); + ((struct wd_request_context *)ctx)->data = NULL; + } } /** @@ -567,8 +606,7 @@ client_counter_request_reply_v2(t_authresponse *authresponse, /* Timing out user */ debug(LOG_DEBUG, "%s - Inactive for more than %ld seconds, removing client and denying in firewall", p1->ip, config->checkinterval * config->clienttimeout); - ev_logout_client(context, p1); - + ev_logout_client(context, p1); } else { UNLOCK_CLIENT_LIST(); /* diff --git a/src/client_list.c b/src/client_list.c index cdb920f11..63d1b9849 100644 --- a/src/client_list.c +++ b/src/client_list.c @@ -153,7 +153,7 @@ offline_client_list_insert_client(t_offline_client *client) t_client * client_list_add(const char *ip, const char *mac, const char *token) { - t_client *curclient; + t_client *curclient; curclient = client_get_new(); diff --git a/src/firewall.c b/src/firewall.c index 0a2e551fe..4c762a8ce 100644 --- a/src/firewall.c +++ b/src/firewall.c @@ -428,9 +428,10 @@ void fw_client_process_from_authserver_response(t_authresponse *authresponse, t_client *p1) { s_config *config = config_get_config(); + assert(p1 != NULL); LOCK_CLIENT_LIST(); - t_client *tmp_c = client_list_find_by_client(p1); + t_client *tmp_c = client_list_find_by_client_id(p1->id); if (!tmp_c) { UNLOCK_CLIENT_LIST(); debug(LOG_NOTICE, "Client was already removed. Skipping auth processing"); @@ -606,15 +607,21 @@ ev_fw_sync_with_authserver(struct wd_request_context *context) /* Update the counters on the remote server only if we have an auth server */ if (config->auth_servers != NULL) { char *uri = get_auth_uri(REQUEST_TYPE_COUNTERS, ONLINE_CLIENT, p1); - if (!uri) continue; + if (!uri) { + debug(LOG_ERR, "Could not get auth uri!"); + client_free_node(p1); + continue; + } debug(LOG_DEBUG, "client's counter uri [%s]", uri); struct evhttp_connection *evcon = NULL; struct evhttp_request *req = NULL; - context->data = p1; + context->data = p1; // free p1 in process_auth_server_counter if (!wd_make_request(context, &evcon, &req, process_auth_server_counter)) evhttp_make_request(evcon, req, EVHTTP_REQ_GET, uri); + else + client_free_node(p1); free(uri); } } diff --git a/src/fw_iptables.c b/src/fw_iptables.c index 9438d931d..0a94783e1 100755 --- a/src/fw_iptables.c +++ b/src/fw_iptables.c @@ -839,6 +839,7 @@ iptables_fw_save_online_clients() } f_fw_allow_close(); + client_list_destroy(sublist); #else #endif } diff --git a/src/http.c b/src/http.c index 0d5cafb2a..6eacabdd5 100644 --- a/src/http.c +++ b/src/http.c @@ -350,6 +350,10 @@ void ev_http_send_redirect(struct evhttp_request * req, const char *url, const char *text) { struct evbuffer *evb = evbuffer_new(); + if (!evb) { + evhttp_send_error(req, 500, "Internal error"); + return; + } struct evkeyvalq *header = evhttp_request_get_output_headers(req); evhttp_add_header(header, "Location", url); evbuffer_add_printf(evb, "Please click here.", url); @@ -368,7 +372,7 @@ void ev_http_callback_auth(struct evhttp_request *req, void *arg) { struct wd_request_context *context = (struct wd_request_context *)arg; - const char *token = ev_http_find_query(req, "token"); + char *token = ev_http_find_query(req, "token"); if (!token) { evhttp_send_error(req, 200, "Invalid token"); return; @@ -377,10 +381,9 @@ ev_http_callback_auth(struct evhttp_request *req, void *arg) char *remote_host; uint16_t port; evhttp_connection_get_peer(evhttp_request_get_connection(req), &remote_host, &port); - - const char *logout = ev_http_find_query(req, "logout"); char *mac = arp_get(remote_host); if (!mac) { + free(token); evhttp_send_error(req, 200, "Failed to retrieve your MAC address"); return; } @@ -401,12 +404,22 @@ ev_http_callback_auth(struct evhttp_request *req, void *arg) UNLOCK_CLIENT_LIST(); free(mac); - if (!new_client && logout) { - ev_logout_client(context, client); - } else if (!logout){ + char *logout = ev_http_find_query(req, "logout"); + if (logout) { + free(logout); + if (new_client) { + debug(LOG_INFO, "Logout request from %s, but client not found, impossible here!", client->ip); + safe_client_list_delete(client); + evhttp_send_error(req, 200, "Logout request from unknown client"); + } else { + debug(LOG_INFO, "Logout request from %s", client->ip); + ev_logout_client(context, client); + } + } else { + debug(LOG_INFO, "Login request from %s", client->ip); ev_authenticate_client(req, context, client); } - + free(token); } /** @@ -620,7 +633,7 @@ ev_http_replay_wisper(struct evhttp_request *req) * @param key The key to search * @return NULL or key's value, the return value need to be free by caller */ -const char * +char * ev_http_find_query(struct evhttp_request *req, const char *key) { const struct evhttp_uri *uri = evhttp_request_get_evhttp_uri(req); @@ -636,7 +649,7 @@ ev_http_find_query(struct evhttp_request *req, const char *key) if (evhttp_parse_query_str(evhttp_uri_get_query(uri), &query)) return NULL; - const char *r_val = NULL; + char *r_val = NULL; const char *val = evhttp_find_header(&query, key); if (val) { r_val = safe_strdup(val); diff --git a/src/http.h b/src/http.h index dd9f7f43c..266d32302 100644 --- a/src/http.h +++ b/src/http.h @@ -70,6 +70,6 @@ void ev_http_send_apple_redirect(struct evhttp_request *, const char *); void ev_http_replay_wisper(struct evhttp_request *); /** @brief get query's value according to key */ -const char *ev_http_find_query(struct evhttp_request *, const char *); +char *ev_http_find_query(struct evhttp_request *, const char *); #endif /* _HTTP_H_ */ diff --git a/src/wd_util.c b/src/wd_util.c index 04653acb0..b10ecb6ab 100644 --- a/src/wd_util.c +++ b/src/wd_util.c @@ -197,8 +197,8 @@ is_auth_online() if (!is_online()) { /* If we're not online auth is definately not online :) */ return (0); - } else if (last_auth_online_time == 0 - || (last_auth_offline_time - last_auth_online_time) >= (config_get_config()->checkinterval * 2)) { + } else if ((last_auth_online_time == 0) || + (last_auth_offline_time - last_auth_online_time) >= (config_get_config()->checkinterval * 2)) { /* Auth is probably offline */ return (0); } else {