From ca9678228611839583631fc7ca996fe52834fbff Mon Sep 17 00:00:00 2001 From: Ivan Polyakov Date: Sun, 23 Mar 2025 21:34:46 +0300 Subject: [PATCH] Improvements in memory management Invalid writes were detected by Valgrind and fixed. Cleaning and freeing of Response, Request and Keyval structures has been improved by adding *_free functions, which frees blocks of memory after calling *_cleanup. This helps avoid confusion when you want to completely clean memory, and *_cleanup functions only _clean_ data without _freeing_ memory blocks. _Cleaning_ to reuse already allocated memory blocks later, but without data, is still available in *_cleanup functions. --- c/keyval.c | 36 +++++++++++++++++++++---------- c/query.c | 10 ++++----- c/request.c | 39 +++++++++++++++++++++++----------- c/response.c | 53 ++++++++++++++++++++++++++-------------------- c/url.c | 11 ++++++---- c/utils.c | 2 +- config.mk | 2 +- include/keyval.h | 22 ++++++++++++++++++- include/request.h | 20 +++++++++++++---- include/response.h | 14 +++++++----- servers/fcgi.c | 21 ++++++++++-------- 11 files changed, 154 insertions(+), 76 deletions(-) diff --git a/c/keyval.c b/c/keyval.c index 41ae2f8..0539691 100644 --- a/c/keyval.c +++ b/c/keyval.c @@ -6,16 +6,6 @@ #include #include -/*! - * \brief Reallocate key-value storage with new capacity. - * - * \param keyval Key-value storage. - * \param capacity New capacity. - * - * \return Status code. - */ -static int rpd_keyval_realloc(rpd_keyval *keyval, int capacity); - int rpd_keyval_init(rpd_keyval *keyval, int capacity) { keyval->size = keyval->capacity = 0; @@ -31,6 +21,11 @@ int rpd_keyval_init(rpd_keyval *keyval, int capacity) return 1; } + for (int i = 0; i < capacity; i++) { + keyval->items[i].key = NULL; + keyval->items[i].val = NULL; + } + keyval->capacity = capacity; return 0; } @@ -122,12 +117,31 @@ void rpd_keyval_cleanup(rpd_keyval *keyval) keyval->size = 0; } -static int rpd_keyval_realloc(rpd_keyval *keyval, int capacity) +void rpd_keyval_free(rpd_keyval *keyval) +{ + rpd_keyval_cleanup(keyval); + + if (keyval->capacity) { + free(keyval->items); + keyval->capacity = 0; + } +} + +int rpd_keyval_realloc(rpd_keyval *keyval, int capacity) { + if (capacity <= keyval->capacity) { + return 0; + } + keyval->items = realloc(keyval->items, sizeof(rpd_keyval_item) * capacity); if (!keyval->items) { return 1; } + + for (; keyval->capacity < capacity; keyval->capacity++) { + keyval->items[keyval->capacity].key = keyval->items[keyval->capacity].val = NULL; + } + keyval->capacity = capacity; return 0; } diff --git a/c/query.c b/c/query.c index 7515bd8..424387c 100644 --- a/c/query.c +++ b/c/query.c @@ -21,10 +21,10 @@ int rpd_query_parse(rpd_keyval *dest, const char *src) if (!query) return 1; - dest->items = realloc(dest->items, sizeof(rpd_keyval_item *) * len); - if (!dest->items) - return 2; - dest->capacity = len; + if (dest->capacity < len) { + if (rpd_keyval_realloc(dest, len)) + return 2; + } char *tokens = query, *p = query; int i = 0; @@ -45,7 +45,7 @@ int rpd_query_parse(rpd_keyval *dest, const char *src) dest->size = i; dest->capacity = i; - free(query); + // free(query); // FIXME: query changed in rpd_strsep and fails on free return 0; } diff --git a/c/request.c b/c/request.c index c1d45c5..89b5c4f 100644 --- a/c/request.c +++ b/c/request.c @@ -5,6 +5,20 @@ #include #include +void rpd_req_init(rpd_req *dest) +{ + dest->method = GET; + + rpd_keyval_init(&dest->headers, 0); + rpd_keyval_init(&dest->query, 0); + rpd_keyval_init(&dest->params, 0); + + dest->path.parts_len = 0; + dest->path.parts = NULL; + + dest->body = NULL; +} + enum rpd_req_methods rpd_req_smethod(const char *method) { if (!strcmp(method, "GET")) @@ -30,22 +44,23 @@ enum rpd_req_methods rpd_req_smethod(const char *method) void rpd_req_cleanup(rpd_req *req) { - rpd_keyval_cleanup(&req->params); - if (req->body) { free(req->body); req->body = NULL; } - if (req->query.size) { - rpd_keyval_cleanup(&req->query); - } + rpd_url_cleanup(&req->path); - if (req->params.size) { - for (int i = 0; i < req->params.size; i++) { - rpd_keyval_item *item = req->params.items + i; - free(item->val); - item->val = NULL; - } - } + rpd_keyval_cleanup(&req->headers); + rpd_keyval_cleanup(&req->query); + rpd_keyval_cleanup(&req->params); +} + +void rpd_req_free(rpd_req *req) +{ + rpd_req_cleanup(req); + + rpd_keyval_free(&req->headers); + rpd_keyval_free(&req->query); + rpd_keyval_free(&req->params); } diff --git a/c/response.c b/c/response.c index 4125582..22030a7 100644 --- a/c/response.c +++ b/c/response.c @@ -2,13 +2,14 @@ /* Copyright 2022 Ivan Polyakov */ #include "response.h" +#include #include #include #include -static size_t calc_res_status_sz(const rpd_res *res); +static size_t calc_res_status_len(const rpd_res *res); -static size_t calc_res_headers_sz(const rpd_keyval *res); +static size_t calc_res_headers_len(const rpd_keyval *res); void rpd_res_init(rpd_res *dest) { @@ -20,12 +21,12 @@ void rpd_res_init(rpd_res *dest) int rpd_res_headers_str(char **dest, const rpd_res *src) { - size_t size, i = 0; + size_t len, i = 0; char *ptr; - size = calc_res_headers_sz(&src->headers); + len = calc_res_headers_len(&src->headers); - *dest = (char *) malloc(sizeof(char) * size); + *dest = (char *) malloc(sizeof(char) * (len + 1)); if (!*dest) { perror("malloc"); return 1; @@ -46,53 +47,55 @@ int rpd_res_headers_str(char **dest, const rpd_res *src) int rpd_res_str(char **dest, const rpd_res *res) { - size_t headers_size, size; + size_t headers_len, len; char *ptr, *headers; - size = headers_size = calc_res_headers_sz(&res->headers); - size += calc_res_status_sz(res); + len = headers_len = calc_res_headers_len(&res->headers); + len += calc_res_status_len(res); + len += 2; /* CRLF */ if (res->body) - size += 2 + strlen(res->body); + len += strlen(res->body); - *dest = (char *) malloc(sizeof(char) * size); + *dest = (char *) malloc(sizeof(char) * (len + 1)); if (!*dest) { perror("malloc"); return 1; } - /* header */ + /* Status header */ ptr = *dest; ptr += sprintf(ptr, "Status: %d\r\n", res->status); + /* Headers */ if (rpd_res_headers_str(&headers, res)) { return 1; } - memcpy(ptr, headers, headers_size); + memcpy(ptr, headers, headers_len); free(headers); - ptr += headers_size; + ptr += headers_len; + + /* CRLF */ memcpy(ptr, "\r\n", 2); ptr += 2; - /* body */ + /* Content */ if (res->body) { int bodylen = strlen(res->body); memcpy(ptr, res->body, bodylen); ptr += bodylen; } + (*dest)[len] = *ptr = '\0'; return 0; } -static size_t calc_res_status_sz(const rpd_res *res) +static size_t calc_res_status_len(const rpd_res *res) { - size_t size = 0; - size += strlen("Status: \r\n"); - size += 3; /* plus status code */ - return size; + return strlen("Status: 000\r\n"); } -static size_t calc_res_headers_sz(const rpd_keyval *headers) +static size_t calc_res_headers_len(const rpd_keyval *headers) { size_t size = 0, i = 0; @@ -116,7 +119,11 @@ void rpd_res_cleanup(rpd_res *res) res->body = NULL; } - if (res->headers.capacity) { - rpd_keyval_cleanup(&res->headers); - } + rpd_keyval_cleanup(&res->headers); +} + +void rpd_res_free(rpd_res *res) +{ + rpd_res_cleanup(res); + rpd_keyval_free(&res->headers); } diff --git a/c/url.c b/c/url.c index 9cf99cf..7ca89d3 100644 --- a/c/url.c +++ b/c/url.c @@ -38,12 +38,15 @@ int rpd_url_parse(rpd_url *dest, const char *src) void rpd_url_cleanup(rpd_url *url) { - int i = 0; + register int i = 0; while (i < url->parts_len) { - free(url->parts[i]); - i++; + free(url->parts[i++]); + } + + if (url->parts) { + free(url->parts); } - free(url->parts); + url->parts = NULL; url->parts_len = 0; } diff --git a/c/utils.c b/c/utils.c index 0df63b3..11014f2 100644 --- a/c/utils.c +++ b/c/utils.c @@ -76,7 +76,7 @@ const char *rpd_splitbyc(char **dest1, char **dest2, const char *src, const char return NULL; } memcpy(*dest2, end, len); - (*dest2)[len + 1] = '\0'; + (*dest2)[len] = '\0'; return 0; } end++; diff --git a/config.mk b/config.mk index e620672..2f0da2b 100644 --- a/config.mk +++ b/config.mk @@ -1,4 +1,4 @@ -VERSION=0.4.1 +VERSION=0.4.2 #arg Installation prefix PREFIX=/usr/local diff --git a/include/keyval.h b/include/keyval.h index a532019..0f241de 100644 --- a/include/keyval.h +++ b/include/keyval.h @@ -44,6 +44,16 @@ typedef struct { */ int rpd_keyval_init(rpd_keyval *keyval, int capacity); +/*! + * \brief Reallocate key-value storage with new capacity. + * + * \param keyval Key-value storage. + * \param capacity New capacity. + * + * \return Status code. + */ +int rpd_keyval_realloc(rpd_keyval *keyval, int capacity); + /*! * \brief Insert new key-value pair into the storage. * @@ -87,7 +97,7 @@ rpd_keyval_item *rpd_keyval_find(const rpd_keyval *keyval, const char *key); rpd_keyval_item **rpd_keyval_findall(const rpd_keyval *keyval, const char *key); /*! - * \brief Free key-val pairs and reset. + * \brief Cleanup key-val pairs and reset. * * This function don't will free rpd_keyval::items memory, * so capacity will be saved. @@ -101,6 +111,16 @@ rpd_keyval_item **rpd_keyval_findall(const rpd_keyval *keyval, const char *key); */ void rpd_keyval_cleanup(rpd_keyval *keyval); +/*! + * \brief Free key-val pairs and reset. + * + * This function is the same as rpd_keyval_cleanup, but it's + * also free the rpd_keyval::items memory, so capacity after that is 0. + * + * \param keyval Key-value storage. + */ +void rpd_keyval_free(rpd_keyval *keyval); + #ifdef __cplusplus } #endif diff --git a/include/request.h b/include/request.h index 5e1cb14..227f97e 100644 --- a/include/request.h +++ b/include/request.h @@ -44,7 +44,14 @@ typedef struct { } rpd_req; /*! - * \brief Parser string request method to enumeration value. + * \brief Fill request structure with initial values. + * + * \param dest Structure to fill. + */ +void rpd_req_init(rpd_req *dest); + +/*! + * \brief Parse string request method to enumeration value. * * \param method Request method string. * @@ -53,14 +60,19 @@ typedef struct { enum rpd_req_methods rpd_req_smethod(const char *method); /*! - * \brief Cleans request data. - * - * Request instance will not be freed, only it's data. + * \brief Clean request data. * * \param req Request instance. */ void rpd_req_cleanup(rpd_req *req); +/*! + * \brief Free request data memory. + * + * \param req Request instance. + */ +void rpd_req_free(rpd_req *req); + #ifdef __cplusplus } #endif diff --git a/include/response.h b/include/response.h index 3cd949a..c0ae646 100644 --- a/include/response.h +++ b/include/response.h @@ -118,15 +118,19 @@ int rpd_res_headers_str(char **dest, const rpd_res *src); int rpd_res_str(char **dest, const rpd_res *src); /*! - * \brief Cleans response instance. + * \brief Clean response data. * - * This function will not frees response instance, - * only it's data. - * - * \param res Response instance to cleanup. + * \param res Response instance. */ void rpd_res_cleanup(rpd_res *res); +/*! + * \brief Free response data memory. + * + * \param res Response instance. + */ +void rpd_res_free(rpd_res *res); + #ifdef __cplusplus } #endif diff --git a/servers/fcgi.c b/servers/fcgi.c index e8dec1b..37572a5 100644 --- a/servers/fcgi.c +++ b/servers/fcgi.c @@ -4,6 +4,7 @@ #include "../include/servers/fcgi.h" #include "../c/utils.h" #include +#include #include #include #include @@ -81,6 +82,8 @@ int rpd_fcgi_server_start(rpd_app *app, const char *sock_path) listen_requests((void *) &data); #endif + FCGX_ShutdownPending(); + return 0; } @@ -131,14 +134,16 @@ static void handle_request(rpd_app *app, FCGX_Request *fcgx_req) req = (rpd_req *) malloc(sizeof(rpd_req)); res = (rpd_res *) malloc(sizeof(rpd_res)); - fcgx_to_rpd_req(req, fcgx_req); + rpd_req_init(req); rpd_res_init(res); + fcgx_to_rpd_req(req, fcgx_req); + rpd_app_handle_request(app, req, res); send_response(res, fcgx_req->out); - rpd_req_cleanup(req); - rpd_res_cleanup(res); + rpd_req_free(req); + rpd_res_free(res); free(req); free(res); } @@ -163,8 +168,6 @@ static int fcgx_to_rpd_req(rpd_req *dest, FCGX_Request *req) rpd_url_parse(&dest->path, FCGX_GetParam("DOCUMENT_URI", req->envp)); // dest->auth = FCGX_GetParam("HTTP_AUTHORIZATION", req->envp); // dest->cookie = FCGX_GetParam("HTTP_COOKIE", req->envp); - rpd_keyval_init(&dest->params, 0); - rpd_keyval_init(&dest->headers, 0); dest->headers.unique = 0; char **env = req->envp; @@ -210,15 +213,17 @@ static int fcgx_to_rpd_req(rpd_req *dest, FCGX_Request *req) static int read_fcgx_req_body(char **dest, FCGX_Request *req) { char *clen = FCGX_GetParam("CONTENT_LENGTH", req->envp); - if (!clen) + if (!clen) { return 1; + } + size_t len = atoll(clen); *dest = (char *) malloc(sizeof(char) * (len + 1)); if (!*dest) return 2; - *dest[len] = '\0'; + (*dest)[len] = '\0'; FCGX_GetStr(*dest, len, req->in); return 0; @@ -226,8 +231,6 @@ static int read_fcgx_req_body(char **dest, FCGX_Request *req) static int env_to_req_header(char **key, char **val, const char *env) { - const char *ptr = NULL; - rpd_splitbyc(key, val, env, '='); if (!*key || !*val) { if (*key)