MessageData Bug (only in special case)

[sappenin]

Hey All,

I found this bug, and am looking for some guidance on how to fix it.

If I draft a message in webmail, and send it to myself, then try to "get" the message via POP3, no POP3 messages are downloaded, and the server throws a NullPointerException as follows, breaking pop3 until I remove the "bad" message from my inbox:

17:46:48,328 ERROR [STDERR] java.lang.NullPointerException
17:46:48,328 ERROR [STDERR] at org.jboss.mail.mailbox.MessageData.getBoundary(MessageData.java:345)
17:46:48,328 ERROR [STDERR] at org.jboss.mail.pop3.handlers.CmdRETR.writeMessage(CmdRETR.java:124)
17:46:48,328 ERROR [STDERR] at org.jboss.mail.pop3.handlers.CmdRETR.org$jboss$mail$pop3$handlers$CmdRETR$handleRequest$aop(CmdRETR.java:92)
17:46:48,328 ERROR [STDERR] at org.jboss.mail.pop3.handlers.CmdRETR$handleRequest_6157744562547821338.invokeNext(CmdRETR$handleRequest_6157744562547821338.java)
17:46:48,328 ERROR [STDERR] at org.jboss.aspects.tx.TxPolicy.invokeInOurTx(TxPolicy.java:79)
17:46:48,328 ERROR [STDERR] at org.jboss.aspects.tx.TxInterceptor$Required.invoke(TxInterceptor.java:197)
17:46:48,328 ERROR [STDERR] at org.jboss.mail.pop3.handlers.CmdRETR$handleRequest_6157744562547821338.invokeNext(CmdRETR$handleRequest_6157744562547821338.java)
17:46:48,328 ERROR [STDERR] at org.jboss.mail.pop3.handlers.CmdRETR.handleRequest(CmdRETR.java)
17:46:48,328 ERROR [STDERR] at org.jboss.mail.pop3.POP3ProtocolInstance.handleRequest(POP3ProtocolInstance.java:211)
17:46:48,328 ERROR [STDERR] at org.jboss.mail.ConnectionHandler.runSocket(ConnectionHandler.java:205)
17:46:48,328 ERROR [STDERR] at org.jboss.mail.ConnectionHandler.run(ConnectionHandler.java:97)
17:46:48,328 ERROR [STDERR] at EDU.oswego.cs.dl.util.concurrent.PooledExecutor$Worker.run(PooledExecutor.java:743)
17:46:48,328 ERROR [STDERR] at java.lang.Thread.run(Thread.java:595)

I've tracked this code down to the getBoundary() function of the MessageData class, here:

String[] header = getHeader("Content-Type").split("\\r\\n");

If I inspect the headers of a "JBCS webmail" created message, there isn't a "Content-type" header, which is why (I'm guessing) a null pointer is thrown. The fix here may just be to check for a null string after calling getHeader("Content-Type").

However, I'm not quite sure if this is the right thing. Why does the boundary pivot around the "content-type" header? Does the SMTP spec mandate that everything after this header must be the body? Is content-type guaranteed to be last? I'll write a fix for this, but I'm not quite sure what it's supposed to be doing. I can see that CMDRetry.java line 124 is apparently using this a a boundary between message bodies (?).

Thanks!

David

[sappenin]

Ok, after a little bit of investigation, I was able to figure out that the "boundary" is for MIME messages. Essentially, each MIME part is separated by a boundary with a boundary id. So, "getBoundary()" is simply trying to detect if there are any MIME boundary parts.

So, I discovered a similar looking function to MessageData's getBoundary() function (with the same name) in the MailHeadersImpl class. It looks like the following:

/**
 * Find the mime boundry string in the content-type header
 * @param header from ih which contains "boundary" potentially
 * @return null if there is NO boundary entry or the actual boundry entry.
 */
 public String getBoundary() {

 String retval = null;
 String[] header = getHeader("Content-Type");

 try {
 for (int i = 0; header != null && i < header.length; i++) {
 if( header.indexOf("boundary=\"") >-1) {
 String[] temp = header.split("boundary\\=\\\"");
 String boundary = temp[temp.length-1];
 boundary = "--"+boundary.split("\\\"")[0];
 retval = boundary;
 break;
 }
 }
 } catch (Exception e) {
 //don't do anything we'll just NOT parse the mime.
 }
 return retval;
 }

Notice that it checks for a null string. So, I proprose that the fix for the issue above simply be using the function syntax in the MessageData class.

One question remains: Would it make sense to adjust the design of the MessageData class to be backed by a Mail object? That way, the "getBoundary" function (and other shared functions) would only need to be coded/maintained once (inside the Mail class). All of the "getter"/setter functions in the MessageData could just forward to the internal Mail object (which is passed in via the public constructor). I know this is probably not a crucial tweak to perform, but it might be worth considering as a future goal?

In the meantime, I'll submit a JIRA issue for this change.

Thanks!

David

[acoliver]

Thanks for finding this. I have a more severe mime rewrite that I'm doing which uses a fork of MIME4J (they have a kinda dumb handling for mail bodies that I made pluggable among other minor things).

I don't know what real advantage we would get by backing the messagedata object with a mail object. Essentailly messagedata is a parsed/persisted mail object.

[sappenin]

See JIRA issue here + patch:

http://jira.jboss.org/jira/browse/JBMAIL-247

Essentially, I moved the creation of the String array into the try/catch block. If an exception is thrown (i.e., no "content-type" header) then it is eaten and ignored.

Note: I kept the "split" around the newline since stored messages have a newline after the "content-type" label. I guess this is one argument in favor of not merging MessageData and Mail classes (i.e., backing a MessageData class with a Mail object per my previous suggestion below) because this function behaves differently in each class.

String[] header = getHeader("Content-Type").split("\\r\\n");

[acoliver]

great. I should be done sometime next week with my rewrite of this code (to use mime4j). I'll merge it in then.