Browse code

db_redis: Fix various pointer and memory issues

Issues discovered by coverity:
* Fix mem leaks in error handling
* Fix potential null pointer deref
* Fix potential out-of-memory cases

Andreas Granig authored on 19/03/2018 16:37:29
Showing 3 changed files
... ...
@@ -339,6 +339,11 @@ int db_redis_get_reply(km_redis_con_t *con, void **reply) {
339 339
     int ret;
340 340
     redis_key_t *query;
341 341
 
342
+    if (!con || !con->con) {
343
+        LM_ERR("Internal error passing null connection\n");
344
+        return -1;
345
+    }
346
+
342 347
     *reply = NULL;
343 348
     ret = redisGetReply(con->con, reply);
344 349
     if (con->con->err == REDIS_ERR_EOF) {
... ...
@@ -378,8 +378,6 @@ static int db_redis_build_entry_keys(km_redis_con_t *con, const str *table_name,
378 378
         goto err;
379 379
     }
380 380
     if (key_found) {
381
-        db_redis_key_add_str(keys, &keyname);
382
-
383 381
         if (db_redis_key_add_str(keys, &keyname) != 0) {
384 382
             LM_ERR("Failed to add key string\n");
385 383
             goto err;
... ...
@@ -470,7 +468,10 @@ static int db_redis_build_type_keys(km_redis_con_t *con, const str *table_name,
470 468
             goto err;
471 469
         }
472 470
         if (key_found) {
473
-            db_redis_key_add_str(keys, &keyname);
471
+            if (db_redis_key_add_str(keys, &keyname) != 0) {
472
+                LM_ERR("Failed to add query key to key list\n");
473
+                goto err;
474
+            }
474 475
             (*keys_count)++;
475 476
             LM_DBG("found key '%.*s' for type '%.*s'\n",
476 477
                     keyname.len, keyname.s,
... ...
@@ -526,7 +527,10 @@ static int db_redis_build_query_keys(km_redis_con_t *con, const str *table_name,
526 527
     if (key_found) {
527 528
         LM_DBG("found suitable entry key '%.*s' for query\n",
528 529
                 keyname.len, keyname.s);
529
-        db_redis_key_add_str(query_keys, &keyname);
530
+        if (db_redis_key_add_str(query_keys, &keyname) != 0) {
531
+            LM_ERR("Failed to add key name to query keys\n");
532
+            goto err;
533
+        }
530 534
         *query_keys_count = 1;
531 535
         pkg_free(keyname.s);
532 536
         keyname.s = NULL;
... ...
@@ -544,10 +548,12 @@ static int db_redis_build_query_keys(km_redis_con_t *con, const str *table_name,
544 548
 
545 549
                 if (db_redis_key_add_string(&query_v, prefix, strlen(prefix)) != 0) {
546 550
                     LM_ERR("Failed to add smembers command to query\n");
551
+                    db_redis_key_free(&query_v);
547 552
                     goto err;
548 553
                 }
549 554
                 if (db_redis_key_add_str(&query_v, &keyname) != 0) {
550 555
                     LM_ERR("Failed to add key name to smembers query\n");
556
+                    db_redis_key_free(&query_v);
551 557
                     goto err;
552 558
                 }
553 559
 
... ...
@@ -1062,7 +1068,7 @@ static int db_redis_perform_query(const db1_con_t* _h, km_redis_con_t *con, cons
1062 1068
     RES_NUM_ROWS(*_r) = RES_ROW_N(*_r) = 0;
1063 1069
     RES_COL_N(*_r) = _nc;
1064 1070
 
1065
-    if (!keys_count && do_table_scan) {
1071
+    if (!(*keys_count) && do_table_scan) {
1066 1072
         LM_DBG("performing full table scan\n");
1067 1073
         if (db_redis_scan_query_keys(con, CON_TABLE(_h), _k, _n,
1068 1074
                     keys, keys_count,
... ...
@@ -1354,7 +1360,9 @@ static int db_redis_perform_delete(const db1_con_t* _h, km_redis_con_t *con, con
1354 1360
             goto error;
1355 1361
         }
1356 1362
         pkg_free(db_keys);
1363
+        db_keys = NULL;
1357 1364
         pkg_free(db_vals);
1365
+        db_vals = NULL;
1358 1366
         db_redis_free_reply(&reply);
1359 1367
 
1360 1368
         if (db_redis_key_add_string(&query_v, "DEL", 3) != 0) {
... ...
@@ -1395,6 +1403,7 @@ static int db_redis_perform_delete(const db1_con_t* _h, km_redis_con_t *con, con
1395 1403
     }
1396 1404
     db_redis_key_free(&type_keys);
1397 1405
     db_redis_key_free(&all_type_keys);
1406
+    db_redis_key_free(&query_v);
1398 1407
 
1399 1408
     return 0;
1400 1409
 
... ...
@@ -1426,7 +1435,7 @@ static int db_redis_perform_update(const db1_con_t* _h, km_redis_con_t *con, con
1426 1435
     int j;
1427 1436
     size_t col;
1428 1437
 
1429
-    if (!keys_count && do_table_scan) {
1438
+    if (!(*keys_count) && do_table_scan) {
1430 1439
         LM_DBG("performing full table scan\n");
1431 1440
         if (db_redis_scan_query_keys(con, CON_TABLE(_h), _k, _n,
1432 1441
                     keys, keys_count,
... ...
@@ -1664,6 +1673,10 @@ int db_redis_query(const db1_con_t* _h, const db_key_t* _k, const db_op_t* _op,
1664 1673
     // TODO: optimize mapping-based manual post-check (remove check for keys already
1665 1674
     // in type query key)
1666 1675
 
1676
+    if (!_r) {
1677
+        LM_ERR("db result is null\n");
1678
+        return -1;
1679
+    }
1667 1680
     con = REDIS_CON(_h);
1668 1681
     if (con && con->con == NULL) {
1669 1682
         if (db_redis_connect(con) != 0) {
... ...
@@ -1683,7 +1696,7 @@ int db_redis_query(const db1_con_t* _h, const db_key_t* _k, const db_op_t* _op,
1683 1696
                 CON_TABLE(_h)->len, CON_TABLE(_h)->s);
1684 1697
     }
1685 1698
 
1686
-    if(_r) *_r = NULL;
1699
+    *_r = NULL;
1687 1700
 
1688 1701
     // check if we have a version query, and return version directly from
1689 1702
     // schema instead of loading it from redis
... ...
@@ -313,8 +313,12 @@ void db_redis_free_tables(km_redis_con_t *con) {
313 313
                 col_last = (&col_ht->table[j])->prev;
314 314
                 clist_foreach(&col_ht->table[j], col_he, next) {
315 315
                     pkg_free(col_he->key.s);
316
-                    pkg_free(col_he);
317
-                    if (col_he == col_last) break;
316
+                    if (col_he == col_last) {
317
+                        pkg_free(col_he);
318
+                        break;
319
+                    } else {
320
+                        pkg_free(col_he);
321
+                    }
318 322
                 }
319 323
             }
320 324
             pkg_free(col_ht->table);
... ...
@@ -340,9 +344,13 @@ void db_redis_free_tables(km_redis_con_t *con) {
340 344
             }
341 345
             pkg_free(table);
342 346
             pkg_free(he->key.s);
343
-            pkg_free(he);
347
+            if (he == last) {
348
+                pkg_free(he);
349
+                break;
350
+            } else {
351
+                pkg_free(he);
352
+            }
344 353
 
345
-            if (he == last) break;
346 354
         }
347 355
     }
348 356
     pkg_free(ht->table);
... ...
@@ -409,6 +417,7 @@ static struct str_hash_entry* db_redis_create_table(str *table) {
409 417
         LM_ERR("Failed to allocate memory for table schema hashtable\n");
410 418
         pkg_free(e->key.s);
411 419
         pkg_free(e);
420
+        pkg_free(t);
412 421
         return NULL;
413 422
     }
414 423
     str_hash_init(&t->columns);
... ...
@@ -426,6 +435,7 @@ static struct str_hash_entry* db_redis_create_column(str *col, str *type) {
426 435
     }
427 436
     if (pkg_str_dup(&e->key, col) != 0) {
428 437
         LM_ERR("Failed to allocate memory for column name\n");
438
+        pkg_free(e);
429 439
         return NULL;
430 440
     }
431 441
     e->flags = 0;
... ...
@@ -453,6 +463,7 @@ static struct str_hash_entry* db_redis_create_column(str *col, str *type) {
453 463
         default:
454 464
             LM_ERR("Invalid schema column type '%.*s', expecting one of string, int, timestamp, double, blob\n",
455 465
                     type->len, type->s);
466
+            pkg_free(e->key.s);
456 467
             pkg_free(e);
457 468
             return NULL;
458 469
     }
... ...
@@ -539,6 +550,10 @@ int db_redis_parse_keys(km_redis_con_t *con) {
539 550
                     if (!table->types) {
540 551
                         table->types = type_target = type;
541 552
                     } else {
553
+                        if (!type_target) {
554
+                            LM_ERR("Internal error accessing null type_target\n");
555
+                            goto err;
556
+                        }
542 557
                         type_target->next = type;
543 558
                         type_target = type_target->next;
544 559
                     }
... ...
@@ -571,6 +586,10 @@ int db_redis_parse_keys(km_redis_con_t *con) {
571 586
                 if (*key_target == NULL) {
572 587
                     *key_target = key_location = key;
573 588
                 } else {
589
+                    if (!key_location) {
590
+                        LM_ERR("Internal error, null key_location pointer\n");
591
+                        goto err;
592
+                    }
574 593
                     key_location->next = key;
575 594
                     key_location = key_location->next;
576 595
                 }
... ...
@@ -608,7 +627,7 @@ int db_redis_parse_schema(km_redis_con_t *con) {
608 627
     char full_path[_POSIX_PATH_MAX + 1];
609 628
     int path_len;
610 629
     struct stat fstat;
611
-    char c;
630
+    unsigned char c;
612 631
 
613 632
     enum {
614 633
         DBREDIS_SCHEMA_COLUMN_ST,