Details

Description

If you for example try to connect to a non-existing database using the client driver, and the name of the database has 18 characters or more, and at least one of the characters in the database name is a non-ascii character, the server will throw an IllegalArgumentException when trying to send the "database not found" message back to the client.

The problem appears to be that DRDAConnThread.writeRDBNAM() uses the length of the string in characters when it calls writeScalarPaddedBytes(). writeScalarPaddedBytes() expects the length in bytes, and gets confused when paddedLength is less than buf.length.

The reason why we need at least 18 characters in the name in order to see the bug, is that the database name is always padded to at least 18 bytes (DRDA requirement).

The attached patch (fix.diff) makes writeRDBNAM() use number of bytes instead of number of characters. I'll see if I can come up with a regression test for the bug.

Knut Anders Hatlen
added a comment - 14/Sep/10 07:47 The problem appears to be that DRDAConnThread.writeRDBNAM() uses the length of the string in characters when it calls writeScalarPaddedBytes(). writeScalarPaddedBytes() expects the length in bytes, and gets confused when paddedLength is less than buf.length.
The reason why we need at least 18 characters in the name in order to see the bug, is that the database name is always padded to at least 18 bytes (DRDA requirement).
The attached patch (fix.diff) makes writeRDBNAM() use number of bytes instead of number of characters. I'll see if I can come up with a regression test for the bug.

Just some thoughts on this. My changes will make it so that a 10.7 client on a 10.7 server never sees this issue. However, a 10.7 client working with an old server will still see it as it will revert to EBCDIC.

Tiago R. Espinha
added a comment - 14/Sep/10 07:47 I think perhaps we should link this with DERBY-728 ?
Just some thoughts on this. My changes will make it so that a 10.7 client on a 10.7 server never sees this issue. However, a 10.7 client working with an old server will still see it as it will revert to EBCDIC.

Will your changes in 10.7 make the error handling stop using writeRDBNAM()? This is not code that uses EBCDIC in the first place, so I was under the impression that it wouldn't be touched by the DERBY-4757 changes.

For the record, the connect command in the bug description is supposed to fail, because the database isn't found. Here's what happens with the fix applied:

ij> connect 'jdbc:derby://localhost/abcdefghijklmnopqå';
ERROR 08004: The connection was refused because the database abcdefghijklmnopqå was not found.

This appears to only be a problem in the case where we expect a failure. If I add create=true to the URL so that the database is created, it works fine also without the fix.

Knut Anders Hatlen
added a comment - 14/Sep/10 08:13 Thanks, I've linked it to DERBY-728 .
Will your changes in 10.7 make the error handling stop using writeRDBNAM()? This is not code that uses EBCDIC in the first place, so I was under the impression that it wouldn't be touched by the DERBY-4757 changes.
For the record, the connect command in the bug description is supposed to fail, because the database isn't found. Here's what happens with the fix applied:
ij> connect 'jdbc:derby://localhost/abcdefghijklmnopqå';
ERROR 08004: The connection was refused because the database abcdefghijklmnopqå was not found.
This appears to only be a problem in the case where we expect a failure. If I add create=true to the URL so that the database is created, it works fine also without the fix.

Tiago R. Espinha
added a comment - 14/Sep/10 08:20 I'm confused now. By looking at your patch, I see that you use "server.DEFAULT_ENCODING" to obtain the bytes, which is in fact UTF8.
Shouldn't all the DRDA commands and arguments until now be using EBCDIC?
My changes do go into this method and rather than using a .length() call, I use the CCSID manager (whichever it is currently) to obtain the length in bytes.

I think we've been sending the contents of the DRDA commands as UTF-8 all the time, and only the command headers as EBCDIC. Not sure exactly where the line was drawn between EBCDIC and UTF-8. I was also a bit surprised that we didn't use EBCDIC here.

I checked the latest patch on DERBY-4757 and didn't see that it touched this method. Is this something you've planned to add in the next revision of the patch? If it already uses UTF-8, I suppose it shouldn't be changed by DERBY-4757 to use EBCDIC against older clients.

Knut Anders Hatlen
added a comment - 14/Sep/10 09:00 I think we've been sending the contents of the DRDA commands as UTF-8 all the time, and only the command headers as EBCDIC. Not sure exactly where the line was drawn between EBCDIC and UTF-8. I was also a bit surprised that we didn't use EBCDIC here.
I checked the latest patch on DERBY-4757 and didn't see that it touched this method. Is this something you've planned to add in the next revision of the patch? If it already uses UTF-8, I suppose it shouldn't be changed by DERBY-4757 to use EBCDIC against older clients.

Now, if we consider that up until now the only CCSID manager was of the type EbcdicCcsidManager, then the client has always been encoding the RDBNAM in EBCDIC. It is only the server, when it has to send the RDBNAM back to the client (it happens in certain occasions), that will send it in UTF-8 as seen on the method you've changed.

I'm not sure this follows the specification and with my changes, rather than getting the length like this, I was pulling it from the CCSID manager's getByteLength() method. I reckon the override for this method requires some adjusting in the EBCDIC case, but I think this would be more correct from a DRDA perspective.

Tiago R. Espinha
added a comment - 14/Sep/10 09:47 I think this is just the behavior on the server. If you look at NetConnectionRequest.buildRDBNAM(String, boolean) you'll find this call:
(line 488)
netAgent_.getCurrentCcsidManager().convertFromJavaString(rdbnam, netAgent_);
Now, if we consider that up until now the only CCSID manager was of the type EbcdicCcsidManager, then the client has always been encoding the RDBNAM in EBCDIC. It is only the server, when it has to send the RDBNAM back to the client (it happens in certain occasions), that will send it in UTF-8 as seen on the method you've changed.
I'm not sure this follows the specification and with my changes, rather than getting the length like this, I was pulling it from the CCSID manager's getByteLength() method. I reckon the override for this method requires some adjusting in the EBCDIC case, but I think this would be more correct from a DRDA perspective.

NetConnectionReply.parseRDBNAM() is the method that parses the RDBNAM value sent from the server, and it indeed uses the CCSID manager. The reason why it has worked is probably that all the callers of that method have called it with skip==true, so that none of them actually do any decoding of the string. They just skip the specified number of bytes.

So I agree, changing DRDAConnThread.writeRDBNAM() so that it uses the CCSID manager would solve this bug. And that it switches to EBCDIC when talking to older clients is not a problem since they don't look at the value anyways.

Should I hold off committing this fix and just check in the test to verify that the problem is solved once you've finished up DERBY-4757?

Knut Anders Hatlen
added a comment - 14/Sep/10 10:28 NetConnectionReply.parseRDBNAM() is the method that parses the RDBNAM value sent from the server, and it indeed uses the CCSID manager. The reason why it has worked is probably that all the callers of that method have called it with skip==true, so that none of them actually do any decoding of the string. They just skip the specified number of bytes.
So I agree, changing DRDAConnThread.writeRDBNAM() so that it uses the CCSID manager would solve this bug. And that it switches to EBCDIC when talking to older clients is not a problem since they don't look at the value anyways.
Should I hold off committing this fix and just check in the test to verify that the problem is solved once you've finished up DERBY-4757 ?

Tiago R. Espinha
added a comment - 14/Sep/10 11:13 > Should I hold off committing this fix and just check in the test to verify that the problem is solved once you've finished up DERBY-4757 ?
I think that's the better option. I have a few changes that aren't on the submitted patches and I've just been waiting on that discussion on the list to see if I should go forward or hold out on it.