You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by Niklas Gustavsson <ni...@protocol7.com> on 2012/09/17 14:21:58 UTC
MINA 2.0.5 backwards compatibility issue
Hey
When upgrading to MINA 2.0.5 in FtpServer, I noticed one of the
FtpServer unit tests breaking. Turns out it's due to a change
(r1374997) in MINA 2.0.5 TextLineDecoder which no longer will throw an
exception on some input which is not valid for the charset. For
example, this test will throw an exception on MINA 2.0.4, but will
pass and output a broken String on 2.0.5.
public void testMina205() throws Exception {
IoBuffer buffer =
IoBuffer.wrap(Hex.decodeHex("ae2e0d0a".toCharArray()));
IoSession session = new DummySession();
ProtocolDecoderOutput out = new AbstractProtocolDecoderOutput(){
public void flush(NextFilter nextFilter, IoSession session) {
}};
TextLineDecoder tld = new TextLineDecoder(Charset.forName("UTF-8"));
tld.decode(session, buffer, out);
}
Here's the diff of the change which causes this:
- writeText(session,
buf.getString(ctx.getDecoder()), out);
+ byte[] data = new byte[buf.limit()];
+ buf.get(data);
+ writeText(session, new String(data,
ctx.getDecoder().charset()), out);
I guess new String() is more lax than CharsetDecoder.decode()
I don't think the exact behavior of TextLineDecoder in this case is
specified, but outputting broken strings seems like a bad thing to me.
In the case of FtpServer, this might lead to files being created with
invalid file names, rather then requests being caught as broken (the
string in the above test is from a unit test in FtpServer that comes
from a real request that ones broke FtpServer :)
Should this be fixed?
/niklas
Re: MINA 2.0.5 backwards compatibility issue
Posted by Niklas Gustavsson <ni...@protocol7.com>.
On Tue, Sep 18, 2012 at 12:33 AM, Emmanuel Lécharny <el...@gmail.com> wrote:
> The point is that you are expecting the TextLineDecoder to detect a bad file
> name, instead of checking the file name in the IoHandler. But anyway, I
> think t's not relevant here.
No, I expect TextLineDecoder to detect input it can't correctly decode
(which in the case of FtpServer might be a file name, a command or a
bunch of other things). In fact, anything where I input a bunch of
bytes and get Strings out, I would always expect to err on invalid
input rather then silently (and without the possibility of detecting
it afterwards) create invalid output. I just assume that new String()
is a historical mistake, but I really have no clue as to the
background :)
/niklas
Re: MINA 2.0.5 backwards compatibility issue
Posted by Emmanuel Lécharny <el...@gmail.com>.
Le 9/17/12 8:47 PM, Niklas Gustavsson a écrit :
> On Mon, Sep 17, 2012 at 4:03 PM, Emmanuel Lécharny <el...@gmail.com> wrote:
>> Thinking about it more, I wonder if the fact that we receive bad chars, but
>> still produce a String, is not a better solution ?
>>
>> All in all, you'll be able to see that the file does not exist and return a
>> correct error, instead of handling an exception.
> Well, that will for example not working for putting files. Also, there
> is a proper FTP error reply for invalid requests, and I think that's
> the proper thing to return for broken requests, which is what
> FtpServer correctly does.
The point is that you are expecting the TextLineDecoder to detect a bad
file name, instead of checking the file name in the IoHandler. But
anyway, I think t's not relevant here.
The correct question is : should the TextLine decoder throw an exception
if there is a wrong byte in the incoming bytes, or should it swallow the
wrong byte, and let the String constructor replace this wrong byte by a
'?' char...
Would we say : we should throw an exception, then the patch works, and
that solve your issue as a side effect .
Should we do that ?
--
Regards,
Cordialement,
Emmanuel Lécharny
www.iktek.com
Re: MINA 2.0.5 backwards compatibility issue
Posted by Niklas Gustavsson <ni...@protocol7.com>.
On Mon, Sep 17, 2012 at 4:03 PM, Emmanuel Lécharny <el...@gmail.com> wrote:
> Thinking about it more, I wonder if the fact that we receive bad chars, but
> still produce a String, is not a better solution ?
>
> All in all, you'll be able to see that the file does not exist and return a
> correct error, instead of handling an exception.
Well, that will for example not working for putting files. Also, there
is a proper FTP error reply for invalid requests, and I think that's
the proper thing to return for broken requests, which is what
FtpServer correctly does.
/niklas
Re: MINA 2.0.5 backwards compatibility issue
Posted by Emmanuel Lécharny <el...@gmail.com>.
Le 9/17/12 4:01 PM, Niklas Gustavsson a écrit :
> On Mon, Sep 17, 2012 at 3:33 PM, Emmanuel Lécharny <el...@gmail.com> wrote:
>> I have a fix for the issue, but it will take a bit of time to get a new MINA
>> release cut.
> No worries, I'd rather have this working properly than rush something out.
Thinking about it more, I wonder if the fact that we receive bad chars,
but still produce a String, is not a better solution ?
All in all, you'll be able to see that the file does not exist and
return a correct error, instead of handling an exception.
wdyt ?
--
Regards,
Cordialement,
Emmanuel Lécharny
www.iktek.com
Re: MINA 2.0.5 backwards compatibility issue
Posted by Niklas Gustavsson <ni...@protocol7.com>.
On Mon, Sep 17, 2012 at 3:33 PM, Emmanuel Lécharny <el...@gmail.com> wrote:
> I have a fix for the issue, but it will take a bit of time to get a new MINA
> release cut.
No worries, I'd rather have this working properly than rush something out.
/niklas
Re: MINA 2.0.5 backwards compatibility issue
Posted by Emmanuel Lécharny <el...@gmail.com>.
Le 9/17/12 2:50 PM, Emmanuel Lécharny a écrit :
> Le 9/17/12 2:21 PM, Niklas Gustavsson a écrit :
>> Hey
> yo,
>>
>> When upgrading to MINA 2.0.5 in FtpServer, I noticed one of the
>> FtpServer unit tests breaking. Turns out it's due to a change
>> (r1374997) in MINA 2.0.5 TextLineDecoder which no longer will throw an
>> exception on some input which is not valid for the charset. For
>> example, this test will throw an exception on MINA 2.0.4, but will
>> pass and output a broken String on 2.0.5.
>
> Seems like a regression.
>
> The modification in MINA 2.0.5 was wuite minor, and was expecting to
> fix a problme with lines containing a 0x00 char (which is valid in
> Strings) which was considered -wrongly - as a line terminator.
>
> I' investigating the impact on FtpServer test atm.
>
>
I have a fix for the issue, but it will take a bit of time to get a new
MINA release cut.
Basically, I don't use anymore 'new String()' directly, as it swallows
bad chars and ignore them (they will be replaced by ? in the resulting
String).
The patch is :
Index:
mina-core/src/main/java/org/apache/mina/filter/codec/textline/TextLineDecoder.java
===================================================================
---
mina-core/src/main/java/org/apache/mina/filter/codec/textline/TextLineDecoder.java
(revision 1375418)
+++
mina-core/src/main/java/org/apache/mina/filter/codec/textline/TextLineDecoder.java
(working copy)
@@ -19,6 +19,8 @@
*/
package org.apache.mina.filter.codec.textline;
+import java.nio.ByteBuffer;
+import java.nio.CharBuffer;
import java.nio.charset.CharacterCodingException;
import java.nio.charset.Charset;
import java.nio.charset.CharsetDecoder;
@@ -269,7 +271,11 @@
try {
byte[] data = new byte[buf.limit()];
buf.get(data);
- writeText(session, new String(data,
ctx.getDecoder().charset()), out);
+ CharsetDecoder decoder = ctx.getDecoder();
+
+ CharBuffer buffer =
decoder.decode(ByteBuffer.wrap(data));
+ String str = new String(buffer.array());
+ writeText(session, str, out);
} finally {
buf.clear();
}
--
Regards,
Cordialement,
Emmanuel Lécharny
www.iktek.com
Re: MINA 2.0.5 backwards compatibility issue
Posted by Emmanuel Lécharny <el...@gmail.com>.
Le 9/17/12 2:21 PM, Niklas Gustavsson a écrit :
> Hey
yo,
>
> When upgrading to MINA 2.0.5 in FtpServer, I noticed one of the
> FtpServer unit tests breaking. Turns out it's due to a change
> (r1374997) in MINA 2.0.5 TextLineDecoder which no longer will throw an
> exception on some input which is not valid for the charset. For
> example, this test will throw an exception on MINA 2.0.4, but will
> pass and output a broken String on 2.0.5.
Seems like a regression.
The modification in MINA 2.0.5 was wuite minor, and was expecting to fix
a problme with lines containing a 0x00 char (which is valid in Strings)
which was considered -wrongly - as a line terminator.
I' investigating the impact on FtpServer test atm.
--
Regards,
Cordialement,
Emmanuel Lécharny
www.iktek.com