From 3b412293c559c955bb2447cb24cb3ab27efb9ad9 Mon Sep 17 00:00:00 2001 From: Andreas Granig Date: Thu, 14 Jun 2018 15:49:11 +0200 Subject: [PATCH] db_redis: Fix memleaks on delete Make sure to release pkg memory on delete operations. Improve error handling to avoid segfault on broken connection. Warn on full table scans to help improve key definition. --- src/modules/db_redis/db_redis_mod.c | 8 +-- src/modules/db_redis/db_redis_mod.h | 2 +- src/modules/db_redis/redis_connection.c | 8 +++ src/modules/db_redis/redis_connection.h | 3 +- src/modules/db_redis/redis_dbase.c | 79 ++++++++++++++++--------- src/modules/db_redis/redis_dbase.h | 2 +- src/modules/db_redis/redis_table.c | 2 +- src/modules/db_redis/redis_table.h | 2 +- 8 files changed, 69 insertions(+), 37 deletions(-) diff --git a/src/modules/db_redis/db_redis_mod.c b/src/modules/db_redis/db_redis_mod.c index 9bc7862b892..2b3b07fead6 100644 --- a/src/modules/db_redis/db_redis_mod.c +++ b/src/modules/db_redis/db_redis_mod.c @@ -93,10 +93,10 @@ static int db_redis_bind_api(db_func_t *dbb) { int keys_param(modparam_t type, void *val) { - if (val == NULL) - return -1; - else - return db_redis_keys_spec((char*)val); + if (val == NULL) + return -1; + else + return db_redis_keys_spec((char*)val); } int mod_register(char *path, int *dlflags, void *p1, void *p2) { diff --git a/src/modules/db_redis/db_redis_mod.h b/src/modules/db_redis/db_redis_mod.h index e376885f5fd..dac420b0dfe 100644 --- a/src/modules/db_redis/db_redis_mod.h +++ b/src/modules/db_redis/db_redis_mod.h @@ -50,4 +50,4 @@ extern str redis_keys; extern str redis_schema_path; -#endif /* _DB_REDIS_MOD_H */ +#endif /* _DB_REDIS_MOD_H */ \ No newline at end of file diff --git a/src/modules/db_redis/redis_connection.c b/src/modules/db_redis/redis_connection.c index 794f5117140..62c63f0530b 100644 --- a/src/modules/db_redis/redis_connection.c +++ b/src/modules/db_redis/redis_connection.c @@ -403,3 +403,11 @@ void db_redis_consume_replies(km_redis_con_t *con) { db_redis_key_free(&query); } } + +const char *db_redis_get_error(km_redis_con_t *con) { + if (con && con->con && con->con->errstr) { + return con->con->errstr; + } else { + return ""; + } +} \ No newline at end of file diff --git a/src/modules/db_redis/redis_connection.h b/src/modules/db_redis/redis_connection.h index eedb7990a4b..8261e2bf963 100644 --- a/src/modules/db_redis/redis_connection.h +++ b/src/modules/db_redis/redis_connection.h @@ -77,5 +77,6 @@ int db_redis_append_command_argv(km_redis_con_t *con, redis_key_t *query, int qu int db_redis_get_reply(km_redis_con_t *con, void **reply); void db_redis_consume_replies(km_redis_con_t *con); void db_redis_free_reply(redisReply **reply); +const char *db_redis_get_error(km_redis_con_t *con); -#endif /* _REDIS_CONNECTION_H_ */ +#endif /* _REDIS_CONNECTION_H_ */ \ No newline at end of file diff --git a/src/modules/db_redis/redis_dbase.c b/src/modules/db_redis/redis_dbase.c index 1d550d66528..46bf38b7ee0 100644 --- a/src/modules/db_redis/redis_dbase.c +++ b/src/modules/db_redis/redis_dbase.c @@ -236,6 +236,7 @@ static int db_redis_build_entry_manual_keys(redis_table_t *table, const db_key_t for (key = table->entry_keys; key; key = key->next) { int subkey_found = 0; int i; + *manual_key_count = 0; LM_DBG("checking for existence of entry key '%.*s' in query to get manual key\n", key->key.len, key->key.s); for (i = 0; i < _n; ++i) { @@ -1080,7 +1081,12 @@ static int db_redis_perform_query(const db1_con_t* _h, km_redis_con_t *con, cons RES_COL_N(*_r) = _nc; if (!(*keys_count) && do_table_scan) { - LM_DBG("performing full table scan\n"); + LM_WARN("performing full table scan on table '%.*s' while performing query\n", + CON_TABLE(_h)->len, CON_TABLE(_h)->s); + for(i = 0; i < _n; ++i) { + LM_WARN(" scan key %d is '%.*s'\n", + i, _k[i]->len, _k[i]->s); + } if (db_redis_scan_query_keys(con, CON_TABLE(_h), _k, _n, keys, keys_count, manual_keys, manual_keys_count) != 0) { @@ -1119,7 +1125,7 @@ static int db_redis_perform_query(const db1_con_t* _h, km_redis_con_t *con, cons LM_ERR("Failed to append redis command\n"); goto error; } - tmp = db_redis_key_unshift(&query_v); + tmp = db_redis_key_shift(&query_v); if (tmp) db_redis_key_free(&tmp); @@ -1174,7 +1180,7 @@ static int db_redis_perform_query(const db1_con_t* _h, km_redis_con_t *con, cons // get reply for EXISTS query if (db_redis_get_reply(con, (void**)&reply) != REDIS_OK) { LM_ERR("Failed to get reply for query: %s\n", - con->con->errstr); + db_redis_get_error(con)); goto error; } db_redis_check_reply(con, reply, error); @@ -1182,7 +1188,11 @@ static int db_redis_perform_query(const db1_con_t* _h, km_redis_con_t *con, cons LM_DBG("key does not exist, returning no row for query\n"); db_redis_free_reply(&reply); // also free next reply, as this is a null row for the HMGET - db_redis_get_reply(con, (void**)&reply); + if (db_redis_get_reply(con, (void**)&reply) != REDIS_OK) { + LM_ERR("Failed to get reply for query: %s\n", + db_redis_get_error(con)); + goto error; + } db_redis_check_reply(con, reply, error); db_redis_free_reply(&reply); continue; @@ -1192,7 +1202,7 @@ static int db_redis_perform_query(const db1_con_t* _h, km_redis_con_t *con, cons // get reply for actual HMGET query if (db_redis_get_reply(con, (void**)&reply) != REDIS_OK) { LM_ERR("Failed to get reply for query: %s\n", - con->con->errstr); + db_redis_get_error(con)); goto error; } db_redis_check_reply(con, reply, error); @@ -1228,10 +1238,10 @@ static int db_redis_perform_query(const db1_con_t* _h, km_redis_con_t *con, cons static int db_redis_perform_delete(const db1_con_t* _h, km_redis_con_t *con, const db_key_t* _k, const db_val_t* _v, const db_op_t *_op, const int _n, - redis_key_t *keys, int keys_count, - int *manual_keys, int manual_keys_count, int do_table_scan) { + redis_key_t **keys, int *keys_count, + int **manual_keys, int *manual_keys_count, int do_table_scan) { - int j = 0; + int i = 0, j = 0; redis_key_t *k = NULL; int type_keys_count = 0; int all_type_keys_count = 0; @@ -1245,11 +1255,16 @@ static int db_redis_perform_delete(const db1_con_t* _h, km_redis_con_t *con, con db_key_t *db_keys = NULL; redis_key_t *type_key; - if (!keys_count && do_table_scan) { - LM_DBG("performing full table scan\n"); + if (!*keys_count && do_table_scan) { + LM_WARN("performing full table scan on table '%.*s' while performing delete\n", + CON_TABLE(_h)->len, CON_TABLE(_h)->s); + for(i = 0; i < _n; ++i) { + LM_WARN(" scan key %d is '%.*s'\n", + i, _k[i]->len, _k[i]->s); + } if (db_redis_scan_query_keys(con, CON_TABLE(_h), _k, _n, - &keys, &keys_count, - &manual_keys, &manual_keys_count) != 0) { + keys, keys_count, + manual_keys, manual_keys_count) != 0) { LM_ERR("failed to scan query keys\n"); goto error; } @@ -1266,7 +1281,7 @@ static int db_redis_perform_delete(const db1_con_t* _h, km_redis_con_t *con, con } LM_DBG("delete all keys\n"); - for (k = keys; k; k = k->next) { + for (k = *keys; k; k = k->next) { redis_key_t *all_type_key; str *key = &k->key; redis_key_t *tmp = NULL; @@ -1288,10 +1303,11 @@ static int db_redis_perform_delete(const db1_con_t* _h, km_redis_con_t *con, con if (reply->integer == 0) { LM_DBG("key does not exist in redis, skip deleting\n"); db_redis_free_reply(&reply); + db_redis_key_free(&query_v); continue; } db_redis_free_reply(&reply); - tmp = db_redis_key_unshift(&query_v); + tmp = db_redis_key_shift(&query_v); if (tmp) db_redis_key_free(&tmp); @@ -1301,8 +1317,8 @@ static int db_redis_perform_delete(const db1_con_t* _h, km_redis_con_t *con, con } // add all manual keys to query - for (j = 0; j < manual_keys_count; ++j) { - int idx = manual_keys[j]; + for (j = 0; j < *manual_keys_count; ++j) { + int idx = (*manual_keys)[j]; str *col = _k[idx]; if (db_redis_key_add_str(&query_v, col) != 0) { @@ -1327,8 +1343,8 @@ static int db_redis_perform_delete(const db1_con_t* _h, km_redis_con_t *con, con // manually filter non-matching replies row_match = 1; for (col = 0; col < reply->elements; ++col) { - if (col < manual_keys_count) { - int idx = manual_keys[col]; + if (col < *manual_keys_count) { + int idx = (*manual_keys)[col]; db_key_t k = _k[idx]; db_val_t v = _v[idx]; db_op_t o = _op[idx]; @@ -1366,7 +1382,7 @@ static int db_redis_perform_delete(const db1_con_t* _h, km_redis_con_t *con, con for (j = 0, all_type_key = all_type_keys; all_type_key; ++j, all_type_key = all_type_key->next) { db_val_t *v = &(db_vals[j]); str *key = &all_type_key->key; - char *value = reply->element[manual_keys_count + j]->str; + char *value = reply->element[*manual_keys_count + j]->str; int coltype = db_redis_schema_get_column_type(con, CON_TABLE(_h), key); if (value == NULL) { VAL_NULL(v) = 1; @@ -1418,11 +1434,9 @@ static int db_redis_perform_delete(const db1_con_t* _h, km_redis_con_t *con, con db_redis_check_reply(con, reply, error); db_redis_free_reply(&reply); } - - //db_redis_key_free(&type_keys); LM_DBG("done with loop '%.*s'\n", k->key.len, k->key.s); + db_redis_key_free(&type_keys); } - db_redis_key_free(&type_keys); db_redis_key_free(&all_type_keys); db_redis_key_free(&query_v); @@ -1457,7 +1471,12 @@ static int db_redis_perform_update(const db1_con_t* _h, km_redis_con_t *con, con size_t col; if (!(*keys_count) && do_table_scan) { - LM_DBG("performing full table scan\n"); + LM_WARN("performing full table scan on table '%.*s' while performing update\n", + CON_TABLE(_h)->len, CON_TABLE(_h)->s); + for(i = 0; i < _n; ++i) { + LM_WARN(" scan key %d is '%.*s'\n", + i, _k[i]->len, _k[i]->s); + } if (db_redis_scan_query_keys(con, CON_TABLE(_h), _k, _n, keys, keys_count, manual_keys, manual_keys_count) != 0) { @@ -1547,7 +1566,7 @@ static int db_redis_perform_update(const db1_con_t* _h, km_redis_con_t *con, con // get reply for EXISTS query if (db_redis_get_reply(con, (void**)&reply) != REDIS_OK) { LM_ERR("Failed to get reply for query: %s\n", - con->con->errstr); + db_redis_get_error(con)); goto error; } db_redis_check_reply(con, reply, error); @@ -1556,7 +1575,11 @@ static int db_redis_perform_update(const db1_con_t* _h, km_redis_con_t *con, con db_redis_free_reply(&reply); // also free next reply, as this is a null row for the HMGET LM_DBG("also fetch hmget reply after non-existent key\n"); - db_redis_get_reply(con, (void**)&reply); + if (db_redis_get_reply(con, (void**)&reply) != REDIS_OK) { + LM_ERR("Failed to get reply for query: %s\n", + db_redis_get_error(con)); + goto error; + } db_redis_check_reply(con, reply, error); db_redis_free_reply(&reply); LM_DBG("continue fetch reply loop\n"); @@ -1567,7 +1590,7 @@ static int db_redis_perform_update(const db1_con_t* _h, km_redis_con_t *con, con // get reply for actual HMGET query if (db_redis_get_reply(con, (void**)&reply) != REDIS_OK) { LM_ERR("Failed to get reply for query: %s\n", - con->con->errstr); + db_redis_get_error(con)); goto error; } db_redis_check_reply(con, reply, error); @@ -1644,7 +1667,7 @@ static int db_redis_perform_update(const db1_con_t* _h, km_redis_con_t *con, con for (i = 0; i < update_queries; ++i) { if (db_redis_get_reply(con, (void**)&reply) != REDIS_OK) { LM_ERR("Failed to get reply for query: %s\n", - con->con->errstr); + db_redis_get_error(con)); goto error; } db_redis_check_reply(con, reply, error); @@ -2019,7 +2042,7 @@ int db_redis_delete(const db1_con_t* _h, const db_key_t* _k, } if (db_redis_perform_delete(_h, con, _k, _v, query_ops, _n, - keys, keys_count, manual_keys, manual_keys_count, do_table_scan) != 0) { + &keys, &keys_count, &manual_keys, &manual_keys_count, do_table_scan) != 0) { goto error; } diff --git a/src/modules/db_redis/redis_dbase.h b/src/modules/db_redis/redis_dbase.h index a1e20f74388..f41be1a2794 100644 --- a/src/modules/db_redis/redis_dbase.h +++ b/src/modules/db_redis/redis_dbase.h @@ -85,4 +85,4 @@ int db_redis_replace(const db1_con_t* handle, const db_key_t* keys, const db_val */ int db_redis_use_table(db1_con_t* _h, const str* _t); -#endif /* _REDIS_BASE_H_ */ +#endif /* _REDIS_BASE_H_ */ \ No newline at end of file diff --git a/src/modules/db_redis/redis_table.c b/src/modules/db_redis/redis_table.c index 2b4bcfb1b35..fb7c06cac0f 100644 --- a/src/modules/db_redis/redis_table.c +++ b/src/modules/db_redis/redis_table.c @@ -103,7 +103,7 @@ int db_redis_key_prepend_string(redis_key_t **list, const char* entry, int len) return -1; } -redis_key_t * db_redis_key_unshift(redis_key_t **list) { +redis_key_t * db_redis_key_shift(redis_key_t **list) { redis_key_t *k; k = *list; diff --git a/src/modules/db_redis/redis_table.h b/src/modules/db_redis/redis_table.h index 995eb6ae033..9e71d2a2ba2 100644 --- a/src/modules/db_redis/redis_table.h +++ b/src/modules/db_redis/redis_table.h @@ -58,7 +58,7 @@ int db_redis_key_add_string(redis_key_t* *list, const char* entry, int len); int db_redis_key_add_str(redis_key_t **list, const str* entry); int db_redis_key_prepend_string(redis_key_t **list, const char* entry, int len); int db_redis_key_list2arr(redis_key_t *list, char ***arr); -redis_key_t * db_redis_key_unshift(redis_key_t **list); +redis_key_t * db_redis_key_shift(redis_key_t **list); void db_redis_key_free(redis_key_t **list); int db_redis_keys_spec(char *spec);