Skip to content

Commit

Permalink
db_redis: Fix various pointer and memory issues
Browse files Browse the repository at this point in the history 
Issues discovered by coverity:
* Fix mem leaks in error handling
* Fix potential null pointer deref
* Fix potential out-of-memory cases
  • Loading branch information
Andreas Granig committed Mar 19, 2018
1 parent eda57ef commit 22bd6ca
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 12 deletions.
5 changes: 5 additions & 0 deletions src/modules/db_redis/redis_connection.c
View file
Expand Up @@ -339,6 +339,11 @@ int db_redis_get_reply(km_redis_con_t *con, void **reply) {
int ret;
redis_key_t *query;

if (!con || !con->con) {
LM_ERR("Internal error passing null connection\n");
return -1;
}

*reply = NULL;
ret = redisGetReply(con->con, reply);
if (con->con->err == REDIS_ERR_EOF) {
Expand Down
27 changes: 20 additions & 7 deletions src/modules/db_redis/redis_dbase.c
View file
Expand Up @@ -378,8 +378,6 @@ static int db_redis_build_entry_keys(km_redis_con_t *con, const str *table_name,
goto err;
}
if (key_found) {
db_redis_key_add_str(keys, &keyname);

if (db_redis_key_add_str(keys, &keyname) != 0) {
LM_ERR("Failed to add key string\n");
goto err;
Expand Down Expand Up @@ -470,7 +468,10 @@ static int db_redis_build_type_keys(km_redis_con_t *con, const str *table_name,
goto err;
}
if (key_found) {
db_redis_key_add_str(keys, &keyname);
if (db_redis_key_add_str(keys, &keyname) != 0) {
LM_ERR("Failed to add query key to key list\n");
goto err;
}
(*keys_count)++;
LM_DBG("found key '%.*s' for type '%.*s'\n",
keyname.len, keyname.s,
Expand Down Expand Up @@ -526,7 +527,10 @@ static int db_redis_build_query_keys(km_redis_con_t *con, const str *table_name,
if (key_found) {
LM_DBG("found suitable entry key '%.*s' for query\n",
keyname.len, keyname.s);
db_redis_key_add_str(query_keys, &keyname);
if (db_redis_key_add_str(query_keys, &keyname) != 0) {
LM_ERR("Failed to add key name to query keys\n");
goto err;
}
*query_keys_count = 1;
pkg_free(keyname.s);
keyname.s = NULL;
Expand All @@ -544,10 +548,12 @@ static int db_redis_build_query_keys(km_redis_con_t *con, const str *table_name,

if (db_redis_key_add_string(&query_v, prefix, strlen(prefix)) != 0) {
LM_ERR("Failed to add smembers command to query\n");
db_redis_key_free(&query_v);
goto err;
}
if (db_redis_key_add_str(&query_v, &keyname) != 0) {
LM_ERR("Failed to add key name to smembers query\n");
db_redis_key_free(&query_v);
goto err;
}

Expand Down Expand Up @@ -1062,7 +1068,7 @@ static int db_redis_perform_query(const db1_con_t* _h, km_redis_con_t *con, cons
RES_NUM_ROWS(*_r) = RES_ROW_N(*_r) = 0;
RES_COL_N(*_r) = _nc;

if (!keys_count && do_table_scan) {
if (!(*keys_count) && do_table_scan) {
LM_DBG("performing full table scan\n");
if (db_redis_scan_query_keys(con, CON_TABLE(_h), _k, _n,
keys, keys_count,
Expand Down Expand Up @@ -1354,7 +1360,9 @@ static int db_redis_perform_delete(const db1_con_t* _h, km_redis_con_t *con, con
goto error;
}
pkg_free(db_keys);
db_keys = NULL;
pkg_free(db_vals);
db_vals = NULL;
db_redis_free_reply(&reply);

if (db_redis_key_add_string(&query_v, "DEL", 3) != 0) {
Expand Down Expand Up @@ -1395,6 +1403,7 @@ static int db_redis_perform_delete(const db1_con_t* _h, km_redis_con_t *con, con
}
db_redis_key_free(&type_keys);
db_redis_key_free(&all_type_keys);
db_redis_key_free(&query_v);

return 0;

Expand Down Expand Up @@ -1426,7 +1435,7 @@ static int db_redis_perform_update(const db1_con_t* _h, km_redis_con_t *con, con
int j;
size_t col;

if (!keys_count && do_table_scan) {
if (!(*keys_count) && do_table_scan) {
LM_DBG("performing full table scan\n");
if (db_redis_scan_query_keys(con, CON_TABLE(_h), _k, _n,
keys, keys_count,
Expand Down Expand Up @@ -1664,6 +1673,10 @@ int db_redis_query(const db1_con_t* _h, const db_key_t* _k, const db_op_t* _op,
// TODO: optimize mapping-based manual post-check (remove check for keys already
// in type query key)

if (!_r) {
LM_ERR("db result is null\n");
return -1;
}
con = REDIS_CON(_h);
if (con && con->con == NULL) {
if (db_redis_connect(con) != 0) {
Expand All @@ -1683,7 +1696,7 @@ int db_redis_query(const db1_con_t* _h, const db_key_t* _k, const db_op_t* _op,
CON_TABLE(_h)->len, CON_TABLE(_h)->s);
}

if(_r) *_r = NULL;
*_r = NULL;

// check if we have a version query, and return version directly from
// schema instead of loading it from redis
Expand Down
29 changes: 24 additions & 5 deletions src/modules/db_redis/redis_table.c
View file
Expand Up @@ -313,8 +313,12 @@ void db_redis_free_tables(km_redis_con_t *con) {
col_last = (&col_ht->table[j])->prev;
clist_foreach(&col_ht->table[j], col_he, next) {
pkg_free(col_he->key.s);
pkg_free(col_he);
if (col_he == col_last) break;
if (col_he == col_last) {
pkg_free(col_he);
break;
} else {
pkg_free(col_he);
}
}
}
pkg_free(col_ht->table);
Expand All @@ -340,9 +344,13 @@ void db_redis_free_tables(km_redis_con_t *con) {
}
pkg_free(table);
pkg_free(he->key.s);
pkg_free(he);
if (he == last) {
pkg_free(he);
break;
} else {
pkg_free(he);
}

if (he == last) break;
}
}
pkg_free(ht->table);
Expand Down Expand Up @@ -409,6 +417,7 @@ static struct str_hash_entry* db_redis_create_table(str *table) {
LM_ERR("Failed to allocate memory for table schema hashtable\n");
pkg_free(e->key.s);
pkg_free(e);
pkg_free(t);
return NULL;
}
str_hash_init(&t->columns);
Expand All @@ -426,6 +435,7 @@ static struct str_hash_entry* db_redis_create_column(str *col, str *type) {
}
if (pkg_str_dup(&e->key, col) != 0) {
LM_ERR("Failed to allocate memory for column name\n");
pkg_free(e);
return NULL;
}
e->flags = 0;
Expand Down Expand Up @@ -453,6 +463,7 @@ static struct str_hash_entry* db_redis_create_column(str *col, str *type) {
default:
LM_ERR("Invalid schema column type '%.*s', expecting one of string, int, timestamp, double, blob\n",
type->len, type->s);
pkg_free(e->key.s);
pkg_free(e);
return NULL;
}
Expand Down Expand Up @@ -539,6 +550,10 @@ int db_redis_parse_keys(km_redis_con_t *con) {
if (!table->types) {
table->types = type_target = type;
} else {
if (!type_target) {
LM_ERR("Internal error accessing null type_target\n");
goto err;
}
type_target->next = type;
type_target = type_target->next;
}
Expand Down Expand Up @@ -571,6 +586,10 @@ int db_redis_parse_keys(km_redis_con_t *con) {
if (*key_target == NULL) {
*key_target = key_location = key;
} else {
if (!key_location) {
LM_ERR("Internal error, null key_location pointer\n");
goto err;
}
key_location->next = key;
key_location = key_location->next;
}
Expand Down Expand Up @@ -608,7 +627,7 @@ int db_redis_parse_schema(km_redis_con_t *con) {
char full_path[_POSIX_PATH_MAX + 1];
int path_len;
struct stat fstat;
char c;
unsigned char c;

enum {
DBREDIS_SCHEMA_COLUMN_ST,
Expand Down

0 comments on commit 22bd6ca

Please sign in to comment.