[asterisk-commits] mmichelson: branch mmichelson/trunk-digiumphones r361609 - in /team/mmichelso...

SVN commits to the Asterisk project asterisk-commits at lists.digium.com
Fri Apr 6 17:05:11 CDT 2012


Author: mmichelson
Date: Fri Apr  6 17:05:07 2012
New Revision: 361609

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=361609
Log:
Many many bug fixes to new voicemail APIs.

* Fixes many cases where NULL inputs could cause crashes or
  falsely return successful even though the operation failed.
* Fixes cases where an invalid number of messages was passed to
  the new functions.
* Adds range and existence checking for message indexes to help
  improve atomicity of operations.
* Changes ast_vm_msg_play() to set the "heard" field for a message
  until after the message has been played back.
* Remove the destination folder option from ast_vm_msg_forward()

(issue AST-845)
(issue AST-853)
(issue AST-855)
(issue AST-851)
(issue AST-849)
(issue AST-857)
(issue AST-859)
(issue AST-861)
(issue AST-863)
(issue AST-865)


Modified:
    team/mmichelson/trunk-digiumphones/apps/app_voicemail.c
    team/mmichelson/trunk-digiumphones/include/asterisk/app_voicemail.h

Modified: team/mmichelson/trunk-digiumphones/apps/app_voicemail.c
URL: http://svnview.digium.com/svn/asterisk/team/mmichelson/trunk-digiumphones/apps/app_voicemail.c?view=diff&rev=361609&r1=361608&r2=361609
==============================================================================
--- team/mmichelson/trunk-digiumphones/apps/app_voicemail.c (original)
+++ team/mmichelson/trunk-digiumphones/apps/app_voicemail.c Fri Apr  6 17:05:07 2012
@@ -14595,6 +14595,11 @@
 	int inbox_index = 0;
 	int old_index = 1;
 
+	if (ast_strlen_zero(mailbox)) {
+		ast_log(LOG_WARNING, "Cannot create a mailbox snapshot since no mailbox was specified\n");
+		return NULL;
+	}
+
 	memset(&vmus, 0, sizeof(vmus));
 
 	if (!(ast_strlen_zero(folder))) {
@@ -14728,6 +14733,51 @@
 	return str;
 }
 
+/*!
+ * \brief common bounds checking and existence check for Voicemail API functions.
+ *
+ * \details
+ * This is called by ast_vm_msg_move, ast_vm_msg_remove, and ast_vm_msg_forward to
+ * ensure that data passed in are valid. This tests the following:
+ *
+ * 1. No negative indexes are given.
+ * 2. No index greater than the highest message index for the folder is given.
+ * 3. All message indexes given point to messages that exist.
+ *
+ * \param vms The voicemail state corresponding to an open mailbox
+ * \param msg_ids An array of message identifiers
+ * \param num_msgs The number of identifiers in msg_ids
+ *
+ * \retval -1 Failure
+ * \retval 0 Success
+ */
+static int message_range_and_existence_check(struct vm_state *vms, int *msg_ids, size_t num_msgs)
+{
+	int i;
+	int res = 0;
+	for (i = 0; i < num_msgs; ++i) {
+		int cur_msg = msg_ids[i];
+		if (cur_msg < 0) {
+			ast_log(LOG_WARNING, "Message has negative index\n");
+			res = -1;
+			break;
+		}
+		if (vms->lastmsg < cur_msg) {
+			ast_log(LOG_WARNING, "Message %d is out of range. Last message is %d\n", cur_msg, vms->lastmsg);
+			res = -1;
+			break;
+		}
+		make_file(vms->fn, sizeof(vms->fn), vms->curdir, cur_msg);
+		if (!EXISTS(vms->curdir, cur_msg, vms->fn, NULL)) {
+			ast_log(LOG_WARNING, "Message %d does not exist.\n", cur_msg);
+			res = -1;
+			break;
+		}
+	}
+
+	return res;
+}
+
 static void notify_new_state(struct ast_vm_user *vmu)
 {
 	int new = 0, old = 0, urgent = 0;
@@ -14744,7 +14794,6 @@
 	const char *from_folder,
 	const char *to_mailbox,
 	const char *to_context,
-	const char *to_folder,
 	size_t num_msgs,
 	int *msg_ids,
 	int delete_old)
@@ -14755,17 +14804,32 @@
 	struct ast_config *msg_cfg;
 	struct ast_flags config_flags = { CONFIG_FLAG_NOCACHE };
 	char filename[PATH_MAX];
