PositionedStoreStream.reposition(pos) with pos greater than length leaves the stream object in an inconsistent state

• Key: DERBY-3781
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Minor
• Assignee: Kristian Waagan
• Reporter: Kristian Waagan
• Component/s: Store
• Affects Version/s: 10.3.3.0, 10.4.1.3, 10.5.1.1
• Fix Version/s: 10.4.2.0, 10.5.1.1

Description

PositionedStoreStream.reposition(pos) with pos greater than the stream length leaves the stream object in an inconsistent state, causing subsequent calls to fail or the state to remain inconsistent (which can cause the wrong data to be returned).

The problem is that the position variable gets out of sync with the underlying stream.
There are at least two ways to fix this (assuming the positioned store stream does not know the length of the underlying stream):
 a) Reset stream to position zero.
 b) Let the stream be positioned at EOF and update the internal position variable.

Option b) leaves the stream in an unusable state, and the next request will cause option a) to be performed. It also require a slight rewrite of 'PositionedStoreStream.skipFully' and 'PositionedStoreStream.reposition' to be able to determine the position of the stream (the length in this case).

Option a) will cause the first page of the stream to be read into the cache (if not already there), but taken the reason for doing this is an error condition it seems acceptable.

A correct value of the position variable is required for correct/valid operation of PositionedStoreStream.

Comments

[Kristian Waagan]

'derby-3781-1a-fix_and_test.diff' fixes the bug by resetting the stream after an unsuccessful reposition attempt.
Also includes a regression test.

I ran all tests without failures, but due to a last minute change in the patch I'm rerunning.
Patch ready for review.

[Kristian Waagan]

All tests ran successfully with patch 1a.

It should be noted that this bug was masked earlier due to the somewhat blunt algorithm causing DERBY-3766, which reset the stream and the position to zero on every repositioning request.

[Knut Anders Hatlen]

The fix looks correct to me.

It seems like part of the problem was that the convenience link "resettable" was used directly instead of calling this.resetStream(), so that the pos variable wasn't updated. Perhaps it would be less confusing and help prevent future bugs if we also removed the convenience link?

[Kristian Waagan]

Regarding the convenience link, using this.resetStream() instead of resettable.resetStream() wouldn't fix the problem. It would only cause another inconsistent state.

The problem is that the stream skipping and the position update isn't "atomic" in the case of requesting a position after EOF. The code simply wasn't properly written for handling this failure scenario.

The convenience link isn't required, and only used in a few places, so removing it sounds like an improvement. I'll make a patch after 1a has been committed.

[Kristian Waagan]

Committed 'derby-3781-1a-fix_and_test.diff' to trunk with revision 677172.

[Kristian Waagan]

'derby-3781-2a-remove_convenience_link.diff' removes the convenience link and replaces it with casts in three places. I also added a instanceof check in the constructor to detect where in the code an invalid stream might be passed in.
I think it is better to catch the error where it originates instead of waiting for a ClassCastException at a later point in time.

Patch ready for review.

[Knut Anders Hatlen]

2a looks good. Perhaps it would be better to use ASSERT instead of IllegalArgumentException? Then we don't need to worry about the risk of presenting non-localized error messages to the users, or about increasing the footprint of the production jars. The error won't be hidden in production code, it'll just happen another place, and I think it'll still be fairly easy to see what the problem is from the ClassCastException that will be presented.

Another thing I came to think about - is it possible to expose the original problem through JDBC? If it is, then it might be valuable to add a regression test that uses JDBC in addition to the one that accesses the PositionedStoredStream class directly.

[Kristian Waagan]

Attached 'derby-3781-2b-remove_convenience_link.diff', which removes the instanceof check.

Knut Anders wrote:
-----
Perhaps it would be better to use ASSERT instead of IllegalArgumentException? Then we don't need to worry about the risk of presenting non-localized error messages to the users, or about increasing the footprint of the production jars. The error won't be hidden in production code, it'll just happen another place, and I think it'll still be fairly easy to see what the problem is from the ClassCastException that will be presented.
-----
After analyzing the code, I've decided to remove the check. It is already checked elsewhere (EmbedBlob and EmbedClob) in sane builds, and it will fail with a CCE during initStream if an invalid stream is passed in from somewhere else in the future in the insane builds.


-----
Another thing I came to think about - is it possible to expose the original problem through JDBC?
-----
Yes, and it's already being done through BlobClob4BlobTest.testGetBytes. It should be easy to write a more specific test to make it more explicit.
Are you happy with the existing test, or should I write a specific regression test for the Jira?

[Knut Anders Hatlen]

Thanks for the new patch. +1 to commit.

BlobClob4BlobTest.testGetBytes is probably fine, but did it expose the original problem? I never saw it failing.

[Kristian Waagan]

The test didn't fail because the bug was masked by another piece of code (see my comment from 15/Jul/08 01:38 PM).
This code will be changed as part of DERBY-3766. It masked the bug by always resetting the stream, which brought the position variable and the underlying stream back into sync.

With my early patch for EmbedBlob.setPosition (not yet posted or committed), BlobClob4BlobTest.testGetBytes failed.

[Kristian Waagan]

Committed 'derby-3781-2b-remove_convenience_link.diff' to trunk with revision 677281.
Will backport both fixes tomorrow if my test runs are successful.

[Kristian Waagan]

Backported patches 1a and 2b to 10.4 with revision 677520 and to 10.3 with revision 677521.