Uploaded image for project: 'James Server'
  1. James Server
  2. JAMES-609

MailImpl.setMessage and possible NPE: regression from 2.2.0 and 2.3.0rc1

    Details

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

      Description

      The change introduced in rc2 to fix the file leaks has produced an NPE in a custom mailet.

      I suggest to revert the setMessage changes removing the dispose() call for the old message and to put the dispose code in the bundled calling mailets.

      This is not a good fix in the long term, but for 2.3.0 I would prefer to keep compatibility with 2.2.0 and to have a leak instead of NPE (difficult to understand by most users).

      1. JAMES-609.diff
        20 kB
        Stefano Bagnara

        Activity

        Hide
        bago Stefano Bagnara added a comment -

        I believe the NPE could be thrown when saving the message after having called SMIMECheckSignature when "stripSignature && strippedMessage != null" because it calls setMessage with the same message.

        Show
        bago Stefano Bagnara added a comment - I believe the NPE could be thrown when saving the message after having called SMIMECheckSignature when "stripSignature && strippedMessage != null" because it calls setMessage with the same message.
        Hide
        bago Stefano Bagnara added a comment -

        This is a possible fix to the problem.
        Few changes are on the tests to solve some temporary file not release (24 before the fix, 20 after the fix).

        Now that I wrote it I'm not sure about this in 2.3: they are a lot of changes... maybe it is better to add a check in setMessage to see if the new message is the same of the old one...

        I have to think more about this...

        Show
        bago Stefano Bagnara added a comment - This is a possible fix to the problem. Few changes are on the tests to solve some temporary file not release (24 before the fix, 20 after the fix). Now that I wrote it I'm not sure about this in 2.3: they are a lot of changes... maybe it is better to add a check in setMessage to see if the new message is the same of the old one... I have to think more about this...
        Hide
        noel Noel J. Bergman added a comment -

        I agree that it would be better to check in setMessage if this.message == message.

        None of these is a perfect solution, since the explicit dispose call reintroduces the problems that exist in other languages with explicit deallocation: how to know when it is safe to deallocate (and clean-up). What if someone were to call getMessage, put the old message object into a new Mail, and put a new message in the old Mail?

        But the least objectionable change for now is probably the check in setMessage. The logic for that one is clear: if we are setting the message to the one we already have, do nothing.

        Show
        noel Noel J. Bergman added a comment - I agree that it would be better to check in setMessage if this.message == message. None of these is a perfect solution, since the explicit dispose call reintroduces the problems that exist in other languages with explicit deallocation: how to know when it is safe to deallocate (and clean-up). What if someone were to call getMessage, put the old message object into a new Mail, and put a new message in the old Mail? But the least objectionable change for now is probably the check in setMessage. The logic for that one is clear: if we are setting the message to the one we already have, do nothing.
        Hide
        bago Stefano Bagnara added a comment -

        We agreed to fix this with the small "identity check" fix.
        Applied to both 2.3 branch and trunk.

        Show
        bago Stefano Bagnara added a comment - We agreed to fix this with the small "identity check" fix. Applied to both 2.3 branch and trunk.
        Hide
        danny@apache.org Danny Angus added a comment -

        Closing issue fixed in released version.

        Show
        danny@apache.org Danny Angus added a comment - Closing issue fixed in released version.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development