DDMReader readBytes ArrayIndexOutOfBoundsException

• Key: DERBY-4088
• Type: Bug
• Status: Resolved
• Resolution: Fixed
• Priority: Major
• Assignee: Knut Anders Hatlen
• Reporter: Urban Widmark
• Component/s: Network Server
• Affects Version/s: 10.4.2.0
• Fix Version/s: 10.4.2.1, 10.5.1.1

Description

DDMReader.readBytes(int length) checks the length vs DssConstants.MAX_DSS_LENGTH, but ignores the fact that the buffer position "pos" might not be 0. If pos is non-zero then the pos + length can be larger than the size of "buffer" causing an ArrayIndexOutOfBoundsException.

For me this happened when sending a BLOB that was 32766 bytes long. The value of pos was 2 in that method.

Comments

[Urban Widmark]

Attached patch changes the if-statement to include buffer position. I have only tested this with my own database and the 32766 byte BLOB where it seems to work fine.

[Knut Anders Hatlen]

Thanks for the bug report. If possible, could you also provide step-by-step description of how to reproduce the bug? The stack trace of the ArrayIndexOutOfBoundsException would also be helpful.

I haven't been able to reproduce the bug myself, so I can't say if the patch is correct. My understanding of that code is that the check against MAX_DSS_LENGTH is used to determine the format of the byte string, which should not be affected by the current position in the read buffer. It is the call to ensureBLayerDataInBuffer() that is supposed to ensure that the read buffer is large enough and prevent that the AIOOBE is thrown. So my initial reaction is that the bug is probably somewhere else.

[Urban Widmark]

I am not necessarily saying the patch is correct, just that making the change made it work for me. I do not claim any knowledge whatsoever about DRDA or DSS :) Have also verified that the data is transfered correctly after the patch by checking md5 sums of the data objects inside and outside the database.

readBytes is called with pos=2, length=32766, the buffer length is 32767 and is not changed by the call to ensureBLayerDataInBuffer.

Server stacktrace:
java.lang.ArrayIndexOutOfBoundsException
        at java.lang.System.arraycopy(Native Method)
        at org.apache.derby.impl.drda.DDMReader.readBytes(Unknown Source)
        at org.apache.derby.impl.drda.DRDAConnThread.readAndSetParams(Unknown Source)
        at org.apache.derby.impl.drda.DRDAConnThread.parseSQLDTA_work(Unknown Source)
        at org.apache.derby.impl.drda.DRDAConnThread.parseSQLDTA(Unknown Source)
        at org.apache.derby.impl.drda.DRDAConnThread.parseEXCSQLSTTobjects(Unknown Source)
        at org.apache.derby.impl.drda.DRDAConnThread.parseEXCSQLSTT(Unknown Source)
        at org.apache.derby.impl.drda.DRDAConnThread.processCommands(Unknown Source)
        at org.apache.derby.impl.drda.DRDAConnThread.run(Unknown Source)

Client stacktrace:
org.apache.derby.client.am.DisconnectException: The DDM object 0x1232 is not supported. The connection has been terminated.
        at org.apache.derby.client.net.NetConnectionReply.doObjnsprmSemantics(Unknown Source)
        at org.apache.derby.client.net.NetConnectionReply.parseCommonError(Unknown Source)
        at org.apache.derby.client.net.NetStatementReply.parseExecuteError(Unknown Source)
        at org.apache.derby.client.net.NetStatementReply.parseEXCSQLSTTreply(Unknown Source)
        at org.apache.derby.client.net.NetStatementReply.readExecute(Unknown Source)
        at org.apache.derby.client.net.StatementReply.readExecute(Unknown Source)
        at org.apache.derby.client.net.NetPreparedStatement.readExecute_(Unknown Source)
        at org.apache.derby.client.am.PreparedStatement.readExecute(Unknown Source)
        at org.apache.derby.client.am.PreparedStatement.flowExecute(Unknown Source)
        at org.apache.derby.client.am.PreparedStatement.executeUpdateX(Unknown Source)
        ... 2 more

I am also attaching a tiny testprogram that triggers this bug for me. Reproduced by:

java -classpath derbynet.jar org.apache.derby.drda.NetworkServerControl start
javac DerbyBug.java && java -cp .:derbyclient.jar DerbyBug

Database definition inside the java source. Tested vs 10.4.2.0 binary release and 10.4 svn branch.

[Urban Widmark]

Testprogram that triggers the problem for me.

[Kristian Waagan]

I downloaded the test program, and can confirm that it triggers the bug.
Note that you should use Java SE 6 and derbyclient.jar, because the drivers isn't loaded explicitly by the test program.

Here's the stack trace:
2009-03-10 15:04:28.064 GMT:
 Booting Derby version The Apache Software Foundation - Apache Derby - 10.5.0.0 alpha - (752129M): instance a816c00e-011f-f0e9-8206-000003d35e00
on database directory /tmp/db

