[IMAPx] Crash in imapx_free_capability()

It seems the imapx_disconnect() can be called from multiple threads,
eventually causing use-after-free on is->priv->cinfo. Adding a lock
around the private cinfo structure should avoid the crash.

Reported at: https://bugzilla.redhat.com/show_bug.cgi?id=1293028

4 files changed, 146 insertions, 20 deletions

diff --git a/camel/providers/imapx/camel-imapx-command.c b/camel/providers/imapx/camel-imapx-command.c
index 0677736d5..8862ca77a 100644
--- a/camel/providers/imapx/camel-imapx-command.c
+++ b/camel/providers/imapx/camel-imapx-command.c
@@ -420,7 +420,7 @@ camel_imapx_command_add_part (CamelIMAPXCommand *ic,
 	if (type & CAMEL_IMAPX_COMMAND_LITERAL_PLUS) {
 		g_string_append_c (buffer, '{');
 		g_string_append_printf (buffer, "%u", ob_size);
-		if (CAMEL_IMAPX_HAVE_CAPABILITY (camel_imapx_server_get_capability_info (ic->is), LITERALPLUS)) {
+		if (camel_imapx_server_have_capability (ic->is, IMAPX_CAPABILITY_LITERALPLUS)) {
 			g_string_append_c (buffer, '+');
 		} else {
 			type &= ~CAMEL_IMAPX_COMMAND_LITERAL_PLUS;
diff --git a/camel/providers/imapx/camel-imapx-server.c b/camel/providers/imapx/camel-imapx-server.c
index 241dd5a91..eb6e85e56 100644
--- a/camel/providers/imapx/camel-imapx-server.c
+++ b/camel/providers/imapx/camel-imapx-server.c
@@ -291,7 +291,7 @@ struct _CamelIMAPXServerPrivate {
 
 	gboolean is_cyrus;
 
-	/* Info about the current connection */
+	/* Info about the current connection; guarded by priv->stream_lock */
 	struct _capability_info *cinfo;
 
 	GRecMutex command_lock;
@@ -782,21 +782,36 @@ imapx_untagged_capability (CamelIMAPXServer *is,
                            GCancellable *cancellable,
                            GError **error)
 {
+	struct _capability_info *cinfo;
+
 	g_return_val_if_fail (CAMEL_IS_IMAPX_SERVER (is), FALSE);
 
-	if (is->priv->cinfo != NULL)
+	g_mutex_lock (&is->priv->stream_lock);
+
+	if (is->priv->cinfo != NULL) {
 		imapx_free_capability (is->priv->cinfo);
+		is->priv->cinfo = NULL;
+	}
 
-	is->priv->cinfo = imapx_parse_capability (
-		CAMEL_IMAPX_INPUT_STREAM (input_stream), cancellable, error);
+	g_mutex_unlock (&is->priv->stream_lock);
 
-	if (is->priv->cinfo == NULL)
+	cinfo = imapx_parse_capability (CAMEL_IMAPX_INPUT_STREAM (input_stream), cancellable, error);
+
+	if (!cinfo)
 		return FALSE;
 
+	g_mutex_lock (&is->priv->stream_lock);
+
+	if (is->priv->cinfo != NULL)
+		imapx_free_capability (is->priv->cinfo);
+	is->priv->cinfo = cinfo;
+
 	c (is->priv->tagprefix, "got capability flags %08x\n", is->priv->cinfo->capa);
 
 	imapx_server_stash_command_arguments (is);
 
+	g_mutex_unlock (&is->priv->stream_lock);
+
 	return TRUE;
 }
 
@@ -1789,7 +1804,11 @@ imapx_untagged_ok_no_bad (CamelIMAPXServer *is,
 		break;
 	case IMAPX_CAPABILITY:
 		if (is->priv->context->sinfo->u.cinfo) {
-			struct _capability_info *cinfo = is->priv->cinfo;
+			struct _capability_info *cinfo;
+
+			g_mutex_lock (&is->priv->stream_lock);
+
+			cinfo = is->priv->cinfo;
 			is->priv->cinfo = is->priv->context->sinfo->u.cinfo;
 			is->priv->context->sinfo->u.cinfo = NULL;
 			if (cinfo)
@@ -1806,7 +1825,10 @@ imapx_untagged_ok_no_bad (CamelIMAPXServer *is,
 					is->priv->cinfo->capa &= ~list_extended;
 				}
 			}
+
 			imapx_server_stash_command_arguments (is);
+
+			g_mutex_unlock (&is->priv->stream_lock);
 		}
 		break;
 	case IMAPX_COPYUID:
@@ -2709,7 +2731,11 @@ connected:
 			goto exit;
 	}
 
+	g_mutex_lock (&is->priv->stream_lock);
+
 	if (!is->priv->cinfo) {
+		g_mutex_unlock (&is->priv->stream_lock);
+
 		ic = camel_imapx_command_new (is, CAMEL_IMAPX_JOB_CAPABILITY, "CAPABILITY");
 
 		success = camel_imapx_server_process_command_sync (is, ic, _("Failed to get capabilities"), cancellable, error);
@@ -2718,17 +2744,24 @@ connected:
 
 		if (!success)
 			goto exit;
+	} else {
+		g_mutex_unlock (&is->priv->stream_lock);
 	}
 
 	if (method == CAMEL_NETWORK_SECURITY_METHOD_STARTTLS_ON_STANDARD_PORT) {
 
+		g_mutex_lock (&is->priv->stream_lock);
+
 		if (CAMEL_IMAPX_LACK_CAPABILITY (is->priv->cinfo, STARTTLS)) {
+			g_mutex_unlock (&is->priv->stream_lock);
 			g_set_error (
 				&local_error, CAMEL_ERROR,
 				CAMEL_ERROR_GENERIC,
 				_("Failed to connect to IMAP server %s in secure mode: %s"),
 				host, _("STARTTLS not supported"));
 			goto exit;
+		} else {
+			g_mutex_unlock (&is->priv->stream_lock);
 		}
 
 		ic = camel_imapx_command_new (is, CAMEL_IMAPX_JOB_STARTTLS, "STARTTLS");
@@ -2736,6 +2769,8 @@ connected:
 		success = camel_imapx_server_process_command_sync (is, ic, _("Failed to issue STARTTLS"), cancellable, error);
 
 		if (success) {
+			g_mutex_lock (&is->priv->stream_lock);
+
 			/* See if we got new capabilities
 			 * in the STARTTLS response. */
 			imapx_free_capability (is->priv->cinfo);
@@ -2746,6 +2781,8 @@ connected:
 				c (is->priv->tagprefix, "got capability flags %08x\n", is->priv->cinfo ? is->priv->cinfo->capa : 0xFFFFFFFF);
 				imapx_server_stash_command_arguments (is);
 			}
+
+			g_mutex_unlock (&is->priv->stream_lock);
 		}
 
 		camel_imapx_command_unref (ic);
@@ -2784,13 +2821,17 @@ connected:
 		}
 
 		/* Get new capabilities if they weren't already given */
+		g_mutex_lock (&is->priv->stream_lock);
 		if (is->priv->cinfo == NULL) {
+			g_mutex_unlock (&is->priv->stream_lock);
 			ic = camel_imapx_command_new (is, CAMEL_IMAPX_JOB_CAPABILITY, "CAPABILITY");
 			success = camel_imapx_server_process_command_sync (is, ic, _("Failed to get capabilities"), cancellable, error);
 			camel_imapx_command_unref (ic);
 
 			if (!success)
 				goto exit;
+		} else {
+			g_mutex_unlock (&is->priv->stream_lock);
 		}
 	}
 
@@ -2859,7 +2900,11 @@ camel_imapx_server_authenticate_sync (CamelIMAPXServer *is,
 	g_object_unref (settings);
 
 	if (mechanism != NULL) {
+		g_mutex_lock (&is->priv->stream_lock);
+
 		if (is->priv->cinfo && !g_hash_table_lookup (is->priv->cinfo->auth_types, mechanism)) {
+			g_mutex_unlock (&is->priv->stream_lock);
+
 			g_set_error (
 				error, CAMEL_SERVICE_ERROR,
 				CAMEL_SERVICE_ERROR_CANT_AUTHENTICATE,
@@ -2867,6 +2912,8 @@ camel_imapx_server_authenticate_sync (CamelIMAPXServer *is,
 				"authentication"), host, mechanism);
 			result = CAMEL_AUTHENTICATION_ERROR;
 			goto exit;
+		} else {
+			g_mutex_unlock (&is->priv->stream_lock);
 		}
 
 		sasl = camel_sasl_new ("imap", mechanism, service);
@@ -2953,6 +3000,8 @@ camel_imapx_server_authenticate_sync (CamelIMAPXServer *is,
 
 	/* Forget old capabilities after login. */
 	if (result == CAMEL_AUTHENTICATION_ACCEPTED) {
+		g_mutex_lock (&is->priv->stream_lock);
+
 		if (is->priv->cinfo) {
 			imapx_free_capability (is->priv->cinfo);
 			is->priv->cinfo = NULL;
@@ -2964,6 +3013,8 @@ camel_imapx_server_authenticate_sync (CamelIMAPXServer *is,
 			c (is->priv->tagprefix, "got capability flags %08x\n", is->priv->cinfo ? is->priv->cinfo->capa : 0xFFFFFFFF);
 			imapx_server_stash_command_arguments (is);
 		}
+
+		g_mutex_unlock (&is->priv->stream_lock);
 	}
 
 	camel_imapx_command_unref (ic);
@@ -3020,9 +3071,12 @@ imapx_reconnect (CamelIMAPXServer *is,
 		goto exception;
 
 	/* After login we re-capa unless the server already told us. */
+	g_mutex_lock (&is->priv->stream_lock);
 	if (is->priv->cinfo == NULL) {
 		GError *local_error = NULL;
 
+		g_mutex_unlock (&is->priv->stream_lock);
+
 		ic = camel_imapx_command_new (is, CAMEL_IMAPX_JOB_CAPABILITY, "CAPABILITY");
 		camel_imapx_server_process_command_sync (is, ic, _("Failed to get capabilities"), cancellable, &local_error);
 		camel_imapx_command_unref (ic);
@@ -3031,15 +3085,20 @@ imapx_reconnect (CamelIMAPXServer *is,
 			g_propagate_error (error, local_error);
 			goto exception;
 		}
+	} else {
+		g_mutex_unlock (&is->priv->stream_lock);
 	}
 
 	is->priv->state = IMAPX_AUTHENTICATED;
 
 preauthed:
 	/* Fetch namespaces (if supported). */
+	g_mutex_lock (&is->priv->stream_lock);
 	if (CAMEL_IMAPX_HAVE_CAPABILITY (is->priv->cinfo, NAMESPACE)) {
 		GError *local_error = NULL;
 
+		g_mutex_unlock (&is->priv->stream_lock);
+
 		ic = camel_imapx_command_new (is, CAMEL_IMAPX_JOB_NAMESPACE, "NAMESPACE");
 		camel_imapx_server_process_command_sync (is, ic, _("Failed to issue NAMESPACE"), cancellable, &local_error);
 		camel_imapx_command_unref (ic);
@@ -3048,12 +3107,16 @@ preauthed:
 			g_propagate_error (error, local_error);
 			goto exception;
 		}
+
+		g_mutex_lock (&is->priv->stream_lock);
 	}
 
 	/* Enable quick mailbox resynchronization (if supported). */
 	if (use_qresync && CAMEL_IMAPX_HAVE_CAPABILITY (is->priv->cinfo, QRESYNC)) {
 		GError *local_error = NULL;
 
+		g_mutex_unlock (&is->priv->stream_lock);
+
 		ic = camel_imapx_command_new (is, CAMEL_IMAPX_JOB_ENABLE, "ENABLE CONDSTORE QRESYNC");
 		camel_imapx_server_process_command_sync (is, ic, _("Failed to enable QResync"), cancellable, &local_error);
 		camel_imapx_command_unref (ic);
@@ -3063,6 +3126,8 @@ preauthed:
 			goto exception;
 		}
 
+		g_mutex_lock (&is->priv->stream_lock);
+
 		is->priv->use_qresync = TRUE;
 	} else {
 		is->priv->use_qresync = FALSE;
@@ -3072,6 +3137,8 @@ preauthed:
 	if (CAMEL_IMAPX_HAVE_CAPABILITY (is->priv->cinfo, NOTIFY)) {
 		GError *local_error = NULL;
 
+		g_mutex_unlock (&is->priv->stream_lock);
+
 		/* XXX The list of FETCH attributes is negotiable. */
 		ic = camel_imapx_command_new (is, CAMEL_IMAPX_JOB_NOTIFY, "NOTIFY SET "
 			"(selected "
@@ -3090,8 +3157,12 @@ preauthed:
 			g_propagate_error (error, local_error);
 			goto exception;
 		}
+
+		g_mutex_lock (&is->priv->stream_lock);
 	}
 
+	g_mutex_unlock (&is->priv->stream_lock);
+
 	is->priv->state = IMAPX_INITIALISED;
 
 	success = TRUE;
@@ -3101,11 +3172,6 @@ preauthed:
 exception:
 	imapx_disconnect (is);
 
-	if (is->priv->cinfo) {
-		imapx_free_capability (is->priv->cinfo);
-		is->priv->cinfo = NULL;
-	}
-
 exit:
 	g_free (mechanism);
 
@@ -3762,6 +3828,11 @@ imapx_disconnect (CamelIMAPXServer *is)
 	g_clear_object (&is->priv->connection);
 	g_clear_object (&is->priv->subprocess);
 
+	if (is->priv->cinfo) {
+		imapx_free_capability (is->priv->cinfo);
+		is->priv->cinfo = NULL;
+	}
+
 	g_mutex_unlock (&is->priv->stream_lock);
 
 	g_mutex_lock (&is->priv->select_lock);
@@ -3769,11 +3840,6 @@ imapx_disconnect (CamelIMAPXServer *is)
 	g_weak_ref_set (&is->priv->select_pending, NULL);
 	g_mutex_unlock (&is->priv->select_lock);
 
-	if (is->priv->cinfo) {
-		imapx_free_capability (is->priv->cinfo);
-		is->priv->cinfo = NULL;
-	}
-
 	is->priv->is_cyrus = FALSE;
 	is->priv->state = IMAPX_DISCONNECTED;
 
@@ -3807,10 +3873,16 @@ camel_imapx_server_connect_sync (CamelIMAPXServer *is,
 	if (!imapx_reconnect (is, cancellable, error))
 		return FALSE;
 
+	g_mutex_lock (&is->priv->stream_lock);
+
 	if (CAMEL_IMAPX_LACK_CAPABILITY (is->priv->cinfo, NAMESPACE)) {
+		g_mutex_unlock (&is->priv->stream_lock);
+
 		/* This also creates a needed faux NAMESPACE */
 		if (!camel_imapx_server_list_sync (is, "INBOX", 0, cancellable, error))
 			return FALSE;
+	} else {
+		g_mutex_unlock (&is->priv->stream_lock);
 	}
 
 	return TRUE;
@@ -4112,10 +4184,14 @@ camel_imapx_server_copy_message_sync (CamelIMAPXServer *is,
 
 	/* If we're moving messages, prefer "UID MOVE" if supported. */
 	if (delete_originals) {
+		g_mutex_lock (&is->priv->stream_lock);
+
 		if (CAMEL_IMAPX_HAVE_CAPABILITY (is->priv->cinfo, MOVE)) {
 			delete_originals = FALSE;
 			use_move_command = TRUE;
 		}
+
+		g_mutex_unlock (&is->priv->stream_lock);
 	}
 
 	source_infos = g_hash_table_new_full (g_str_hash, g_str_equal, NULL, camel_message_info_unref);
@@ -5745,11 +5821,17 @@ camel_imapx_server_update_quota_info_sync (CamelIMAPXServer *is,
 	g_return_val_if_fail (CAMEL_IS_IMAPX_SERVER (is), FALSE);
 	g_return_val_if_fail (CAMEL_IS_IMAPX_MAILBOX (mailbox), FALSE);
 
+	g_mutex_lock (&is->priv->stream_lock);
+
 	if (CAMEL_IMAPX_LACK_CAPABILITY (is->priv->cinfo, QUOTA)) {
+		g_mutex_unlock (&is->priv->stream_lock);
+
 		g_set_error_literal (
 			error, G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED,
 			_("IMAP server does not support quotas"));
 		return FALSE;
+	} else {
+		g_mutex_unlock (&is->priv->stream_lock);
 	}
 
 	success = camel_imapx_server_ensure_selected_sync (is, mailbox, cancellable, error);
@@ -6020,9 +6102,14 @@ camel_imapx_server_can_use_idle (CamelIMAPXServer *is)
 {
 	gboolean use_idle = FALSE;
 
+	g_mutex_lock (&is->priv->stream_lock);
+
 	/* No need for IDLE if the server supports NOTIFY. */
-	if (CAMEL_IMAPX_HAVE_CAPABILITY (is->priv->cinfo, NOTIFY))
+	if (CAMEL_IMAPX_HAVE_CAPABILITY (is->priv->cinfo, NOTIFY)) {
+		g_mutex_unlock (&is->priv->stream_lock);
+
 		return FALSE;
+	}
 
 	if (CAMEL_IMAPX_HAVE_CAPABILITY (is->priv->cinfo, IDLE)) {
 		CamelIMAPXSettings *settings;
@@ -6032,6 +6119,8 @@ camel_imapx_server_can_use_idle (CamelIMAPXServer *is)
 		g_object_unref (settings);
 	}
 
+	g_mutex_unlock (&is->priv->stream_lock);
+
 	return use_idle;
 }
 
@@ -6282,6 +6371,7 @@ camel_imapx_server_register_untagged_handler (CamelIMAPXServer *is,
 	return previous;
 }
 
+/* This function is not thread-safe. */
 const struct _capability_info *
 camel_imapx_server_get_capability_info (CamelIMAPXServer *is)
 {
@@ -6290,6 +6380,36 @@ camel_imapx_server_get_capability_info (CamelIMAPXServer *is)
 	return is->priv->cinfo;
 }
 
+gboolean
+camel_imapx_server_have_capability (CamelIMAPXServer *is,
+				    guint32 capability)
+{
+	gboolean have;
+
+	g_return_val_if_fail (CAMEL_IS_IMAPX_SERVER (is), FALSE);
+
+	g_mutex_lock (&is->priv->stream_lock);
+	have = is->priv->cinfo != NULL && (is->priv->cinfo->capa & capability) != 0;
+	g_mutex_unlock (&is->priv->stream_lock);
+
+	return have;
+}
+
+gboolean
+camel_imapx_server_lack_capability (CamelIMAPXServer *is,
+				    guint32 capability)
+{
+	gboolean lack;
+
+	g_return_val_if_fail (CAMEL_IS_IMAPX_SERVER (is), FALSE);
+
+	g_mutex_lock (&is->priv->stream_lock);
+	lack = is->priv->cinfo != NULL && (is->priv->cinfo->capa & capability) == 0;
+	g_mutex_unlock (&is->priv->stream_lock);
+
+	return lack;
+}
+
 gchar
 camel_imapx_server_get_tagprefix (CamelIMAPXServer *is)
 {
diff --git a/camel/providers/imapx/camel-imapx-server.h b/camel/providers/imapx/camel-imapx-server.h
index a51c39d05..16d5090c6 100644
--- a/camel/providers/imapx/camel-imapx-server.h
+++ b/camel/providers/imapx/camel-imapx-server.h
@@ -135,6 +135,12 @@ CamelIMAPXMailbox *
 const struct _capability_info *
 		camel_imapx_server_get_capability_info
 						(CamelIMAPXServer *is);
+gboolean	camel_imapx_server_have_capability
+						(CamelIMAPXServer *is,
+						 guint32 capability);
+gboolean	camel_imapx_server_lack_capability
+						(CamelIMAPXServer *is,
+						 guint32 capability);
 gchar		camel_imapx_server_get_tagprefix
 						(CamelIMAPXServer *is);
 void		camel_imapx_server_set_tagprefix
diff --git a/camel/providers/imapx/camel-imapx-store.c b/camel/providers/imapx/camel-imapx-store.c
index d42611372..bf99e250c 100644
--- a/camel/providers/imapx/camel-imapx-store.c
+++ b/camel/providers/imapx/camel-imapx-store.c
@@ -3223,7 +3223,7 @@ camel_imapx_store_handle_list_response (CamelIMAPXStore *imapx_store,
 
 	/* Fabricate a CamelIMAPXNamespaceResponse if the server lacks the
 	 * NAMESPACE capability and this is the first LIST / LSUB response. */
-	if (CAMEL_IMAPX_LACK_CAPABILITY (camel_imapx_server_get_capability_info (imapx_server), NAMESPACE)) {
+	if (camel_imapx_server_lack_capability (imapx_server, IMAPX_CAPABILITY_NAMESPACE)) {
 		g_mutex_lock (&imapx_store->priv->namespaces_lock);
 		if (imapx_store->priv->namespaces == NULL) {
 			imapx_store->priv->namespaces = camel_imapx_namespace_response_faux_new (response);
@@ -3287,7 +3287,7 @@ camel_imapx_store_handle_lsub_response (CamelIMAPXStore *imapx_store,
 
 	/* Fabricate a CamelIMAPXNamespaceResponse if the server lacks the
 	 * NAMESPACE capability and this is the first LIST / LSUB response. */
-	if (CAMEL_IMAPX_LACK_CAPABILITY (camel_imapx_server_get_capability_info (imapx_server), NAMESPACE)) {
+	if (camel_imapx_server_lack_capability (imapx_server, IMAPX_CAPABILITY_NAMESPACE)) {
 		g_mutex_lock (&imapx_store->priv->namespaces_lock);
 		if (imapx_store->priv->namespaces == NULL) {
 			imapx_store->priv->namespaces = camel_imapx_namespace_response_faux_new (response);