James Mime4j
  1. James Mime4j
  2. MIME4J-79

Message.writeTo() prematurely closes output stream if transfer encoding is BASE64

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 0.5
    • Fix Version/s: 0.5
    • Component/s: None
    • Labels:
      None

      Description

      Entity.writeTo(OutputStream out, int mode) wraps the output stream in a Base64OutputStream if the transfer encoding is BASE64. Later the wrapper stream gets closed and (despite of a comment that says otherwise) the inner stream gets closed, too.

      1. Base64BugDemo.java
        2 kB
        Markus Wiederkehr
      2. mime4j-base64.patch
        2 kB
        Markus Wiederkehr
      3. mime4j-close-codec.patch
        9 kB
        Oleg Kalnichevski
      4. mime4j-close-codec-ioex.patch
        33 kB
        Markus Wiederkehr

        Activity

        Hide
        Robert Burrell Donkin added a comment -

        Closing all issues fixed previously, after a brief review of each.

        Show
        Robert Burrell Donkin added a comment - Closing all issues fixed previously, after a brief review of each.
        Hide
        Markus Wiederkehr added a comment -

        @Stefano: Sorry, I did not mean to imply that it does any harm. Thanks for applying the patch.

        Show
        Markus Wiederkehr added a comment - @Stefano: Sorry, I did not mean to imply that it does any harm. Thanks for applying the patch.
        Hide
        Stefano Bagnara added a comment -

        @Markus: as we don't actually throw the IllegalArgumentException I prefer to deal with a possible len == -1 updating the closed variable. Of course the whole thing could be refactored to do that code in another method and not "abuse" the "standard" write method. Do you see any harm with the change I made?

        Show
        Stefano Bagnara added a comment - @Markus: as we don't actually throw the IllegalArgumentException I prefer to deal with a possible len == -1 updating the closed variable. Of course the whole thing could be refactored to do that code in another method and not "abuse" the "standard" write method. Do you see any harm with the change I made?
        Hide
        Markus Wiederkehr added a comment -

        Adding "closed = true" would not have been necessary because this branch only gets executed if len < 0.

        len < 0 would normally be an illegal argument for this method and should be punished by throwing an IllegalArgumentException. In case of this implementation it is used by close() to indicate EOF. And close() already takes care of setting closed to true in its finally block.

        Show
        Markus Wiederkehr added a comment - Adding "closed = true" would not have been necessary because this branch only gets executed if len < 0. len < 0 would normally be an illegal argument for this method and should be punished by throwing an IllegalArgumentException. In case of this implementation it is used by close() to indicate EOF. And close() already takes care of setting closed to true in its finally block.
        Hide
        Stefano Bagnara added a comment -

        Patch applied.

        The only change from the proposed patch is the "closed = true" when "len < 0" (in Base64OutputStream) because len == -1 is the way we are notified of a stream end and writing more data would result in corrupted output, so it seems safer to consider the stream closed after that call.

        I also changed some really trivial code formatting things.

        Show
        Stefano Bagnara added a comment - Patch applied. The only change from the proposed patch is the "closed = true" when "len < 0" (in Base64OutputStream) because len == -1 is the way we are notified of a stream end and writing more data would result in corrupted output, so it seems safer to consider the stream closed after that call. I also changed some really trivial code formatting things.
        Hide
        Markus Wiederkehr added a comment -

        Before this gets postponed please review my mime4j-close-codec-ioex.patch.

        It is based on Oleg's patch with the following changes:

        • encoders / decoders throw IOException from read / write once closed
        • makes QuotedPrintableOutputStream a public class
        • QuotedPrintableOutputStream.write(int) no longer throws an UnsupportedOperationException
        • unit tests for QuotedPrintableOutputStream
        • QuotedPrintableEncoder becomes a package private class
        Show
        Markus Wiederkehr added a comment - Before this gets postponed please review my mime4j-close-codec-ioex.patch. It is based on Oleg's patch with the following changes: encoders / decoders throw IOException from read / write once closed makes QuotedPrintableOutputStream a public class QuotedPrintableOutputStream.write(int) no longer throws an UnsupportedOperationException unit tests for QuotedPrintableOutputStream QuotedPrintableEncoder becomes a package private class
        Hide
        Stefano Bagnara added a comment -

        My bad.. I fixed this issue in the old Base64OutputStream implementation and I forgot about it when I changed the implementation. I thought "tests pass, so it must be ok".. mea culpa I didn't wrote a unit test the first time I fixed it.

        The patch proposed by Oleg seems fine; Markus hint about the IOException may be appropriate but the class is for internal use ATM, and it should never happen to call that methods when the stream is closed in mime4j, so we can improve this even in a future release.

        Show
        Stefano Bagnara added a comment - My bad.. I fixed this issue in the old Base64OutputStream implementation and I forgot about it when I changed the implementation. I thought "tests pass, so it must be ok".. mea culpa I didn't wrote a unit test the first time I fixed it. The patch proposed by Oleg seems fine; Markus hint about the IOException may be appropriate but the class is for internal use ATM, and it should never happen to call that methods when the stream is closed in mime4j, so we can improve this even in a future release.
        Hide
        Markus Wiederkehr added a comment -

        One more thing that you might want to consider is the general contract of InputStream / OutputStream. Although it is more slack than the one of Reader / Writer it seems to indicate that an IOException should be thrown in read() / write() once the stream has been closed. Closing an already closed stream should have no effect.

        For example see InputStream.read(byte[]): "Throws: IOException - If the first byte cannot be read for any reason other than the end of the file, if the input stream has been closed, or if some other I/O error occurs."

        Show
        Markus Wiederkehr added a comment - One more thing that you might want to consider is the general contract of InputStream / OutputStream. Although it is more slack than the one of Reader / Writer it seems to indicate that an IOException should be thrown in read() / write() once the stream has been closed. Closing an already closed stream should have no effect. For example see InputStream.read(byte[]): "Throws: IOException - If the first byte cannot be read for any reason other than the end of the file, if the input stream has been closed, or if some other I/O error occurs."
        Hide
        Markus Wiederkehr added a comment -

        I think this is a good idea and your patch looks fine to me.

        But maybe the same concept should also be applied to QuotedPrintableOutputStream? Although it is an inner class of CodecUtil it can be accessed the same way as the base64 version through wrapQuotedPrintable(OutputStream, boolean). Maybe it should even become a public class for consistency reasons..

        Show
        Markus Wiederkehr added a comment - I think this is a good idea and your patch looks fine to me. But maybe the same concept should also be applied to QuotedPrintableOutputStream? Although it is an inner class of CodecUtil it can be accessed the same way as the base64 version through wrapQuotedPrintable(OutputStream, boolean). Maybe it should even become a public class for consistency reasons..
        Hide
        Oleg Kalnichevski added a comment -

        Slightly different take at fixing the problem. Encoding / decoding streams no longer close the underlying stream. They become unreadable / non-writable instead.

        Here's the patch includes test cases. Please review.

        Oleg

        Show
        Oleg Kalnichevski added a comment - Slightly different take at fixing the problem. Encoding / decoding streams no longer close the underlying stream. They become unreadable / non-writable instead. Here's the patch includes test cases. Please review. Oleg
        Hide
        Markus Wiederkehr added a comment -

        If there are no use cases where the underlying stream should be closed as well you could simply remove the statement out.close() from Base64OutputStream.close() (line 393) to resolve the issue.

        Show
        Markus Wiederkehr added a comment - If there are no use cases where the underlying stream should be closed as well you could simply remove the statement out.close() from Base64OutputStream.close() (line 393) to resolve the issue.
        Hide
        Oleg Kalnichevski added a comment -

        I think content encoding / decoding streams MUST NOT close the underlying stream EVER, as the encoded content may be just a part of a larger message.

        Oleg

        Show
        Oleg Kalnichevski added a comment - I think content encoding / decoding streams MUST NOT close the underlying stream EVER, as the encoded content may be just a part of a larger message. Oleg
        Hide
        Markus Wiederkehr added a comment -

        The attached patch mime4j-base64.patch would resolve the problem but you might prefer another solution..

        Show
        Markus Wiederkehr added a comment - The attached patch mime4j-base64.patch would resolve the problem but you might prefer another solution..
        Hide
        Markus Wiederkehr added a comment -

        See attachment Base64BugDemo.java for a simple test that shows the problem.

        Show
        Markus Wiederkehr added a comment - See attachment Base64BugDemo.java for a simple test that shows the problem.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development