Database Class Loader started - derby.database.classpath=''
2009-03-10 15:05:36.546 GMT Thread[DRDAConnThread_3,5,main] (DATABASE = db), (DRDAID = NF000001.D3F3-4110940798161594440{2}), null
2009-03-10 15:05:36.546 GMT : null
java.lang.ArrayIndexOutOfBoundsException
        at java.lang.System.arraycopy(Native Method)
        at org.apache.derby.impl.drda.DDMReader.readBytes(DDMReader.java:1509)
        at org.apache.derby.impl.drda.DRDAConnThread.readAndSetParams(DRDAConnThread.java:4697)
        at org.apache.derby.impl.drda.DRDAConnThread.parseSQLDTA_work(DRDAConnThread.java:4507)
        at org.apache.derby.impl.drda.DRDAConnThread.parseSQLDTA(DRDAConnThread.java:4379)
        at org.apache.derby.impl.drda.DRDAConnThread.parseEXCSQLSTTobjects(DRDAConnThread.java:4254)
        at org.apache.derby.impl.drda.DRDAConnThread.parseEXCSQLSTT(DRDAConnThread.java:4084)
        at org.apache.derby.impl.drda.DRDAConnThread.processCommands(DRDAConnThread.java:1003)
        at org.apache.derby.impl.drda.DRDAConnThread.run(DRDAConnThread.java:290)
2009-03-10 15:05:36.547 GMT Thread[DRDAConnThread_3,5,main] (DATABASE = db), (DRDAID = NF000001.D3F3-4110940798161594440{2}), Execution failed because of Permanent Agent Error: SVRCOD = 40; RDBNAM = db; diagnostic msg = null
2009-03-10 15:05:36.547 GMT : Execution failed because of Permanent Agent Error: SVRCOD = 40; RDBNAM = db; diagnostic msg = null
org.apache.derby.impl.drda.DRDAProtocolException: Execution failed because of Permanent Agent Error: SVRCOD = 40; RDBNAM = db; diagnostic msg = null
        at org.apache.derby.impl.drda.DRDAProtocolException.newAgentError(DRDAProtocolException.java:339)
        at org.apache.derby.impl.drda.DRDAConnThread.sendUnexpectedException(DRDAConnThread.java:7993)
        at org.apache.derby.impl.drda.DRDAConnThread.handleException(DRDAConnThread.java:7944)
        at org.apache.derby.impl.drda.DRDAConnThread.run(DRDAConnThread.java:315)
2009-03-10 15:05:36.547 GMT : null
java.lang.ArrayIndexOutOfBoundsException
        at java.lang.System.arraycopy(Native Method)
        at org.apache.derby.impl.drda.DDMReader.readBytes(DDMReader.java:1509)
        at org.apache.derby.impl.drda.DRDAConnThread.readAndSetParams(DRDAConnThread.java:4697)
        at org.apache.derby.impl.drda.DRDAConnThread.parseSQLDTA_work(DRDAConnThread.java:4507)
        at org.apache.derby.impl.drda.DRDAConnThread.parseSQLDTA(DRDAConnThread.java:4379)
        at org.apache.derby.impl.drda.DRDAConnThread.parseEXCSQLSTTobjects(DRDAConnThread.java:4254)
        at org.apache.derby.impl.drda.DRDAConnThread.parseEXCSQLSTT(DRDAConnThread.java:4084)
        at org.apache.derby.impl.drda.DRDAConnThread.processCommands(DRDAConnThread.java:1003)
        at org.apache.derby.impl.drda.DRDAConnThread.run(DRDAConnThread.java:290)

[Knut Anders Hatlen]

Thanks, I see the exception too now.

I haven't worked out all the details yet, but here's what I've found
so far:

readBytes() calls ensureBLayerDataInBuffer() which again calls
ensureALayerDataInBuffer(). ensureALayerDataInBuffer() ends up
enlarging the buffer and shifting the contents to the left so there is
enough space in the buffer (there is in fact enough space in the
buffer when that method returns) and fills the buffer with data.

After calling ensureALayerDataInBuffer(), ensureBLayerDataInBuffer()
detects that the DSS is shorter than the requested length, which means
that the data continues in a subsequent DSS. It calls
compressBLayerData() to remove DSS header bytes from the buffer so
that the buffer only contains the pure byte string and nothing
else. Since we only requested length bytes from the network, and some
of those bytes are header bytes, this means that the buffer contains
less than length bytes after we have "compressed" the buffer. This is
what's eventually causing the exception when we try to copy more bytes
from the buffer than we actually have available.

I think one possible fix is to make compressBLayerData() fill the
buffer with the missing bytes from the continuation DSS.

(One other thing to investigate is why the client splits the data into
smaller chunks when it is small enough to fit in a single DSS. It
sounds suboptimal.)

[Bryan Pendleton]

> why the client splits the data into smaller chunks when it is small enough to fit in a single DSS

Yes, this is quite intriguing. It might be more than suboptimal. I vaguely recall thinking
that the algorithms for assembling fragments across buffers assumed that the
only 'short' segment was the last segment.

But it's been several years since I studied this code, so I may be misremembering.

[Knut Anders Hatlen]

