James Server
  1. James Server
  2. JAMES-421

MailImpls sharing MimeMessages / LinearProcessor not cloning it after a partial match.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.2.0, 2.3.0
    • Fix Version/s: 2.3.0
    • Component/s: James Core
    • Labels:
      None

      Description

      LinearProcessor match a single recipient for a 2 recipient mail.
      it run "MailImpl.duplicate". duplicate DOES NOT clone the "MimeMessage".
      The following mailet will handle 2 different MailImpl sharing the same MimeMessage.

      Attached is the proving test.

      1. MimeMessageCopyOnWriteProxy.java
        22 kB
        Stefano Bagnara
      2. LinearProcessorTest.java
        8 kB
        Stefano Bagnara
      3. copyonwriteproxy.patch
        47 kB
        Stefano Bagnara

        Activity

        Hide
        Stefano Bagnara added a comment -

        void setUp() create a new LinearProcessor in a mock environment.
        <processor>
        <mailet match="RecipientIs=rec1@domain.com" class="MyMailet">
        </mailet>
        <mailet match="All" class="MyMailet">
        </mailet>
        <mailet match="All" class="DumpSystemErr">
        </mailet>
        </processor>
        Look at the output and you will see that it doesn't match what we expect.
        The test is not finished but looking at System.err you will understand the bug.

        Show
        Stefano Bagnara added a comment - void setUp() create a new LinearProcessor in a mock environment. <processor> <mailet match="RecipientIs=rec1@domain.com" class="MyMailet"> </mailet> <mailet match="All" class="MyMailet"> </mailet> <mailet match="All" class="DumpSystemErr"> </mailet> </processor> Look at the output and you will see that it doesn't match what we expect. The test is not finished but looking at System.err you will understand the bug.
        Hide
        Noel J. Bergman added a comment -

        Mail objects are just addressing carriers for the message. They are not supposed to clone the message, since we may simply be effecting routing within the pipeline. The overhead would be huge if every split in routing caused a clone of the underlying message.

        Show
        Noel J. Bergman added a comment - Mail objects are just addressing carriers for the message. They are not supposed to clone the message, since we may simply be effecting routing within the pipeline. The overhead would be huge if every split in routing caused a clone of the underlying message.
        Hide
        Stefano Bagnara added a comment -

        It doesn't matter what Mail is supposed to do: if this is not a bug in MailImpl it is a bug in LinearProcessor!
        And a critical one: James send a different mail from what it's supposed to send.

        Show
        Stefano Bagnara added a comment - It doesn't matter what Mail is supposed to do: if this is not a bug in MailImpl it is a bug in LinearProcessor! And a critical one: James send a different mail from what it's supposed to send.
        Hide
        Stefano Bagnara added a comment -

        Here is what it happens in my test:
        I send a mail "mail1" to rec1@domain.com and rec2@domain.com and "original body" body.
        The first matcher split the mail1 by duplicating it to "mail1-!27120": it then remove a recipient from mail1 and the other from mail1-!27120.
        mail1 run to the fist MyMailet that change its body to new text 0.
        Both Mails run then through the second MyMailet that should change the body of one mail to "new text 1" and the body of the other mail to "new text 2".
        I then print the 2 bodies: I have 2 messages with "new text 2" body and NO ONE with "new text 1".

        • AbstractRedirect does correctly clone the MimeMessage after the call to MailImpl.duplicate
        • Many mailets just use sendMail with the shared MimeMessage: we should add a comment to mailetContext sendMails to let the use know that we lock the MimeMessage until we processed it and we never change it.
        • LinearProcessor (after partial matching) does duplicate and this way sends multiple Mails with sharing MimeMessage to the following mailets.

        We could solve the issue by cloning the MimeMessage in LinearProcessor but this is too easy to exploit: IMHO we should change the duplicate so that we wrap the object with a "CopyOnWrite" shield: so we can safely share the MimeMessage also when storing it and after multiple operations.

        Show
        Stefano Bagnara added a comment - Here is what it happens in my test: I send a mail "mail1" to rec1@domain.com and rec2@domain.com and "original body" body. The first matcher split the mail1 by duplicating it to "mail1-!27120": it then remove a recipient from mail1 and the other from mail1-!27120. mail1 run to the fist MyMailet that change its body to new text 0. Both Mails run then through the second MyMailet that should change the body of one mail to "new text 1" and the body of the other mail to "new text 2". I then print the 2 bodies: I have 2 messages with "new text 2" body and NO ONE with "new text 1". AbstractRedirect does correctly clone the MimeMessage after the call to MailImpl.duplicate Many mailets just use sendMail with the shared MimeMessage: we should add a comment to mailetContext sendMails to let the use know that we lock the MimeMessage until we processed it and we never change it. LinearProcessor (after partial matching) does duplicate and this way sends multiple Mails with sharing MimeMessage to the following mailets. We could solve the issue by cloning the MimeMessage in LinearProcessor but this is too easy to exploit: IMHO we should change the duplicate so that we wrap the object with a "CopyOnWrite" shield: so we can safely share the MimeMessage also when storing it and after multiple operations.
        Hide
        Stefano Bagnara added a comment -

        Here is a possible fix.

        The attached class should be named MimeMessageWrapper and MimeMessageWrapper should be renamed to MimeMessageLazyLoader so we should be more backward compatible (most code now create MimeMessageWrapper and we want the copy-on-write shield since from the beginning).

        Then the only change needed is update the MailImpl constructor from:
        public MailImpl(String name, MailAddress sender, Collection recipients, MimeMessage message)

        { this(name, sender, recipients); this.setMessage(message); }

        to:
        public MailImpl(String name, MailAddress sender, Collection recipients, MimeMessage message) throws MessagingException

        { this(name, sender, recipients); this.setMessage(new MimeMessageWrapper(message)); }

        This is the constructor used in the MailImpl.duplicate, that is called by the LinearProcessor when splitting the messages.

        Show
        Stefano Bagnara added a comment - Here is a possible fix. The attached class should be named MimeMessageWrapper and MimeMessageWrapper should be renamed to MimeMessageLazyLoader so we should be more backward compatible (most code now create MimeMessageWrapper and we want the copy-on-write shield since from the beginning). Then the only change needed is update the MailImpl constructor from: public MailImpl(String name, MailAddress sender, Collection recipients, MimeMessage message) { this(name, sender, recipients); this.setMessage(message); } to: public MailImpl(String name, MailAddress sender, Collection recipients, MimeMessage message) throws MessagingException { this(name, sender, recipients); this.setMessage(new MimeMessageWrapper(message)); } This is the constructor used in the MailImpl.duplicate, that is called by the LinearProcessor when splitting the messages.
        Hide
        Stefano Bagnara added a comment -

        This is the full patch after adding optimizations in the code that expected to find MimeMessageWrappers instead.
        This should fix the bug without the loss of performance of a full copy every time. Needs testing.

        Show
        Stefano Bagnara added a comment - This is the full patch after adding optimizations in the code that expected to find MimeMessageWrappers instead. This should fix the bug without the loss of performance of a full copy every time. Needs testing.
        Hide
        Stefano Bagnara added a comment -

        The last commit solved this issue. A review of the commit is welcome!

        Show
        Stefano Bagnara added a comment - The last commit solved this issue. A review of the commit is welcome!
        Hide
        Danny Angus added a comment -

        Closing issue fixed in released version.

        Show
        Danny Angus added a comment - Closing issue fixed in released version.

          People

          • Assignee:
            Stefano Bagnara
            Reporter:
            Stefano Bagnara
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development