-	int from_folder_index = get_folder_by_name(from_folder);
-	int to_folder_index = get_folder_by_name(to_folder);
+	int from_folder_index;
 	int open = 0;
 	int res = 0;
 	int i;
 
+	if (ast_strlen_zero(from_mailbox) || ast_strlen_zero(to_mailbox)) {
+		ast_log(LOG_WARNING, "Cannot forward message because either the from or to mailbox was not specified\n");
+		return -1;
+	}
+
+	if (!num_msgs) {
+		ast_log(LOG_WARNING, "Invalid number of messages specified to forward: %zu\n", num_msgs);
+		return -1;
+	}
+
+	if (ast_strlen_zero(from_folder)) {
+		ast_log(LOG_WARNING, "Cannot forward message because the fromfolder was not specified\n");
+		return -1;
+	}
+
 	memset(&vmus, 0, sizeof(vmus));
 	memset(&to_vmus, 0, sizeof(to_vmus));
 	memset(&from_vms, 0, sizeof(from_vms));
 
-	if (to_folder_index == -1 || from_folder_index == -1) {
+	from_folder_index = get_folder_by_name(from_folder);
+	if (from_folder_index == -1) {
 		return -1;
 	}
 
@@ -14792,20 +14856,30 @@
 
 	open = 1;
 
+	if ((from_vms.lastmsg + 1) < num_msgs) {
+		ast_log(LOG_WARNING, "Folder %s has less than %zu messages\n", from_folder, num_msgs);
+		res = -1;
+		goto vm_forward_cleanup;
+	}
+
+	if ((res = message_range_and_existence_check(&from_vms, msg_ids, num_msgs) < 0)) {
+		goto vm_forward_cleanup;
+	}
+
+	/* Now we actually forward the messages */
 	for (i = 0; i < num_msgs; i++) {
 		int cur_msg = msg_ids[i];
 		int duration = 0;
 		const char *value;
 
-		if (cur_msg >= 0 && from_vms.lastmsg < cur_msg) {
-			/* msg does not exist */
-			ast_log(LOG_WARNING, "msg %d does not exist to forward. Last msg is %d\n", cur_msg, from_vms.lastmsg);
-			continue;
-		}
 		make_file(from_vms.fn, sizeof(from_vms.fn), from_vms.curdir, cur_msg);
 		snprintf(filename, sizeof(filename), "%s.txt", from_vms.fn);
 		RETRIEVE(from_vms.curdir, cur_msg, vmu->mailbox, vmu->context);
 		msg_cfg = ast_config_load(filename, config_flags);
+		/* XXX This likely will not fail since we previously ensured that the
+		 * message we are looking for exists. However, there still could be some
+		 * circumstance where this fails, so atomicity is not guaranteed.
+		 */
 		if (!msg_cfg || msg_cfg == CONFIG_STATUS_FILEINVALID) {
 			DISPOSE(from_vms.curdir, cur_msg);
 			continue;
@@ -14857,11 +14931,29 @@
 {
 	struct vm_state vms;
 	struct ast_vm_user *vmu = NULL, vmus;
-	int old_folder_index = get_folder_by_name(oldfolder);
-	int new_folder_index = get_folder_by_name(newfolder);
+	int old_folder_index;
+	int new_folder_index;
 	int open = 0;
 	int res = 0;
 	int i;
+
+	if (ast_strlen_zero(mailbox)) {
+		ast_log(LOG_WARNING, "Cannot move message because no mailbox was specified\n");
+		return -1;
+	}
+
+	if (!num_msgs) {
+		ast_log(LOG_WARNING, "Invalid number of messages specified to move: %zu\n", num_msgs);
+		return -1;
+	}
+
+	if (ast_strlen_zero(oldfolder) || ast_strlen_zero(newfolder)) {
+		ast_log(LOG_WARNING, "Cannot move message because either oldfolder or newfolder was not specified\n");
+		return -1;
+	}
+
+	old_folder_index = get_folder_by_name(oldfolder);
+	new_folder_index = get_folder_by_name(newfolder);
 
 	memset(&vmus, 0, sizeof(vmus));
 	memset(&vms, 0, sizeof(vms));
@@ -14887,12 +14979,12 @@
 
 	open = 1;
 
-	for (i = 0; i < num_msgs; i++) {
-		if (vms.lastmsg < old_msg_nums[i]) {
-			/* msg does not exist */
-			res = -1;
-			goto vm_move_cleanup;
-		}
+	if ((res = message_range_and_existence_check(&vms, old_msg_nums, num_msgs)) < 0) {
+		goto vm_move_cleanup;
+	}
+
+	/* Now actually move the message */
+	for (i = 0; i < num_msgs; ++i) {
 		if (save_to_folder(vmu, &vms, old_msg_nums[i], new_folder_index, (new_msg_nums + i))) {
 			res = -1;
 			goto vm_move_cleanup;
@@ -14937,6 +15029,16 @@
 	int res = 0;
 	int i;
 
+	if (ast_strlen_zero(mailbox)) {
+		ast_log(LOG_WARNING, "Cannot remove message because no mailbox was specified\n");
+		return -1;
+	}
+
+	if (!num_msgs) {
+		ast_log(LOG_WARNING, "Invalid number of messages specified to remove: %zu\n", num_msgs);
+		return -1;
+	}
+
 	memset(&vmus, 0, sizeof(vmus));
 	memset(&vms, 0, sizeof(vms));
 
@@ -14963,13 +15065,17 @@
 
 	open = 1;
 
+	if ((vms.lastmsg + 1) < num_msgs) {
+		ast_log(LOG_WARNING, "Folder %s has less than %zu messages\n", folder, num_msgs);
+		res = -1;
+		goto vm_remove_cleanup;
+	}
+
+	if ((res = message_range_and_existence_check(&vms, msgs, num_msgs)) < 0) {
+		goto vm_remove_cleanup;
+	}
+
 	for (i = 0; i < num_msgs; i++) {
-		if (vms.lastmsg < msgs[i]) {
-			/* msg does not exist */
-			ast_log(AST_LOG_ERROR, "Could not remove msg %d from folder %s because it does not exist.\n", msgs[i], folder);
-			res = -1;
-			goto vm_remove_cleanup;
-		}
 		vms.deleted[msgs[i]] = 1;
 	}
 
@@ -15018,6 +15124,25 @@
 	int res = 0;
 	int open = 0;
 	int i;
+	char filename[PATH_MAX];
+	struct ast_config *msg_cfg;
+	struct ast_flags config_flags = { CONFIG_FLAG_NOCACHE };
+	int duration = 0;
+	const char *value;
+
+	if (ast_strlen_zero(mailbox)) {
+		ast_log(LOG_WARNING, "Cannot play message because no mailbox was specified\n");
+		return -1;
+	}
+
+	if (ast_strlen_zero(folder)) {
+		ast_log(LOG_WARNING, "Cannot play message because no folder was specified\n");
+		return -1;
+	}
+
+	if (ast_strlen_zero(msg_num)) {
+		ast_log(LOG_WARNING, "Cannot play message because no message number was specified\n");
+	}
 
 	memset(&vmus, 0, sizeof(vmus));
 	memset(&vms, 0, sizeof(vms));
@@ -15027,69 +15152,60 @@
 	}
 
 	if (!(vmu = find_user(&vmus, context, mailbox))) {
+		return -1;
+	}
+
+	i = get_folder_by_name(folder);
+	ast_copy_string(vms.username, mailbox, sizeof(vms.username));
+	vms.lastmsg = -1;
+	if ((res = open_mailbox(&vms, vmu, i)) < 0) {
+		ast_log(LOG_WARNING, "Could not open mailbox %s\n", mailbox);
 		goto play2_msg_cleanup;
 	}
-
-	if (!ast_strlen_zero(msg_num) && !ast_strlen_zero(folder)) {
-		char filename[PATH_MAX];
-		struct ast_config *msg_cfg;
-		struct ast_flags config_flags = { CONFIG_FLAG_NOCACHE };
-		int duration = 0;
-		const char *value;
-
-		i = get_folder_by_name(folder);
-		ast_copy_string(vms.username, mailbox, sizeof(vms.username));
-		vms.lastmsg = -1;
-		if ((res = open_mailbox(&vms, vmu, i)) < 0) {
-			ast_log(LOG_WARNING, "Could not open mailbox %s\n", mailbox);
-			res = -1;
-			goto play2_msg_cleanup;
-		}
-		open = 1;
-
-		vms.curmsg = atoi(msg_num);
-		if (vms.curmsg > vms.lastmsg) {
-			res = -1;
-			goto play2_msg_cleanup;
-		}
-
-		/* Find the msg */
-		make_file(vms.fn, sizeof(vms.fn), vms.curdir, vms.curmsg);
-		snprintf(filename, sizeof(filename), "%s.txt", vms.fn);
-		RETRIEVE(vms.curdir, vms.curmsg, vmu->mailbox, vmu->context);
-
-		msg_cfg = ast_config_load(filename, config_flags);
-		if (!msg_cfg || msg_cfg == CONFIG_STATUS_FILEINVALID) {
-			DISPOSE(vms.curdir, vms.curmsg);
-			res = -1;
-			goto play2_msg_cleanup;
-		}
-		if ((value = ast_variable_retrieve(msg_cfg, "message", "duration"))) {
-			duration = atoi(value);
-		}
-		ast_config_destroy(msg_cfg);
-
-		vms.heard[vms.curmsg] = 1;
+	open = 1;
+
+	vms.curmsg = atoi(msg_num);
+	if (vms.curmsg > vms.lastmsg || vms.curmsg < 0) {
+		res = -1;
+		goto play2_msg_cleanup;
+	}
+
+	/* Find the msg */
+	make_file(vms.fn, sizeof(vms.fn), vms.curdir, vms.curmsg);
+	snprintf(filename, sizeof(filename), "%s.txt", vms.fn);
+	RETRIEVE(vms.curdir, vms.curmsg, vmu->mailbox, vmu->context);
+
+	msg_cfg = ast_config_load(filename, config_flags);
+	if (!msg_cfg || msg_cfg == CONFIG_STATUS_FILEINVALID) {
+		DISPOSE(vms.curdir, vms.curmsg);
+		res = -1;
+		goto play2_msg_cleanup;
+	}
+	if ((value = ast_variable_retrieve(msg_cfg, "message", "duration"))) {
+		duration = atoi(value);
+	}
+	ast_config_destroy(msg_cfg);
 
 #ifdef IMAP_STORAGE
-		/*IMAP storage stores any prepended message from a forward
-		 * as a separate file from the rest of the message
-		 */
-		if (!ast_strlen_zero(vms.introfn) && ast_fileexists(vms.introfn, NULL, NULL) > 0) {
-			wait_file(chan, &vms, vms.introfn);
-		}
+	/*IMAP storage stores any prepended message from a forward
+	 * as a separate file from the rest of the message
+	 */
+	if (!ast_strlen_zero(vms.introfn) && ast_fileexists(vms.introfn, NULL, NULL) > 0) {
+		wait_file(chan, &vms, vms.introfn);
+	}
 #endif
-		if (cb) {
-			cb(chan, vms.fn, duration);
-		} else if ((wait_file(chan, &vms, vms.fn)) < 0) {
-			ast_log(AST_LOG_WARNING, "Playback of message %s failed\n", vms.fn);
-		} else {
-			res = 0;
-		}
-
-		/* cleanup configs and msg */
-		DISPOSE(vms.curdir, vms.curmsg);
-	}
+	if (cb) {
+		cb(chan, vms.fn, duration);
+	} else if ((wait_file(chan, &vms, vms.fn)) < 0) {
+		ast_log(AST_LOG_WARNING, "Playback of message %s failed\n", vms.fn);
+	} else {
+		res = 0;
+	}
+
+	vms.heard[vms.curmsg] = 1;
+
+	/* cleanup configs and msg */
+	DISPOSE(vms.curdir, vms.curmsg);
 
 play2_msg_cleanup:
 	if (vmu && open) {

Modified: team/mmichelson/trunk-digiumphones/include/asterisk/app_voicemail.h
URL: http://svnview.digium.com/svn/asterisk/team/mmichelson/trunk-digiumphones/include/asterisk/app_voicemail.h?view=diff&rev=361609&r1=361608&r2=361609
==============================================================================
--- team/mmichelson/trunk-digiumphones/include/asterisk/app_voicemail.h (original)
+++ team/mmichelson/trunk-digiumphones/include/asterisk/app_voicemail.h Fri Apr  6 17:05:07 2012
@@ -135,7 +135,6 @@
  * \brief from_folder The folder from which the message is being forwarded
  * \brief to_mailbox The mailbox to forward the message to
  * \brief to_context The voicemail context of the to_mailbox
- * \brief to_folder The folder to which the message is being forwarded
  * \brief num_msgs The number of messages being forwarded
  * \brief msg_ids The message IDs of the messages in from_mailbox to forward
  * \brief delete_old If non-zero, the forwarded messages are also deleted from from_mailbox.
@@ -149,7 +148,6 @@
 	const char *from_folder,
 	const char *to_mailbox,
 	const char *to_context,
-	const char *to_folder,
 	size_t num_msgs,
 	int *msg_ids,
 	int delete_old);




More information about the asterisk-commits mailing list