THRIFT-5006 Implement DEFAULT_MAX_LENGTH at TFramedTransport

Client: Delphi
Patch: Jens Geyer

4 files changed, 54 insertions, 44 deletions

diff --git a/CHANGES.md b/CHANGES.md
index 8fffab6b3..2f128684e 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -6,7 +6,7 @@
 
 - [THRIFT-4990](https://issues.apache.org/jira/browse/THRIFT-4990) - Upgrade to .NET Core 3.0
 - [THRIFT-4981](https://issues.apache.org/jira/browse/THRIFT-4981) - Remove deprecated netcore bindings from the code base
-
+- [THRIFT-5006](https://issues.apache.org/jira/browse/THRIFT-5006) - Implement DEFAULT_MAX_LENGTH at TFramedTransport
 
 ## 0.13.0
 
diff --git a/lib/delphi/src/Thrift.Server.pas b/lib/delphi/src/Thrift.Server.pas
index c7cce7375..c9365c6db 100644
--- a/lib/delphi/src/Thrift.Server.pas
+++ b/lib/delphi/src/Thrift.Server.pas
@@ -61,7 +61,7 @@ type
   public
     type
       TLogDelegate = reference to procedure( const str: string);
-  strict protected
+  protected
     FProcessor : IProcessor;
     FServerTransport : IServerTransport;
     FInputTransportFactory : ITransportFactory;
@@ -116,7 +116,7 @@ type
 
 
   TSimpleServer = class( TServerImpl)
-  strict private
+  private
     FStop : Boolean;
   public
     constructor Create(
diff --git a/lib/delphi/src/Thrift.Transport.pas b/lib/delphi/src/Thrift.Transport.pas
index b6e3c99f6..3067bcd76 100644
--- a/lib/delphi/src/Thrift.Transport.pas
+++ b/lib/delphi/src/Thrift.Transport.pas
@@ -91,7 +91,8 @@ type
         TimedOut,
         EndOfFile,
         BadArgs,
-        Interrupted
+        Interrupted,
+        CorruptedData
       );
   strict protected
     constructor HiddenCreate(const Msg: string);
@@ -144,6 +145,11 @@ type
     class function GetType: TTransportException.TExceptionType;  override;
   end;
 
+  TTransportExceptionCorruptedData = class (TTransportExceptionSpecialized)
+  protected
+    class function GetType: TTransportException.TExceptionType;  override;
+  end;
+
   TSecureProtocol = (
     SSL_2, SSL_3, TLS_1,   // outdated, for compatibilty only
     TLS_1_1, TLS_1_2       // secure (as of today)
@@ -368,15 +374,17 @@ type
   end;
 
   TFramedTransportImpl = class( TTransportImpl)
-  strict private const
-    FHeaderSize : Integer = 4;
-  strict private class var
-    FHeader_Dummy : array of Byte;
+  strict protected const
+    DEFAULT_MAX_LENGTH = 16384000;      // this value is used by all Thrift libraries
+  strict protected type
+    TFramedHeader = Int32;
   strict protected
     FTransport : ITransport;
     FWriteBuffer : TMemoryStream;
     FReadBuffer : TMemoryStream;
+    FMaxFrameSize : Integer;
 
+    procedure InitMaxFrameSize;
     procedure InitWriteBuffer;
     procedure ReadFrame;
   public
@@ -386,10 +394,6 @@ type
         function GetTransport( const ATrans: ITransport): ITransport; override;
       end;
 
-    {$IFDEF HAVE_CLASS_CTOR}
-    class constructor Create;
-    {$ENDIF}
-
     constructor Create; overload;
     constructor Create( const ATrans: ITransport); overload;
     destructor Destroy; override;
@@ -403,9 +407,6 @@ type
     procedure Flush; override;
   end;
 
-{$IFNDEF HAVE_CLASS_CTOR}
-procedure TFramedTransportImpl_Initialize;
-{$ENDIF}
 
 const
   DEFAULT_THRIFT_TIMEOUT = 5 * 1000; // ms
@@ -549,6 +550,11 @@ begin
   result := TExceptionType.Interrupted;
 end;
 
+class function TTransportExceptionCorruptedData.GetType: TTransportException.TExceptionType;
+begin
+  result := TExceptionType.CorruptedData;
+end;
+
 { TTransportFactoryImpl }
 
 function TTransportFactoryImpl.GetTransport( const ATrans: ITransport): ITransport;
@@ -993,7 +999,7 @@ end;
 
 procedure TStreamTransportImpl.Open;
 begin
-
+  // nothing to do
 end;
 
 function TStreamTransportImpl.Read( const pBuf : Pointer; const buflen : Integer; off: Integer; len: Integer): Integer;
@@ -1087,35 +1093,19 @@ end;
 
 { TFramedTransportImpl }
 
-{$IFDEF HAVE_CLASS_CTOR}
-class constructor TFramedTransportImpl.Create;
-begin
-  SetLength( FHeader_Dummy, FHeaderSize);
-  FillChar( FHeader_Dummy[0], Length( FHeader_Dummy) * SizeOf( Byte ), 0);
-end;
-{$ELSE}
-procedure TFramedTransportImpl_Initialize;
-begin
-  SetLength( TFramedTransportImpl.FHeader_Dummy, TFramedTransportImpl.FHeaderSize);
-  FillChar( TFramedTransportImpl.FHeader_Dummy[0],
-    Length( TFramedTransportImpl.FHeader_Dummy) * SizeOf( Byte ), 0);
-end;
-{$ENDIF}
-
 constructor TFramedTransportImpl.Create;
 begin
   inherited Create;
-  InitWriteBuffer;
-end;
 
-procedure TFramedTransportImpl.Close;
-begin
-  FTransport.Close;
+  InitMaxFrameSize;
+  InitWriteBuffer;
 end;
 
 constructor TFramedTransportImpl.Create( const ATrans: ITransport);
 begin
   inherited Create;
+
+  InitMaxFrameSize;
   InitWriteBuffer;
   FTransport := ATrans;
 end;
@@ -1127,12 +1117,21 @@ begin
   inherited;
 end;
 
+procedure TFramedTransportImpl.InitMaxFrameSize;
+begin
+  FMaxFrameSize := DEFAULT_MAX_LENGTH;
+end;
+
+procedure TFramedTransportImpl.Close;
+begin
+  FTransport.Close;
+end;
+
 procedure TFramedTransportImpl.Flush;
 var
   buf : TBytes;
   len : Integer;
   data_len : Integer;
-
 begin
   len := FWriteBuffer.Size;
   SetLength( buf, len);
@@ -1140,7 +1139,7 @@ begin
     System.Move( FWriteBuffer.Memory^, buf[0], len );
   end;
 
-  data_len := len - FHeaderSize;
+  data_len := len - SizeOf(TFramedHeader);
   if (data_len < 0) then begin
     raise TTransportExceptionUnknown.Create('TFramedTransport.Flush: data_len < 0' );
   end;
@@ -1166,11 +1165,12 @@ type
   end;
 
 procedure TFramedTransportImpl.InitWriteBuffer;
+const DUMMY_HEADER : TFramedHeader = 0;
 begin
   FWriteBuffer.Free;
   FWriteBuffer := TMemoryStream.Create;
   TAccessMemoryStream(FWriteBuffer).Capacity := 1024;
-  FWriteBuffer.Write( Pointer(@FHeader_Dummy[0])^, FHeaderSize);
+  FWriteBuffer.Write( DUMMY_HEADER, SizeOf(DUMMY_HEADER));
 end;
 
 procedure TFramedTransportImpl.Open;
@@ -1202,17 +1202,27 @@ end;
 
 procedure TFramedTransportImpl.ReadFrame;
 var
-  i32rd : TBytes;
+  i32rd : packed array[0..SizeOf(TFramedHeader)-1] of Byte;
   size : Integer;
   buff : TBytes;
 begin
-  SetLength( i32rd, FHeaderSize );
-  FTransport.ReadAll( i32rd, 0, FHeaderSize);
+  FTransport.ReadAll( @i32rd[0], SizeOf(i32rd), 0, SizeOf(i32rd));
   size :=
     ((i32rd[0] and $FF) shl 24) or
     ((i32rd[1] and $FF) shl 16) or
     ((i32rd[2] and $FF) shl 8) or
      (i32rd[3] and $FF);
+
+  if size < 0 then begin
+    Close();
+    raise TTransportExceptionCorruptedData.Create('Read a negative frame size ('+IntToStr(size)+')');
+  end;
+
+  if size > FMaxFrameSize then begin
+    Close();
+    raise TTransportExceptionCorruptedData.Create('Frame size ('+IntToStr(size)+') larger than allowed maximum ('+IntToStr(FMaxFrameSize)+')');
+  end;
+
   SetLength( buff, size );
   FTransport.ReadAll( buff, 0, size );
   FReadBuffer.Free;
diff --git a/lib/delphi/test/TestClient.pas b/lib/delphi/test/TestClient.pas
index e59c32720..a488cacae 100644
--- a/lib/delphi/test/TestClient.pas
+++ b/lib/delphi/test/TestClient.pas
@@ -93,7 +93,7 @@ type
       Normal,          // Fairly small array of usual size (256 bytes)
       ByteArrayTest,   // THRIFT-4454 Large writes/reads may cause range check errors in debug mode
       PipeWriteLimit,  // THRIFT-4372 Pipe write operations across a network are limited to 65,535 bytes per write.
-      TwentyMB         // that's quite a bit of data
+      FifteenMB        // quite a bit of data, but still below the default max frame size
     );
 
   private
@@ -1024,7 +1024,7 @@ begin
     Normal         : SetLength( result, $100);
     ByteArrayTest  : SetLength( result, SizeOf(TByteArray) + 128);
     PipeWriteLimit : SetLength( result, 65535 + 128);
-    TwentyMB       : SetLength( result, 20 * 1024 * 1024);
+    FifteenMB      : SetLength( result, 15 * 1024 * 1024);
   else
     raise EArgumentException.Create('aSize');
   end;