Re: [Evolution-hackers] bug fixing in stable 2.22.x branch

[Patrick Ohly <Patrick Ohly gmx de>]

On Di, 2008-09-02 at 19:04 +0200, Philipp Riegger wrote:
> On Mon, 2008-09-01 at 19:55 +0200, Patrick Ohly wrote:
> >       * 499932: [Bug 499932] Not deleting from e-mail server after
> > specified time
> 
> I applied this to the gentoo evolution-data-server ebuild and had some
> instability. Evolution died after a timeout in one of my pop3 accounts
> (server could not be found/reached). Be sure, to double check that.

Thanks a lot for reporting this! I didn't notice that when trying out
trunk.

Now I had a look at the actual patch and found that it has a
(potentially huge?) memory leak: in pop3_get_message_time_from_cache()
after camel_data_cache_get() the returned stream already has a refcount
of 1, but the code did another camel_object_ref(). Therefore the
camel_object_unref() later on only reduced the refcount to 1 and the
stream was never freed.

The stream was also not freed if reading from it failed or didn't
contain the expected content - not sure whether that really happens.

Attached is the original and the revised patch. It works for me on
2.22.x. Okay to commit on 2.22.x and to fix the trunk accordingly?

-- 
Bye, Patrick Ohly
--  
Patrick Ohly gmx de
http://www.estamos.de/

Index: camel/providers/pop3/ChangeLog
===================================================================
--- camel/providers/pop3/ChangeLog	(revision 8571)
+++ camel/providers/pop3/ChangeLog	(working copy)
@@ -1,3 +1,11 @@
+2008-03-12  Milan Crha  <mcrha redhat com>
+
+	** Fix for bug #514827
+
+	* camel-pop3-folder.c: (pop3_get_message_time_from_cache),
+	(camel_pop3_delete_old): POP3 folder doesn't have a summary,
+	it has its own cache, so read message time from it.
+
 2008-02-18  Chenthill Palanisamy  <pchenthill novell com>
 
 	* camel-pop3-store.c (connect_to_server): Fix for some
Index: camel/providers/pop3/camel-pop3-folder.c
===================================================================
--- camel/providers/pop3/camel-pop3-folder.c	(revision 8571)
+++ camel/providers/pop3/camel-pop3-folder.c	(working copy)
@@ -356,6 +356,49 @@ pop3_sync (CamelFolder *folder, gboolean
 	camel_pop3_store_expunge (pop3_store, ex);
 }
 
