Bug 667725 - imapx_untagged: code should not be reached

This code is evil.

When we scan a folder for new messages, we issue a 'FETCH 1:* (UID FLAGS)'
or similar command.

When we receive an untagged FETCH from the server telling us flags for a
message, we make a decision about whether that information was solicited
by such a command, or whether it was unsolicited.

If it was unsolicited, we process it normally as an asynchronous flags
update and all is well.

If it was solicited, we add the UID to a list. When the FETCH (UID FLAGS)
command *completes*, we'll sort that list and then fetch the full headers
for each message.

However, we weren't very good at telling when an update was solicited.
Assuming that only solicited messages will have a UID is bogus.

This was failing if an unsolicited update came in when the (UID FLAGS)
fetch had completed, and we were already fetching the message headers.
The "new" UID would be added to the end of the list, even if we were
already fetching that message or if we already had it in cache. We'd
issue a FETCH command for it, and the barf when the server complied,
because when the UID list wasn't sorted we wouldn't find the offending
uid when we looked for it.

The simple "fix" for this is to keep a boolean flag 'scan_changes' which
is TRUE only when that FETCH (UID FLAGS) command is running. If a flags
change comes in at any other time, it is definitely unsolicited and
should *not* be added to the uidset. This at least protects us from
having UIDs added after we've sorted the list and started to do other
things with it, which was causing the crash.

In fact, this whole 'solicited' vs. 'unsolicited' thing is a design
mistake. In imapx_untagged() we should never care about what we asked
for and what we didn't. That's why the responses are *untagged*. The
server tells us things about the state of the mailboxes, and we should
process that information into our own local cache — it shouldn't
*matter* what we asked for. But that's a more intrusive fix for another day.

In addition, we were reliably *triggering* this behaviour in some cases
because we had to issue a SELECT for the folder in question before
issuing the FETCH (UID FLAGS) command. And on completion of the SELECT,
if UIDNEXT had increased, we were automatically issuing a *new* FETCH
(UID FLAGS) command starting from the last-known-uid in our cache. This
was entirely gratiutous, so use the same scan_changes boolean flag to
avoid it in that situation.
(cherry picked from commit 17f3fa1b12faa89158458d976c110cc9f8733a56)

1 files changed, 36 insertions, 6 deletions

