From b6756688b1bf07c50b999c9d30dd6cb224d3812b Mon Sep 17 00:00:00 2001 From: Andrew Stadler Date: Wed, 7 Oct 2009 11:42:27 -0700 Subject: [PATCH] Handle IMAP empty bodies more safely Some IMAP servers return NIL if you BODY.PEEK[TEXT] a messsage with no body, instead of the more canonical {0}CRLF. Instead of messing with the parser to deal with that, it makes more sense not to try and fetch empty bodies. So there are three changes: * Don't fetch parts when size = 0 * Don't append "null" when there is null body text * Slight change to attachment handling so size is reported >0 * Unit tests on some of the related lower-level protocol stuff Bug http://b/issue?id=2160387 Change-Id: Ifb8fb0ed5ce7297908e1ae8d5a02dda5975c4a3c --- src/com/android/email/LegacyConversions.java | 5 +- .../android/email/MessagingController.java | 3 +- .../email/mail/internet/MimeBodyPart.java | 4 ++ .../android/email/mail/store/ImapStore.java | 23 +++---- .../android/email/LegacyConversionsTests.java | 21 +++++++ .../email/mail/store/ImapStoreUnitTests.java | 60 +++++++++++++++++++ 6 files changed, 104 insertions(+), 12 deletions(-) diff --git a/src/com/android/email/LegacyConversions.java b/src/com/android/email/LegacyConversions.java index b03364b46..cd39aeb26 100644 --- a/src/com/android/email/LegacyConversions.java +++ b/src/com/android/email/LegacyConversions.java @@ -222,7 +222,10 @@ public class LegacyConversions { * that deals with single strings. */ private static StringBuffer appendTextPart(StringBuffer sb, String newText) { - if (sb == null) { + if (newText == null) { + return sb; + } + else if (sb == null) { sb = new StringBuffer(newText); } else { if (sb.length() > 0) { diff --git a/src/com/android/email/MessagingController.java b/src/com/android/email/MessagingController.java index 879026ba0..6273f8ede 100644 --- a/src/com/android/email/MessagingController.java +++ b/src/com/android/email/MessagingController.java @@ -1866,7 +1866,8 @@ public class MessagingController implements Runnable { // 3. Generate a shell message in which to retrieve the attachment, // and a shell BodyPart for the attachment. Then glue them together. Message storeMessage = remoteFolder.createMessage(message.mServerId); - BodyPart storePart = new MimeBodyPart(); + MimeBodyPart storePart = new MimeBodyPart(); + storePart.setSize((int)attachment.mSize); storePart.setHeader(MimeHeader.HEADER_ANDROID_ATTACHMENT_STORE_DATA, attachment.mLocation); storePart.setHeader(MimeHeader.HEADER_CONTENT_TYPE, diff --git a/src/com/android/email/mail/internet/MimeBodyPart.java b/src/com/android/email/mail/internet/MimeBodyPart.java index f40e2f5e9..67c2077a7 100644 --- a/src/com/android/email/mail/internet/MimeBodyPart.java +++ b/src/com/android/email/mail/internet/MimeBodyPart.java @@ -134,6 +134,10 @@ public class MimeBodyPart extends BodyPart { return getMimeType().equals(mimeType); } + public void setSize(int size) { + this.mSize = size; + } + public int getSize() throws MessagingException { return mSize; } diff --git a/src/com/android/email/mail/store/ImapStore.java b/src/com/android/email/mail/store/ImapStore.java index 5b98eb3e0..cffc79888 100644 --- a/src/com/android/email/mail/store/ImapStore.java +++ b/src/com/android/email/mail/store/ImapStore.java @@ -755,13 +755,16 @@ public class ImapStore extends Store { for (Object o : fp) { if (o instanceof Part) { Part part = (Part) o; - InputStream bodyStream = fetchList.getLiteral(fetchList.size() - 1); - String contentType = part.getContentType(); - String contentTransferEncoding = part.getHeader( - MimeHeader.HEADER_CONTENT_TRANSFER_ENCODING)[0]; - part.setBody(MimeUtility.decodeBody( - bodyStream, - contentTransferEncoding)); + if (part.getSize() > 0) { + InputStream bodyStream = + fetchList.getLiteral(fetchList.size() - 1); + String contentType = part.getContentType(); + String contentTransferEncoding = part.getHeader( + MimeHeader.HEADER_CONTENT_TRANSFER_ENCODING)[0]; + part.setBody(MimeUtility.decodeBody( + bodyStream, + contentTransferEncoding)); + } } } @@ -1298,9 +1301,9 @@ public class ImapStore extends Store { super(); } - public void setSize(int size) { - this.mSize = size; - } +// public void setSize(int size) { +// this.mSize = size; +// } } class ImapException extends MessagingException { diff --git a/tests/src/com/android/email/LegacyConversionsTests.java b/tests/src/com/android/email/LegacyConversionsTests.java index 49aaf4fee..91674cc96 100644 --- a/tests/src/com/android/email/LegacyConversionsTests.java +++ b/tests/src/com/android/email/LegacyConversionsTests.java @@ -26,6 +26,7 @@ import com.android.email.mail.Part; import com.android.email.mail.Message.RecipientType; import com.android.email.mail.MessageTestUtils.MessageBuilder; import com.android.email.mail.MessageTestUtils.MultipartBuilder; +import com.android.email.mail.internet.MimeBodyPart; import com.android.email.mail.internet.MimeHeader; import com.android.email.mail.internet.MimeMessage; import com.android.email.mail.internet.MimeUtility; @@ -171,6 +172,26 @@ public class LegacyConversionsTests extends ProviderTestCase2 { return message; } + /** + * Basic test of body parts conversion from Store message to Provider message. + * This tests that a null body part simply results in null text, and does not crash + * or return "null". + * + * TODO very incomplete, there are many permutations to be explored + */ + public void testUpdateBodyFieldsNullText() throws MessagingException { + EmailContent.Body localBody = new EmailContent.Body(); + EmailContent.Message localMessage = new EmailContent.Message(); + ArrayList viewables = new ArrayList(); + Part emptyTextPart = new MimeBodyPart(null, "text/plain"); + viewables.add(emptyTextPart); + + // a "null" body part of type text/plain should result in a null mTextContent + boolean result = LegacyConversions.updateBodyFields(localBody, localMessage, viewables); + assertTrue(result); + assertNull(localBody.mTextContent); + } + /** * Sunny day test of adding attachments from an IMAP message. */ diff --git a/tests/src/com/android/email/mail/store/ImapStoreUnitTests.java b/tests/src/com/android/email/mail/store/ImapStoreUnitTests.java index ad22151da..f193a2613 100644 --- a/tests/src/com/android/email/mail/store/ImapStoreUnitTests.java +++ b/tests/src/com/android/email/mail/store/ImapStoreUnitTests.java @@ -16,24 +16,32 @@ package com.android.email.mail.store; +import com.android.email.mail.FetchProfile; import com.android.email.mail.Flag; import com.android.email.mail.Folder; +import com.android.email.mail.Message; import com.android.email.mail.MessagingException; +import com.android.email.mail.Part; import com.android.email.mail.Transport; import com.android.email.mail.Folder.FolderType; import com.android.email.mail.Folder.OpenMode; import com.android.email.mail.internet.BinaryTempFileBody; +import com.android.email.mail.internet.MimeUtility; import com.android.email.mail.transport.MockTransport; import android.test.AndroidTestCase; import android.test.suitebuilder.annotation.SmallTest; +import java.util.ArrayList; import java.util.Date; import java.util.Locale; /** * This is a series of unit tests for the ImapStore class. These tests must be locally * complete - no server(s) required. + * + * To run these tests alone, use: + * $ runtest -c com.android.email.mail.store.ImapStoreUnitTests email */ @SmallTest public class ImapStoreUnitTests extends AndroidTestCase { @@ -224,4 +232,56 @@ public class ImapStoreUnitTests extends AndroidTestCase { int unreadCount = mFolder.getUnreadMessageCount(); assertEquals("getUnreadMessageCount with literal string", 10, unreadCount); } + + /** + * Test for proper operations on servers that return "NIL" for empty message bodies. + */ + public void testNilMessage() throws MessagingException { + MockTransport mock = openAndInjectMockTransport(); + setupOpenFolder(mock); + mFolder.open(OpenMode.READ_WRITE, null); + + // Prepare to pull structure and peek body text - this is like the "large message" + // loop in MessagingController.synchronizeMailboxGeneric() + FetchProfile fp = new FetchProfile();fp.clear(); + fp.add(FetchProfile.Item.STRUCTURE); + Message message1 = mFolder.createMessage("1"); + mock.expect("3 UID FETCH 1 \\(UID BODYSTRUCTURE\\)", new String[] { + "* 1 FETCH (UID 1 BODYSTRUCTURE (TEXT PLAIN NIL NIL NIL 7BIT 0 0 NIL NIL NIL))", + "3 OK SUCCESS" + }); + mFolder.fetch(new Message[] { message1 }, fp, null); + + // The expected result for an empty body is: + // * 1 FETCH (UID 1 BODY[TEXT] {0}) + // But some servers are returning NIL for the empty body: + // * 1 FETCH (UID 1 BODY[TEXT] NIL) + // Because this breaks our little parser, fetch() skips over empty parts. + // The rest of this test is confirming that this is the case. + + mock.expect("4 UID FETCH 1 \\(UID BODY.PEEK\\[TEXT\\]\\)", new String[] { + "* 1 FETCH (UID 1 BODY[TEXT] NIL)", + "4 OK SUCCESS" + }); + ArrayList viewables = new ArrayList(); + ArrayList attachments = new ArrayList(); + MimeUtility.collectParts(message1, viewables, attachments); + assertTrue(viewables.size() == 1); + Part emptyBodyPart = viewables.get(0); + fp.clear(); + fp.add(emptyBodyPart); + mFolder.fetch(new Message[] { message1 }, fp, null); + + // If this wasn't working properly, there would be an attempted interpretation + // of the empty part's NIL and possibly a crash. + + // If this worked properly, the "empty" body can now be retrieved + viewables = new ArrayList(); + attachments = new ArrayList(); + MimeUtility.collectParts(message1, viewables, attachments); + assertTrue(viewables.size() == 1); + emptyBodyPart = viewables.get(0); + String text = MimeUtility.getTextFromPart(emptyBodyPart); + assertNull(text); + } }