Browse code

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.

Andreas Granig authored on 14/06/2018 13:49:11
Showing 8 changed files
... ...
@@ -93,10 +93,10 @@ static int db_redis_bind_api(db_func_t *dbb) {
93 93
 
94 94
 int keys_param(modparam_t type, void *val)
95 95
 {
96
-	if (val == NULL)
97
-		return -1;
98
-	else
99
-		return db_redis_keys_spec((char*)val);
96
+    if (val == NULL)
97
+        return -1;
98
+    else
99
+        return db_redis_keys_spec((char*)val);
100 100
 }
101 101
 
102 102
 int mod_register(char *path, int *dlflags, void *p1, void *p2) {
... ...
@@ -50,4 +50,4 @@
50 50
 extern str redis_keys;
51 51
 extern str redis_schema_path;
52 52
 
53
-#endif /* _DB_REDIS_MOD_H */
53
+#endif /* _DB_REDIS_MOD_H */
54 54
\ No newline at end of file
... ...
@@ -403,3 +403,11 @@ void db_redis_consume_replies(km_redis_con_t *con) {
403 403
         db_redis_key_free(&query);
404 404
     }
405 405
 }
406
+
407
+const char *db_redis_get_error(km_redis_con_t *con) {
408
+    if (con && con->con && con->con->errstr) {
409
+        return con->con->errstr;
410
+    } else {
411
+        return "<broken redis connection>";
412
+    }
413
+}
406 414
\ No newline at end of file
... ...
@@ -77,5 +77,6 @@ int db_redis_append_command_argv(km_redis_con_t *con, redis_key_t *query, int qu
77 77
 int db_redis_get_reply(km_redis_con_t *con, void **reply);
78 78
 void db_redis_consume_replies(km_redis_con_t *con);
79 79
 void db_redis_free_reply(redisReply **reply);
80
+const char *db_redis_get_error(km_redis_con_t *con);
80 81
 
81
-#endif /* _REDIS_CONNECTION_H_ */
82
+#endif /* _REDIS_CONNECTION_H_ */
82 83
\ No newline at end of file
... ...
@@ -236,6 +236,7 @@ static int db_redis_build_entry_manual_keys(redis_table_t *table, const db_key_t
236 236
     for (key = table->entry_keys; key; key = key->next) {
237 237
         int subkey_found = 0;
238 238
         int i;
239
+        *manual_key_count = 0;
239 240
         LM_DBG("checking for existence of entry key '%.*s' in query to get manual key\n",
240 241
                 key->key.len, key->key.s);
241 242
         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
1080 1081
     RES_COL_N(*_r) = _nc;
1081 1082
 
1082 1083
     if (!(*keys_count) && do_table_scan) {
1083
-        LM_DBG("performing full table scan\n");
1084
+        LM_WARN("performing full table scan on table '%.*s' while performing query\n",
1085
+                CON_TABLE(_h)->len, CON_TABLE(_h)->s);
1086
+        for(i = 0; i < _n; ++i) {
1087
+            LM_WARN("  scan key %d is '%.*s'\n",
1088
+                    i, _k[i]->len, _k[i]->s);
1089
+        }
1084 1090
         if (db_redis_scan_query_keys(con, CON_TABLE(_h), _k, _n,
1085 1091
                     keys, keys_count,
1086 1092
                     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
1119 1125
             LM_ERR("Failed to append redis command\n");
1120 1126
             goto error;
1121 1127
         }
1122
-        tmp = db_redis_key_unshift(&query_v);
1128
+        tmp = db_redis_key_shift(&query_v);
1123 1129
         if (tmp)
1124 1130
             db_redis_key_free(&tmp);
1125 1131
 
... ...
@@ -1174,7 +1180,7 @@ static int db_redis_perform_query(const db1_con_t* _h, km_redis_con_t *con, cons
1174 1180
                 // get reply for EXISTS query
1175 1181
                 if (db_redis_get_reply(con, (void**)&reply) != REDIS_OK) {
1176 1182
                     LM_ERR("Failed to get reply for query: %s\n",
1177
-                            con->con->errstr);
1183
+                            db_redis_get_error(con));
1178 1184
                     goto error;
1179 1185
                 }
1180 1186
                 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
1182 1188
                     LM_DBG("key does not exist, returning no row for query\n");
1183 1189
                     db_redis_free_reply(&reply);
1184 1190
                     // also free next reply, as this is a null row for the HMGET
1185
-                    db_redis_get_reply(con, (void**)&reply);
1191
+                    if (db_redis_get_reply(con, (void**)&reply) != REDIS_OK) {
1192
+                        LM_ERR("Failed to get reply for query: %s\n",
1193
+                                db_redis_get_error(con));
1194
+                        goto error;
1195
+                    }
1186 1196
                     db_redis_check_reply(con, reply, error);
1187 1197
                     db_redis_free_reply(&reply);
1188 1198
                     continue;
... ...
@@ -1192,7 +1202,7 @@ static int db_redis_perform_query(const db1_con_t* _h, km_redis_con_t *con, cons
1192 1202
                 // get reply for actual HMGET query
1193 1203
                 if (db_redis_get_reply(con, (void**)&reply) != REDIS_OK) {
1194 1204
                     LM_ERR("Failed to get reply for query: %s\n",
1195
-                            con->con->errstr);
1205
+                            db_redis_get_error(con));
1196 1206
                     goto error;
1197 1207
                 }
1198 1208
                 db_redis_check_reply(con, reply, error);
... ...
@@ -1228,10 +1238,10 @@ error:
1228 1238
 
1229 1239
 static int db_redis_perform_delete(const db1_con_t* _h, km_redis_con_t *con, const db_key_t* _k,
1230 1240
         const db_val_t* _v, const db_op_t *_op, const int _n,
1231
-        redis_key_t *keys, int keys_count,
1232
-        int *manual_keys, int manual_keys_count, int do_table_scan) {
1241
+        redis_key_t **keys, int *keys_count,
1242
+        int **manual_keys, int *manual_keys_count, int do_table_scan) {
1233 1243
 
1234
-    int j = 0;
1244
+    int i = 0, j = 0;
1235 1245
     redis_key_t *k = NULL;
1236 1246
     int type_keys_count = 0;
1237 1247
     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
1245 1255
     db_key_t *db_keys = NULL;
1246 1256
     redis_key_t *type_key;
1247 1257
 
1248
-    if (!keys_count && do_table_scan) {
1249
-        LM_DBG("performing full table scan\n");
1258
+    if (!*keys_count && do_table_scan) {
1259
+        LM_WARN("performing full table scan on table '%.*s' while performing delete\n",
1260
+                CON_TABLE(_h)->len, CON_TABLE(_h)->s);
1261
+        for(i = 0; i < _n; ++i) {
1262
+            LM_WARN("  scan key %d is '%.*s'\n",
1263
+                    i, _k[i]->len, _k[i]->s);
1264
+        }
1250 1265
         if (db_redis_scan_query_keys(con, CON_TABLE(_h), _k, _n,
1251
-                    &keys, &keys_count,
1252
-                    &manual_keys, &manual_keys_count) != 0) {
1266
+                    keys, keys_count,
1267
+                    manual_keys, manual_keys_count) != 0) {
1253 1268
             LM_ERR("failed to scan query keys\n");
1254 1269
             goto error;
1255 1270
         }
... ...
@@ -1266,7 +1281,7 @@ static int db_redis_perform_delete(const db1_con_t* _h, km_redis_con_t *con, con
1266 1281
     }
1267 1282
 
1268 1283
     LM_DBG("delete all keys\n");
1269
-    for (k = keys; k; k = k->next) {
1284
+    for (k = *keys; k; k = k->next) {
1270 1285
         redis_key_t *all_type_key;
1271 1286
         str *key = &k->key;
1272 1287
         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
1288 1303
         if (reply->integer == 0) {
1289 1304
             LM_DBG("key does not exist in redis, skip deleting\n");
1290 1305
             db_redis_free_reply(&reply);
1306
+            db_redis_key_free(&query_v);
1291 1307
             continue;
1292 1308
         }
1293 1309
         db_redis_free_reply(&reply);
1294
-        tmp = db_redis_key_unshift(&query_v);
1310
+        tmp = db_redis_key_shift(&query_v);
1295 1311
         if (tmp)
1296 1312
             db_redis_key_free(&tmp);
1297 1313
 
... ...
@@ -1301,8 +1317,8 @@ static int db_redis_perform_delete(const db1_con_t* _h, km_redis_con_t *con, con
1301 1317
         }
1302 1318
 
1303 1319
         // add all manual keys to query
1304
-        for (j = 0; j < manual_keys_count; ++j) {
1305
-            int idx = manual_keys[j];
1320
+        for (j = 0; j < *manual_keys_count; ++j) {
1321
+            int idx = (*manual_keys)[j];
1306 1322
             str *col = _k[idx];
1307 1323
 
1308 1324
             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
1327 1343
         // manually filter non-matching replies
1328 1344
         row_match = 1;
1329 1345
         for (col = 0; col < reply->elements; ++col) {
1330
-            if (col < manual_keys_count) {
1331
-                int idx = manual_keys[col];
1346
+            if (col < *manual_keys_count) {
1347
+                int idx = (*manual_keys)[col];
1332 1348
                 db_key_t k = _k[idx];
1333 1349
                 db_val_t v = _v[idx];
1334 1350
                 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
1366 1382
         for (j = 0, all_type_key = all_type_keys; all_type_key; ++j, all_type_key = all_type_key->next) {
1367 1383
             db_val_t *v = &(db_vals[j]);
1368 1384
             str *key = &all_type_key->key;
1369
-            char *value = reply->element[manual_keys_count + j]->str;
1385
+            char *value = reply->element[*manual_keys_count + j]->str;
1370 1386
             int coltype = db_redis_schema_get_column_type(con, CON_TABLE(_h), key);
1371 1387
             if (value == NULL) {
1372 1388
                 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
1418 1434
             db_redis_check_reply(con, reply, error);
1419 1435
             db_redis_free_reply(&reply);
1420 1436
         }
1421
-
1422
-        //db_redis_key_free(&type_keys);
1423 1437
         LM_DBG("done with loop '%.*s'\n", k->key.len, k->key.s);
1438
+        db_redis_key_free(&type_keys);
1424 1439
     }
1425
-    db_redis_key_free(&type_keys);
1426 1440
     db_redis_key_free(&all_type_keys);
1427 1441
     db_redis_key_free(&query_v);
1428 1442
 
... ...
@@ -1457,7 +1471,12 @@ static int db_redis_perform_update(const db1_con_t* _h, km_redis_con_t *con, con
1457 1471
     size_t col;
1458 1472
 
1459 1473
     if (!(*keys_count) && do_table_scan) {
1460
-        LM_DBG("performing full table scan\n");
1474
+        LM_WARN("performing full table scan on table '%.*s' while performing update\n",
1475
+                CON_TABLE(_h)->len, CON_TABLE(_h)->s);
1476
+        for(i = 0; i < _n; ++i) {
1477
+            LM_WARN("  scan key %d is '%.*s'\n",
1478
+                    i, _k[i]->len, _k[i]->s);
1479
+        }
1461 1480
         if (db_redis_scan_query_keys(con, CON_TABLE(_h), _k, _n,
1462 1481
                     keys, keys_count,
1463 1482
                     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
1547 1566
         // get reply for EXISTS query
1548 1567
         if (db_redis_get_reply(con, (void**)&reply) != REDIS_OK) {
1549 1568
             LM_ERR("Failed to get reply for query: %s\n",
1550
-                    con->con->errstr);
1569
+                    db_redis_get_error(con));
1551 1570
             goto error;
1552 1571
         }
1553 1572
         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
1556 1575
             db_redis_free_reply(&reply);
1557 1576
             // also free next reply, as this is a null row for the HMGET
1558 1577
             LM_DBG("also fetch hmget reply after non-existent key\n");
1559
-            db_redis_get_reply(con, (void**)&reply);
1578
+            if (db_redis_get_reply(con, (void**)&reply) != REDIS_OK) {
1579
+                LM_ERR("Failed to get reply for query: %s\n",
1580
+                        db_redis_get_error(con));
1581
+                goto error;
1582
+            }
1560 1583
             db_redis_check_reply(con, reply, error);
1561 1584
             db_redis_free_reply(&reply);
1562 1585
             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
1567 1590
         // get reply for actual HMGET query
1568 1591
         if (db_redis_get_reply(con, (void**)&reply) != REDIS_OK) {
1569 1592
             LM_ERR("Failed to get reply for query: %s\n",
1570
-                    con->con->errstr);
1593
+                    db_redis_get_error(con));
1571 1594
             goto error;
1572 1595
         }
1573 1596
         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
1644 1667
     for (i = 0; i < update_queries; ++i) {
1645 1668
         if (db_redis_get_reply(con, (void**)&reply) != REDIS_OK) {
1646 1669
             LM_ERR("Failed to get reply for query: %s\n",
1647
-                    con->con->errstr);
1670
+                    db_redis_get_error(con));
1648 1671
             goto error;
1649 1672
         }
1650 1673
         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,
2019 2042
     }
2020 2043
 
2021 2044
     if (db_redis_perform_delete(_h, con, _k, _v, query_ops, _n,
2022
-        keys, keys_count, manual_keys, manual_keys_count, do_table_scan) != 0) {
2045
+        &keys, &keys_count, &manual_keys, &manual_keys_count, do_table_scan) != 0) {
2023 2046
         goto error;
2024 2047
     }
2025 2048
 
... ...
@@ -85,4 +85,4 @@ int db_redis_replace(const db1_con_t* handle, const db_key_t* keys, const db_val
85 85
  */
86 86
 int db_redis_use_table(db1_con_t* _h, const str* _t);
87 87
 
88
-#endif  /* _REDIS_BASE_H_ */
88
+#endif  /* _REDIS_BASE_H_ */
89 89
\ No newline at end of file
... ...
@@ -103,7 +103,7 @@ err:
103 103
     return -1;
104 104
 }
105 105
 
106
-redis_key_t * db_redis_key_unshift(redis_key_t **list) {
106
+redis_key_t * db_redis_key_shift(redis_key_t **list) {
107 107
     redis_key_t *k;
108 108
 
109 109
     k = *list;
... ...
@@ -58,7 +58,7 @@ int db_redis_key_add_string(redis_key_t* *list, const char* entry, int len);
58 58
 int db_redis_key_add_str(redis_key_t **list, const str* entry);
59 59
 int db_redis_key_prepend_string(redis_key_t **list, const char* entry, int len);
60 60
 int db_redis_key_list2arr(redis_key_t *list, char ***arr);
61
-redis_key_t * db_redis_key_unshift(redis_key_t **list);
61
+redis_key_t * db_redis_key_shift(redis_key_t **list);
62 62
 void db_redis_key_free(redis_key_t **list);
63 63
 
64 64
 int db_redis_keys_spec(char *spec);