Thursday, October 10, 2013

TStreamAdapter.Seek Broken For Large Files

EDIT I was completely wrong in my analysis and this bug was indeed fixed in XE4. I'm leaving the original (slightly edited) post below - firstly to remind myself to thoroughly test everything before posting and secondly to help some poor programmer who will run into the same problem with Delphi XE, XE2, XE3 ... in the future.


Yesterday a colleague informed me that he added a workaround for a VCL bug (which we confirmed to exist in XE2, XE4 and XE5) to our codebase and asked me if I could “inform the authorities”, i.e. enter it into QualityCentral. Imagine my surprise when I not only found the bug already entered but marked as fixed!

The problematic code is posted below, problem marked with a yellow color as it is hard to spot.

function TStreamAdapter.Seek(dlibMove: Largeint; dwOrigin: Longint;
out libNewPosition: Largeint): HResult;
var
NewPos: LargeInt;
begin
try
if (dwOrigin < STREAM_SEEK_SET) or (dwOrigin > STREAM_SEEK_END) then
begin
Result := STG_E_INVALIDFUNCTION;
Exit;
end;
NewPos := FStream.Seek(dlibMove, dwOrigin);
if @libNewPosition <> nil then libNewPosition := NewPos;
Result := S_OK;
except
Result := STG_E_INVALIDPOINTER;
end;
end;

TStream implements two Seek methods (below). The former is here for backwards compatibility and allows only seeking in 32-bit-addressable (i.e. smaller than 4 GB) files. The latter supports files larger than 4 GB.

function Seek(Offset: Longint; Origin: Word): Longint; overload; virtual;
function Seek(const Offset: Int64; Origin: TSeekOrigin): Int64; overload; virtual
;

If you Ctrl+Click on ‘Seek’ in the TStreamAdapter.Seek implementation, IDE will jump to the latter Seek – which is incorrect as in reality the former (32-bit) seek is called. That makes the problem harder to spot.


Embarcadero people – can somebody please reopen the original bug report? This is something that surely needs to be fixed.


Our temporary (we’ll see for how many future Delphi versions) fix is the same as in the QC #105985 – make a copy of the TStreamAdapter.Seek and cast the dwOrigin parameter into TSeekOrigin enumeration so that the correct TStream.Seek method is called.

type
TStreamAdapter = class
(Classes.TStreamAdapter)
public
function Seek(dlibMove: Int64; dwOrigin: Integer; out libNewPosition: Int64): HRESULT; override; stdcall
;
end
;

function TStreamAdapter.Seek(dlibMove: Int64; dwOrigin: Integer; out libNewPosition: Int64):
HRESULT;
var
NewPos:
LargeInt;
begin
try
if (dwOrigin < STREAM_SEEK_SET) or (dwOrigin > STREAM_SEEK_END)
then
begin
Result :=
STG_E_INVALIDFUNCTION;
Exit;
end
;
NewPos := Stream.Seek(dlibMove,
TSeekOrigin(dwOrigin));
if @libNewPosition <> nil
then
libNewPosition :=
NewPos;
Result :=
S_OK;
except
Result :=
STG_E_INVALIDPOINTER;
end
;
end
;

7 comments:

  1. I remember asking in one of the very first Delphi betas to introduce an enumerated type like TSeekOrigin. That got denied, as it "was working correctly". Not so future proof...

    ReplyDelete
  2. Talk about great timing we were fighting this yesterday and was stepping through the same code.

    We were using XE

    ReplyDelete
  3. Sigh. Just looked in XE, and it's not pretty. Robert, did you fix it?

    ReplyDelete
  4. Well, to be reasonable, EMBT claimed they made fix for the bug, not that they did release that fix to public... :-D

    ReplyDelete
  5. Perhaps it would be easay enough to make in-memory runtime patch liek that old VCL FixPack series.

    At least for win32/xe2

    Can there test project be made, that would not require actualyl creating many-GB files on disk ?

    ReplyDelete
    Replies
    1. You can test with TMemoryStream as it is also inherited from TStreamAdapter to void working with huge files on your hard drive.

      Delete
    2. Published as a separate post: http://www.thedelphigeek.com/2013/10/tstreamadapterseek-test.html

      Delete