diff --git a/camel/providers/imapx/camel-imapx-server.c b/camel/providers/imapx/camel-imapx-server.c
index b69885b8c..f4819efeb 100644
--- a/camel/providers/imapx/camel-imapx-server.c
+++ b/camel/providers/imapx/camel-imapx-server.c
@@ -105,6 +105,7 @@ struct _RefreshInfoData {
 	gint fetch_msg_limit;
 	CamelFetchType fetch_type;
 	gboolean update_unseen;
+	gboolean scan_changes;
 	struct _uidset_state uidset;
 	/* changes during refresh */
 	CamelFolderChangeInfo *changes;
@@ -1264,17 +1265,19 @@ imapx_untagged (CamelIMAPXServer *is,
 
 		if ((finfo->got & FETCH_FLAGS) && !(finfo->got & FETCH_HEADER)) {
 			CamelIMAPXJob *job = imapx_match_active_job (is, IMAPX_JOB_FETCH_NEW_MESSAGES | IMAPX_JOB_REFRESH_INFO | IMAPX_JOB_FETCH_MESSAGES, NULL);
+			RefreshInfoData *data = NULL;
+
+			if (job) {
+				data = camel_imapx_job_get_data (job);
+				g_return_val_if_fail (data != NULL, FALSE);
+			}
+
 			/* This is either a refresh_info job, check to see if it is and update
 			 * if so, otherwise it must've been an unsolicited response, so update
 			 * the summary to match */
-
-			if (job && (finfo->got & FETCH_UID)) {
-				RefreshInfoData *data;
+			if (data && (finfo->got & FETCH_UID) && data->scan_changes) {
 				struct _refresh_info r;
 
-				data = camel_imapx_job_get_data (job);
-				g_return_val_if_fail (data != NULL, FALSE);
-
 				r.uid = finfo->uid;
 				finfo->uid = NULL;
 				r.server_flags = finfo->flags;
@@ -2452,10 +2455,25 @@ imapx_command_select_done (CamelIMAPXServer *is,
 		ifolder->exists_on_server = is->exists;
 		ifolder->modseq_on_server = is->highestmodseq;
 		if (ifolder->uidnext_on_server < is->uidnext) {
+			/* We don't want to fetch new messages if the command we selected this
+			   folder for is *already* fetching all messages (i.e. scan_changes).
+			   Bug #667725. */
+			CamelIMAPXJob *job = imapx_is_job_in_queue (is, is->select_pending,
+								    IMAPX_JOB_REFRESH_INFO, NULL);
+			if (job) {
+				RefreshInfoData *data = camel_imapx_job_get_data (job);
+
+				if (data->scan_changes) {
+					c(is->tagprefix, "Will not fetch_new_messages when already in scan_changes\n");
+					goto no_fetch_new;
+				}
+			}
 			imapx_server_fetch_new_messages (is, is->select_pending, TRUE, TRUE, NULL, NULL);
 			/* We don't do this right now because we want the new messages to
 			 * update the unseen count. */
 			//ifolder->uidnext_on_server = is->uidnext;
+		no_fetch_new:
+			;
 		}
 		ifolder->uidvalidity_on_server = is->uidvalidity;
 		selected_folder = camel_folder_get_full_name (is->select_folder);
@@ -3696,6 +3714,8 @@ imapx_command_step_fetch_done (CamelIMAPXServer *is,
 	data = camel_imapx_job_get_data (job);
 	g_return_val_if_fail (data != NULL, FALSE);
 
+	data->scan_changes = FALSE;
+
 	ifolder = (CamelIMAPXFolder *) job->folder;
 	isum = (CamelIMAPXSummary *) job->folder->summary;
 
@@ -3846,6 +3866,8 @@ imapx_job_scan_changes_done (CamelIMAPXServer *is,
 	data = camel_imapx_job_get_data (job);
 	g_return_val_if_fail (data != NULL, FALSE);
 
+	data->scan_changes = FALSE;
+
 	service = CAMEL_SERVICE (is->store);
 	settings = camel_service_get_settings (service);
 
@@ -4049,6 +4071,8 @@ imapx_job_scan_changes_start (CamelIMAPXJob *job,
 		"UID FETCH %s:* (UID FLAGS)", uid ? uid : "1");
 	camel_imapx_command_set_job (ic, job);
 	ic->complete = imapx_job_scan_changes_done;
+
+	data->scan_changes = TRUE;
 	ic->pri = job->pri;
 	data->infos = g_array_new (0, 0, sizeof (struct _refresh_info));
 	imapx_command_queue (is, ic);
@@ -4128,6 +4152,8 @@ imapx_command_fetch_new_uids_done (CamelIMAPXServer *is,
 	data = camel_imapx_job_get_data (job);
 	g_return_val_if_fail (data != NULL, FALSE);
 
+	data->scan_changes = FALSE;
+
 	qsort (
 		data->infos->data,
 		data->infos->len,
@@ -4192,6 +4218,8 @@ imapx_job_fetch_new_messages_start (CamelIMAPXJob *job,
 		data->infos = g_array_new (0, 0, sizeof (struct _refresh_info));
 		ic->pri = job->pri;
 
+		data->scan_changes = TRUE;
+
 		if (fetch_order == CAMEL_SORT_DESCENDING)
 			ic->complete = imapx_command_fetch_new_uids_done;
 		else
@@ -4293,6 +4321,8 @@ imapx_job_fetch_messages_start (CamelIMAPXJob *job,
 		data->infos = g_array_new (0, 0, sizeof (struct _refresh_info));
 		ic->pri = job->pri;
 
+		data->scan_changes = TRUE;
+
 		if (fetch_order == CAMEL_SORT_DESCENDING)
 			ic->complete = imapx_command_fetch_new_uids_done;
 		else