Avoid a DB1 infinite loop bug

Explicity check the last entry in the DB and make sure that we don't iterate
past it. Since there can be no duplicates, this just makes sure that we stop
after matching the last key.

This patch also refactors the code to get away from some code duplication. A
previous patch added many astdb tests and this patch passed them.

Review: https://reviewboard.asterisk.org/r/1259/


git-svn-id: https://origsvn.digium.com/svn/asterisk/branches/1.8@322981 65c4cc65-6c06-0410-ace0-fbb531ad65f3

main/db.c

@@ -100,9 +100,12 @@ ASTERISK_FILE_VERSION(__FILE__, "$Revision$")
 	</manager>
  ***/
 
+#define MAX_DB_FIELD 256
+
 static DB *astdb;
 AST_MUTEX_DEFINE_STATIC(dblock);
 static ast_cond_t dbcond;
+typedef int (*process_keys_cb)(DBT *key, DBT *value, const char *filter, void *data);
 
 static void db_sync(void);
 
@@ -143,56 +146,102 @@ static inline int subkeymatch(const char *key, const char *suffix)
 	return 0;
 }
 
-int ast_db_deltree(const char *family, const char *keytree)
+static const char *dbt_data2str(DBT *dbt)
 {
-	char prefix[256];
+	char *data = "";
-	DBT key, data;
-	char *keys;
-	int res;
-	int pass;
-	int counter = 0;
 
-	if (family) {
+	if (dbt->size) {
-		if (keytree) {
+		data = dbt->data;
-			snprintf(prefix, sizeof(prefix), "/%s/%s", family, keytree);
+		data[dbt->size - 1] = '\0';
-		} else {
-			snprintf(prefix, sizeof(prefix), "/%s", family);
 	}
-	} else if (keytree) {
+
-		return -1;
+	return data;
-	} else {
+}
-		prefix[0] = '\0';
+
+static inline const char *dbt_data2str_full(DBT *dbt, const char *def)
+{
+	return S_OR(dbt_data2str(dbt), def);
 }
 
+static int process_db_keys(process_keys_cb cb, void *data, const char *filter, int sync)
+{
+	DBT key = { 0, }, value = { 0, }, last_key = { 0, };
+	int counter = 0;
+	int res, last = 0;
+	char last_key_s[MAX_DB_FIELD];
+
 	ast_mutex_lock(&dblock);
 	if (dbinit()) {
 		ast_mutex_unlock(&dblock);
 		return -1;
 	}
 
-	memset(&key, 0, sizeof(key));
+	/* Somehow, the database can become corrupted such that astdb->seq will continue looping through
-	memset(&data, 0, sizeof(data));
+	 * the database indefinitely. The pointer to last_key.data ends up getting re-used by the BDB lib
-	pass = 0;
+	 * so this specifically queries for the last entry, makes a copy of the key, and then uses it as
-	while (!(res = astdb->seq(astdb, &key, &data, pass++ ? R_NEXT : R_FIRST))) {
+	 * a sentinel to avoid repeatedly looping over the list. */
-		if (key.size) {
+
-			keys = key.data;
+	if (astdb->seq(astdb, &last_key, &value, R_LAST)) {
-			keys[key.size - 1] = '\0';
+		/* Empty database */
-		} else {
+		ast_mutex_unlock(&dblock);
-			keys = "<bad key>";
+		return 0;
 	}
-		if (keymatch(keys, prefix)) {
+
-			astdb->del(astdb, &key, 0);
+	memcpy(last_key_s, last_key.data, MIN(last_key.size - 1, sizeof(last_key_s)));
-			counter++;
+	last_key_s[last_key.size - 1] = '\0';
+	for (res = astdb->seq(astdb, &key, &value, R_FIRST);
+			!res;
+			res = astdb->seq(astdb, &key, &value, R_NEXT)) {
+		/* The callback might delete the key, so we have to check it before calling */
+		last = !strcmp(dbt_data2str_full(&key, "<bad key>"), last_key_s);
+		counter += cb(&key, &value, filter, data);
+		if (last) {
+			break;
 		}
 	}
+
+	if (sync) {
 		db_sync();
+	}
+
 	ast_mutex_unlock(&dblock);
+
 	return counter;
 }
 
+static int db_deltree_cb(DBT *key, DBT *value, const char *filter, void *data)
+{
+	int res = 0;
+
+	if (keymatch(dbt_data2str_full(key, "<bad key>"), filter)) {
+		astdb->del(astdb, key, 0);
+		res = 1;
+	}
+	return res;
+}
+
+int ast_db_deltree(const char *family, const char *keytree)
+{
+	char prefix[MAX_DB_FIELD];
+
+	if (family) {
+		if (keytree) {
+			snprintf(prefix, sizeof(prefix), "/%s/%s", family, keytree);
+		} else {
+			snprintf(prefix, sizeof(prefix), "/%s", family);
+		}
+	} else if (keytree) {
+		return -1;
+	} else {
+		prefix[0] = '\0';
+	}
+
+	return process_db_keys(db_deltree_cb, NULL, prefix, 1);
+}
+
 int ast_db_put(const char *family, const char *keys, const char *value)
 {
-	char fullkey[256];
+	char fullkey[MAX_DB_FIELD];
 	DBT key, data;
 	int res, fullkeylen;
 
@@ -214,12 +263,13 @@ int ast_db_put(const char *family, const char *keys, const char *value)
 	ast_mutex_unlock(&dblock);
 	if (res)
 		ast_log(LOG_WARNING, "Unable to put value '%s' for key '%s' in family '%s'\n", value, keys, family);
+
 	return res;
 }
 
 int ast_db_get(const char *family, const char *keys, char *value, int valuelen)
 {
-	char fullkey[256] = "";
+	char fullkey[MAX_DB_FIELD] = "";
 	DBT key, data;
 	int res, fullkeylen;
 
@@ -263,7 +313,7 @@ int ast_db_get(const char *family, const char *keys, char *value, int valuelen)
 
 int ast_db_del(const char *family, const char *keys)
 {
-	char fullkey[256];
+	char fullkey[MAX_DB_FIELD];
 	DBT key;
 	int res, fullkeylen;
 
@@ -319,7 +369,7 @@ static char *handle_cli_database_put(struct ast_cli_entry *e, int cmd, struct as
 static char *handle_cli_database_get(struct ast_cli_entry *e, int cmd, struct ast_cli_args *a)
 {
 	int res;
-	char tmp[256];
+	char tmp[MAX_DB_FIELD];
 
 	switch (cmd) {
 	case CLI_INIT:
@@ -402,13 +452,23 @@ static char *handle_cli_database_deltree(struct ast_cli_entry *e, int cmd, struc
 	return CLI_SUCCESS;
 }
 
+static int db_show_cb(DBT *key, DBT *value, const char *filter, void *data)
+{
+	struct ast_cli_args *a = data;
+	const char *key_s = dbt_data2str_full(key, "<bad key>");
+	const char *value_s = dbt_data2str_full(value, "<bad value>");
+
+	if (keymatch(key_s, filter)) {
+		ast_cli(a->fd, "%-50s: %-25s\n", key_s, value_s);
+		return 1;
+	}
+
+	return 0;
+}
+
 static char *handle_cli_database_show(struct ast_cli_entry *e, int cmd, struct ast_cli_args *a)
 {
-	char prefix[256];
+	char prefix[MAX_DB_FIELD];
-	DBT key, data;
-	char *keys, *values;
-	int res;
-	int pass;
 	int counter = 0;
 
 	switch (cmd) {
@@ -435,45 +495,33 @@ static char *handle_cli_database_show(struct ast_cli_entry *e, int cmd, struct a
 	} else {
 		return CLI_SHOWUSAGE;
 	}
-	ast_mutex_lock(&dblock);
+
-	if (dbinit()) {
+	if((counter = process_db_keys(db_show_cb, a, prefix, 0)) < 0) {
-		ast_mutex_unlock(&dblock);
 		ast_cli(a->fd, "Database unavailable\n");
 		return CLI_SUCCESS;
 	}
-	memset(&key, 0, sizeof(key));
+
-	memset(&data, 0, sizeof(data));
-	pass = 0;
-	while (!(res = astdb->seq(astdb, &key, &data, pass++ ? R_NEXT : R_FIRST))) {
-		if (key.size) {
-			keys = key.data;
-			keys[key.size - 1] = '\0';
-		} else {
-			keys = "<bad key>";
-		}
-		if (data.size) {
-			values = data.data;
-			values[data.size - 1]='\0';
-		} else {
-			values = "<bad value>";
-		}
-		if (keymatch(keys, prefix)) {
-			ast_cli(a->fd, "%-50s: %-25s\n", keys, values);
-			counter++;
-		}
-	}
-	ast_mutex_unlock(&dblock);
 	ast_cli(a->fd, "%d results found.\n", counter);
 	return CLI_SUCCESS;
 }
 
+static int db_showkey_cb(DBT *key, DBT *value, const char *filter, void *data)
+{
+	struct ast_cli_args *a = data;
+	const char *key_s = dbt_data2str_full(key, "<bad key>");
+	const char *value_s = dbt_data2str_full(value, "<bad value>");
+
+	if (subkeymatch(key_s, filter)) {
+		ast_cli(a->fd, "%-50s: %-25s\n", key_s, value_s);
+		return 1;
+	}
+
+	return 0;
+}
+
 static char *handle_cli_database_showkey(struct ast_cli_entry *e, int cmd, struct ast_cli_args *a)
 {
-	char suffix[256];
+	char suffix[MAX_DB_FIELD];
-	DBT key, data;
-	char *keys, *values;
-	int res;
-	int pass;
 	int counter = 0;
 
 	switch (cmd) {
@@ -493,48 +541,40 @@ static char *handle_cli_database_showkey(struct ast_cli_entry *e, int cmd, struc
 	} else {
 		return CLI_SHOWUSAGE;
 	}
-	ast_mutex_lock(&dblock);
+
-	if (dbinit()) {
+	if ((counter = process_db_keys(db_showkey_cb, a, suffix, 0)) < 0) {
-		ast_mutex_unlock(&dblock);
 		ast_cli(a->fd, "Database unavailable\n");
 		return CLI_SUCCESS;
 	}
-	memset(&key, 0, sizeof(key));
+
-	memset(&data, 0, sizeof(data));
-	pass = 0;
-	while (!(res = astdb->seq(astdb, &key, &data, pass++ ? R_NEXT : R_FIRST))) {
-		if (key.size) {
-			keys = key.data;
-			keys[key.size - 1] = '\0';
-		} else {
-			keys = "<bad key>";
-		}
-		if (data.size) {
-			values = data.data;
-			values[data.size - 1]='\0';
-		} else {
-			values = "<bad value>";
-		}
-		if (subkeymatch(keys, suffix)) {
-			ast_cli(a->fd, "%-50s: %-25s\n", keys, values);
-			counter++;
-		}
-	}
-	ast_mutex_unlock(&dblock);
 	ast_cli(a->fd, "%d results found.\n", counter);
 	return CLI_SUCCESS;
 }
 
+static int db_gettree_cb(DBT *key, DBT *value, const char *filter, void *data)
+{
+	struct ast_db_entry **ret = data;
+	struct ast_db_entry *cur;
+	const char *key_s = dbt_data2str_full(key, "<bad key>");
+	const char *value_s = dbt_data2str_full(value, "<bad value>");
+	size_t key_slen = strlen(key_s) + 1, value_slen = strlen(value_s) + 1;
+
+	if (keymatch(key_s, filter) && (cur = ast_malloc(sizeof(*cur) + key_slen + value_slen))) {
+		cur->next = *ret;
+		cur->key = cur->data + value_slen;
+		strcpy(cur->data, value_s);
+		strcpy(cur->key, key_s);
+		*ret = cur;
+		return 1;
+	}
+
+	return 0;
+}
+
 struct ast_db_entry *ast_db_gettree(const char *family, const char *keytree)
 {
-	char prefix[256];
+	char prefix[MAX_DB_FIELD];
-	DBT key, data;
+	struct ast_db_entry *ret = NULL;
-	char *keys, *values;
-	int values_len;
-	int res;
-	int pass;
-	struct ast_db_entry *last = NULL;
-	struct ast_db_entry *cur, *ret=NULL;
 
 	if (!ast_strlen_zero(family)) {
 		if (!ast_strlen_zero(keytree)) {
@@ -547,43 +587,12 @@ struct ast_db_entry *ast_db_gettree(const char *family, const char *keytree)
 	} else {
 		prefix[0] = '\0';
 	}
-	ast_mutex_lock(&dblock);
+
-	if (dbinit()) {
+	if (process_db_keys(db_gettree_cb, &ret, prefix, 0) < 0) {
-		ast_mutex_unlock(&dblock);
 		ast_log(LOG_WARNING, "Database unavailable\n");
 		return NULL;
 	}
-	memset(&key, 0, sizeof(key));
+
-	memset(&data, 0, sizeof(data));
-	pass = 0;
-	while (!(res = astdb->seq(astdb, &key, &data, pass++ ? R_NEXT : R_FIRST))) {
-		if (key.size) {
-			keys = key.data;
-			keys[key.size - 1] = '\0';
-		} else {
-			keys = "<bad key>";
-		}
-		if (data.size) {
-			values = data.data;
-			values[data.size - 1] = '\0';
-		} else {
-			values = "<bad value>";
-		}
-		values_len = strlen(values) + 1;
-		if (keymatch(keys, prefix) && (cur = ast_malloc(sizeof(*cur) + strlen(keys) + 1 + values_len))) {
-			cur->next = NULL;
-			cur->key = cur->data + values_len;
-			strcpy(cur->data, values);
-			strcpy(cur->key, keys);
-			if (last) {
-				last->next = cur;
-			} else {
-				ret = cur;
-			}
-			last = cur;
-		}
-	}
-	ast_mutex_unlock(&dblock);
 	return ret;
 }
 
@@ -637,7 +646,7 @@ static int manager_dbget(struct mansession *s, const struct message *m)
 	char idText[256] = "";
 	const char *family = astman_get_header(m, "Family");
 	const char *key = astman_get_header(m, "Key");
-	char tmp[256];
+	char tmp[MAX_DB_FIELD];
 	int res;
 
 	if (ast_strlen_zero(family)) {