4 Replies Latest reply on Aug 23, 2006 1:40 PM by acoliver

    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


        • 1. Re: MessageData Bug (only in special case)
          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

          • 2. Re: MessageData Bug (only in special case)
            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.

            • 3. Re: MessageData Bug (only in special case)
              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");


              • 4. Re: MessageData Bug (only in special case)
                acoliver

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