+static gboolean
+pop3_get_message_time_from_cache (CamelFolder *folder, const char *uid, time_t *message_time)
+{
+	CamelPOP3Store *pop3_store;
+	CamelStream *stream = NULL;
+	char buffer[1];
+	gboolean res = FALSE;
+
+	g_return_val_if_fail (folder != NULL, FALSE);
+	g_return_val_if_fail (uid != NULL, FALSE);
+	g_return_val_if_fail (message_time != NULL, FALSE);
+
+	pop3_store = CAMEL_POP3_STORE (folder->parent_store);
+
+	g_return_val_if_fail (pop3_store->cache != NULL, FALSE);
+
+	if ((stream = camel_data_cache_get (pop3_store->cache, "cache", uid, NULL)) != NULL
+	    && camel_stream_read (stream, buffer, 1) == 1
+	    && buffer[0] == '#') {
+		CamelMimeMessage *message;
+
+		camel_object_ref ((CamelObject *)stream);
+
+		message = camel_mime_message_new ();
+		if (camel_data_wrapper_construct_from_stream ((CamelDataWrapper *)message, stream) == -1) {
+			g_warning (_("Cannot get message %s: %s"), uid, g_strerror (errno));
+			camel_object_unref ((CamelObject *)message);
+			message = NULL;
+		}
+
+		if (message) {
+			res = TRUE;
+			*message_time = message->date + message->date_offset;
+
+			camel_object_unref ((CamelObject *)message);
+		}
+
+		camel_object_unref ((CamelObject *)stream);
+	}
+
+	return res;
+}
+
 int
 camel_pop3_delete_old(CamelFolder *folder, int days_to_delete,	CamelException *ex)
 {
@@ -363,8 +406,7 @@ camel_pop3_delete_old(CamelFolder *folde
 	CamelPOP3FolderInfo *fi;
 	int i;
 	CamelPOP3Store *pop3_store;
-	time_t temp;
-	CamelMessageInfo *minfo;
+	time_t temp, message_time;
 
 	pop3_folder = CAMEL_POP3_FOLDER (folder);
 	pop3_store = CAMEL_POP3_STORE (CAMEL_FOLDER(pop3_folder)->parent_store);
@@ -374,10 +416,8 @@ camel_pop3_delete_old(CamelFolder *folde
 	for (i = 0; i < pop3_folder->uids->len; i++) {
 		fi = pop3_folder->uids->pdata[i];
 
-		minfo = camel_folder_get_message_info (folder, fi->uid);
 		d(printf("%s(%d): fi->uid=[%s]\n", __FILE__, __LINE__, fi->uid));
-		if(minfo) {
-			time_t message_time = ((CamelMessageInfoBase *)minfo)->date_received;
+		if (pop3_get_message_time_from_cache (folder, fi->uid, &message_time)) {
 			double time_diff = difftime(temp,message_time);
 			int day_lag = time_diff/(60*60*24);
 
@@ -407,8 +447,6 @@ camel_pop3_delete_old(CamelFolder *folde
 					camel_data_cache_remove(pop3_store->cache, "cache", fi->uid, NULL);
 				}
 			}
-			/* free message - not used anymore */
-			camel_folder_free_message_info (folder, minfo);
 		}
 	}
 

Index: camel-pop3-folder.c
===================================================================
--- camel-pop3-folder.c	(revision 9476)
+++ camel-pop3-folder.c	(working copy)
@@ -356,6 +356,48 @@
 	camel_pop3_store_expunge (pop3_store, ex);
 }
 
+static gboolean
+pop3_get_message_time_from_cache (CamelFolder *folder, const char *uid, time_t *message_time)
+{
+	CamelPOP3Store *pop3_store;
+	CamelStream *stream = NULL;
+	char buffer[1];
+	gboolean res = FALSE;
+
+	g_return_val_if_fail (folder != NULL, FALSE);
+	g_return_val_if_fail (uid != NULL, FALSE);
+	g_return_val_if_fail (message_time != NULL, FALSE);
+
+	pop3_store = CAMEL_POP3_STORE (folder->parent_store);
+
+	g_return_val_if_fail (pop3_store->cache != NULL, FALSE);
+
+	if ((stream = camel_data_cache_get (pop3_store->cache, "cache", uid, NULL)) != NULL
+	    && camel_stream_read (stream, buffer, 1) == 1
+	    && buffer[0] == '#') {
+		CamelMimeMessage *message;
+
+		message = camel_mime_message_new ();
+		if (camel_data_wrapper_construct_from_stream ((CamelDataWrapper *)message, stream) == -1) {
+			g_warning (_("Cannot get message %s: %s"), uid, g_strerror (errno));
+			camel_object_unref ((CamelObject *)message);
+			message = NULL;
+		}
+
+		if (message) {
+			res = TRUE;
+			*message_time = message->date + message->date_offset;
+
+			camel_object_unref ((CamelObject *)message);
+		}
+	}
+
+	if (stream) {
+		camel_object_unref (stream);
+	}
+	return res;
+}
+
 int
 camel_pop3_delete_old(CamelFolder *folder, int days_to_delete,	CamelException *ex)
 {
@@ -363,8 +405,7 @@
 	CamelPOP3FolderInfo *fi;
 	int i;
 	CamelPOP3Store *pop3_store;
-	time_t temp;
-	CamelMessageInfo *minfo;
+	time_t temp, message_time;
 
 	pop3_folder = CAMEL_POP3_FOLDER (folder);
 	pop3_store = CAMEL_POP3_STORE (CAMEL_FOLDER(pop3_folder)->parent_store);
@@ -374,10 +415,8 @@
 	for (i = 0; i < pop3_folder->uids->len; i++) {
 		fi = pop3_folder->uids->pdata[i];
 
-		minfo = camel_folder_get_message_info (folder, fi->uid);
 		d(printf("%s(%d): fi->uid=[%s]\n", __FILE__, __LINE__, fi->uid));
-		if(minfo) {
-			time_t message_time = ((CamelMessageInfoBase *)minfo)->date_received;
+		if (pop3_get_message_time_from_cache (folder, fi->uid, &message_time)) {
 			double time_diff = difftime(temp,message_time);
 			int day_lag = time_diff/(60*60*24);
 
@@ -407,8 +446,6 @@
 					camel_data_cache_remove(pop3_store->cache, "cache", fi->uid, NULL);
 				}
 			}
-			/* free message - not used anymore */
-			camel_folder_free_message_info (folder, minfo);
 		}
 	}