Here's a first attempt to fix the bug. I haven't investigated why the
client splits the value yet, but since the protocol seems to allow it,
it seems like we should fix it on the server anyway.

The important part of the patch is that it adds calls to
ensureALayerDataInBuffer() two places in compressBLayerData() to
ensure that we never try to access bytes that haven't been buffered
yet. compressBLayerData() takes multiple DSSs and concatenates them so
that they look like a single DSS for the caller. Since the new calls
to ensureALayerDataInBuffer() ensures that we have all the bytes for
the low-level DSSs in the buffer, it means that we also have all the
bytes needed to construct the concatenated "super DSS" created by
compressBLayerData(). This again ensures that when
ensureBLayerDataInBuffer() returns in readBytes(), we have the entire
byte string in the buffer, and System.arraycopy() won't go out of
bounds.

(Some might find it a bit confusing that compressBLayerData() calls
ensureALayerDataInBuffer() and not ensureBLayerDataInBuffer(). The
explanation is that ensureBLayerDataInBuffer() uses
ensureALayerDataInBuffer() as a lower-level primitive to actually
fetch the physical bytes, whereas the Layer B method provides a
higher-level logical view of those bytes. Representing a sequence of
DSSs as a single DSS is one example of this. compressBLayerData() is
performing work for ensureBLayerDataInBuffer() to turn the physical
view of the bytes into the logical view, and therefore needs to use
the Layer A methods that give the physical view.)

Now, the use of ensureALayerDataInBuffer() in compressBLayerData()
made it necessary to make some more changes. compressBLayerData() has
a local variable called tempPos which specifies a position in the read
buffer. This position variable is used to access parts of the read
buffer without destructively skipping bytes up to the interesting
portion of the buffer. This is effectively the same as the position of
the first valid byte in the read buffer plus an offset, and it depends
on the position of the first valid byte being constant throughout the
method.

However, ensureALayerDataInBuffer() may shift bytes to the left in
order to free up space at the end of the buffer. Then the position of
the first valid byte will change, and tempPos will need to be updated
accordingly. To compensate for this shift, I renamed the tempPos
variable to tempOffset and replaced all occurrences of tempPos with
(pos + tempOffset). This means that the local variable is just an
offset from the start of the valid region of the buffer, and it
doesn't change even if the valid region is shifted left or
right. Since pos is added to the offset each time it is used, we'll
automatically compensate for the shift.

I haven't run the full regression test suite yet, but I have added a
test case to derbynet/PrepareStatementTest that fails with an
ArrayIndexOutOfBoundsException without the fix and passes with the
fix.

[Knut Anders Hatlen]

All the regression tests ran cleanly. Reviews would be appreciated. Thanks.

[Kathey Marsden]

Thanks Knut for fixing this issue. I was really surprised that we had never tried inserting a blob of length 32766. Just for good measure, with your patch, I made sure we could insert byte[]'s from 0 to 40000 and select them back again individually to hopefully make sure we don't have any more gaps and it all passed.

I wonder if the client's Reply.java has a similar problem with its compressBLayerData and how we might trigger that. The javadoc says it shouldn't be used but we do cover it in code coverage.

[Urban Widmark]

Works for me after applying Knut's patch.

[Knut Anders Hatlen]

Thanks for testing the patch, Urban and Kathey.
Committed revision 752813.

[Knut Anders Hatlen]

> I wonder if the client's Reply.java has a similar problem with its
> compressBLayerData and how we might trigger that. The javadoc says
> it shouldn't be used but we do cover it in code coverage.

It looks like it has the same problem. I've made that method throw an
exception when it's called and started suites.All so that I can see
which tests are triggering it.

[Knut Anders Hatlen]

Two of the test cases (testBigTable and testLargeReplies) in derbynet/PrepareStatementTest trigger the method in Reply.java. Looking more closely at the code in Reply.java, I don't think the problem is present on the client. When it calls compressBLayerData(), it does this:

            ensureALayerDataInBuffer(desiredDataSize + (continueDssHeaderCount * 2));
            compressBLayerData(continueDssHeaderCount);

Note that we add (continueDssHeaderCount*2) before calling ensureALayerDataInBuffer(). (continueDssHeaderCount*2) is equal to the overhead imposed by splitting the data, so it ensures that we have all the data we need before we enter compressBLayerData().

For consistency, it might be a good idea to do the same both places. I think that the solution in Reply.java is simpler than the solution I chose in DDMReader.java, so I think I'll back out those changes and use the other approach in DDMReader too.

[Knut Anders Hatlen]

The attached patch reverts the changes to DDMReader.compressBLayerData() made in revision 752813 and instead makes ensureBLayerDataInBuffer() use the same approach as Reply.java. All the regression tests, including the regression test for this issue, still pass.

[Knut Anders Hatlen]

Committed derby-4088-2.diff to trunk with revision 755866.

[Knut Anders Hatlen]

Committed derby-4088-2.diff to 10.5 with revision 755906.
Committed derby-4088.diff and derby-4088-2.diff to 10.4 with revision 755907.