View Issue Details

IDProjectCategoryView StatusLast Update
0036758FPCPackagespublic2020-03-07 19:07
ReporterMarcin WiazowskiAssigned ToMichael Van Canneyt 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Product Version3.3.1Product Build44263 
Target VersionFixed in Version3.3.1 
Summary0036758: Small improvement for Tcompressionstream
DescriptionWhen writing compressed data to a stream, the Tcompressionstream implementation uses either stream's Write or WriteBuffer method.

Using the WriteBuffer method is a good way of writing data - in case of writing error, an exception is raised.

On the contrary - in case of writing error - the Write method just returns smaller number of written bytes as a result. The problem is that silent data loss will occur in such case:



Function TCompressionstream.ClearOutBuffer : Integer;
begin
  { Flush the buffer to the stream and update progress }
  Result:=source.write(Fbuffer^,bufsize); <====== returned Result may be less than bufsize - and no exception is raised
  inc(compressed_written,Result);

  { reset output buffer } <====== data buffer is cleared, although not fully written to the output stream
  Fstream.next_out:=Fbuffer;
  Fstream.avail_out:=bufsize;
end;



To avoid silent data loss, the attached patch converts Write call to a WriteBuffer call.

In addition, due to requesting "bufsize-Fstream.avail_out" bytes to be written, instead of just "bufsize", the modified ClearOutBuffer implementation may be also used to replace a bunch of code in a Tcompressionstream.flush implementation.

Since both these changes affect the same code, both of them are contained in the attached patch.
TagsNo tags attached.
Fixed in Revision44282
FPCOldBugId
FPCTarget4.0.0
Attached Files
  • patch.diff (1,330 bytes)
    Index: zstream.pp
    ===================================================================
    --- zstream.pp	(revision 44263)
    +++ zstream.pp	(working copy)
    @@ -56,7 +56,7 @@
     
             Tcompressionstream=class(Tcustomzlibstream)
             private
    -          function ClearOutBuffer: Integer;
    +          procedure ClearOutBuffer;
             protected
               raw_written,compressed_written: int64;
             public
    @@ -206,13 +206,12 @@
       get_compressionrate:=100*compressed_written/raw_written;
     end;
     
    -Function TCompressionstream.ClearOutBuffer : Integer;
    +procedure TCompressionstream.ClearOutBuffer;
     
    -
     begin
       { Flush the buffer to the stream and update progress }
    -  Result:=source.write(Fbuffer^,bufsize);
    -  inc(compressed_written,Result);
    +  source.writebuffer(Fbuffer^,bufsize-Fstream.avail_out);
    +  inc(compressed_written,bufsize-Fstream.avail_out);
       progress(self);
       { reset output buffer }
       Fstream.next_out:=Fbuffer;
    @@ -235,13 +234,7 @@
           raise Ecompressionerror.create(zerror(err));
       until false;
       if Fstream.avail_out<bufsize then
    -    begin
    -      source.writebuffer(FBuffer^,bufsize-Fstream.avail_out);
    -      inc(compressed_written,bufsize-Fstream.avail_out);
    -      progress(self);
    -      Fstream.next_out:=Fbuffer;
    -      Fstream.avail_out:=bufsize;
    -    end;
    +    ClearOutBuffer;
     end;
     
     
    
    patch.diff (1,330 bytes)

Activities

Marcin Wiazowski

2020-03-04 23:49

reporter  

patch.diff (1,330 bytes)
Index: zstream.pp
===================================================================
--- zstream.pp	(revision 44263)
+++ zstream.pp	(working copy)
@@ -56,7 +56,7 @@
 
         Tcompressionstream=class(Tcustomzlibstream)
         private
-          function ClearOutBuffer: Integer;
+          procedure ClearOutBuffer;
         protected
           raw_written,compressed_written: int64;
         public
@@ -206,13 +206,12 @@
   get_compressionrate:=100*compressed_written/raw_written;
 end;
 
-Function TCompressionstream.ClearOutBuffer : Integer;
+procedure TCompressionstream.ClearOutBuffer;
 
-
 begin
   { Flush the buffer to the stream and update progress }
-  Result:=source.write(Fbuffer^,bufsize);
-  inc(compressed_written,Result);
+  source.writebuffer(Fbuffer^,bufsize-Fstream.avail_out);
+  inc(compressed_written,bufsize-Fstream.avail_out);
   progress(self);
   { reset output buffer }
   Fstream.next_out:=Fbuffer;
@@ -235,13 +234,7 @@
       raise Ecompressionerror.create(zerror(err));
   until false;
   if Fstream.avail_out<bufsize then
-    begin
-      source.writebuffer(FBuffer^,bufsize-Fstream.avail_out);
-      inc(compressed_written,bufsize-Fstream.avail_out);
-      progress(self);
-      Fstream.next_out:=Fbuffer;
-      Fstream.avail_out:=bufsize;
-    end;
+    ClearOutBuffer;
 end;
 
 
patch.diff (1,330 bytes)

Michael Van Canneyt

2020-03-07 16:01

administrator   ~0121434

Checked and applied, thank you very much !

Marcin Wiazowski

2020-03-07 19:07

reporter   ~0121440

Thanks!

Issue History

Date Modified Username Field Change
2020-03-04 23:49 Marcin Wiazowski New Issue
2020-03-04 23:49 Marcin Wiazowski File Added: patch.diff
2020-03-07 16:01 Michael Van Canneyt Assigned To => Michael Van Canneyt
2020-03-07 16:01 Michael Van Canneyt Status new => resolved
2020-03-07 16:01 Michael Van Canneyt Resolution open => fixed
2020-03-07 16:01 Michael Van Canneyt Fixed in Version => 3.3.1
2020-03-07 16:01 Michael Van Canneyt Fixed in Revision => 44282
2020-03-07 16:01 Michael Van Canneyt FPCTarget => 4.0.0
2020-03-07 16:01 Michael Van Canneyt Note Added: 0121434
2020-03-07 19:07 Marcin Wiazowski Status resolved => closed
2020-03-07 19:07 Marcin Wiazowski Note Added: 0121440