LZMACompressor: Fix race in NoMoreInput check.

Previously, it checked if the input buffer was empty and then if NoMoreInput=True. That's backwards; the input buffer could become non-empty in between. If that happened, compression could end with that remaining input unprocessed, leading to a "Setup files are corrupted" error. It's likely nobody ever ran into this, though. I could only reproduce it by adding a Sleep between the input buffer and NoMoreInput checks, and a Sleep at the end of StartEncode.
jrsoftware · Nov 9, 2024 · f41bfe1 · f41bfe1
1 parent a289db2
commit f41bfe1
Show file tree

Hide file tree

Showing 2 changed files with 29 additions and 6 deletions.
diff --git a/Projects/Src/Compression.LZMACompressor.pas b/Projects/Src/Compression.LZMACompressor.pas
@@ -559,8 +559,17 @@ function TLZMAWorkerThread.FillBuffer(const AWrite: Boolean;
       LimitedSize := Size;
     if AWrite then
       Bytes := RingBufferWrite(FShared.OutputBuffer, P^, LimitedSize)
-    else
+    else begin
+      if FShared.NoMoreInput then begin
+        { If NoMoreInput=True and *then* we see that the input buffer is
+          empty (ordering matters!), we know that all input has been
+          processed and that the input buffer will stay empty }
+        MemoryBarrier;
+        if FShared.InputBuffer.Count = 0 then
+          Break;
+      end;
       Bytes := RingBufferRead(FShared.InputBuffer, P^, LimitedSize);
+    end;
     if Bytes = 0 then begin
       if AWrite then begin
         { Output buffer full; wait for the main thread to flush it }
@@ -571,8 +580,6 @@ function TLZMAWorkerThread.FillBuffer(const AWrite: Boolean;
       end
       else begin
         { Input buffer empty; wait for the main thread to fill it }
-        if FShared.NoMoreInput then
-          Break;
         Result := WakeMainAndWaitUntil(FEvents.WorkerWaitingOnInputEvent,
           FEvents.EndWaitOnInputEvent);
         if Result <> S_OK then
@@ -1072,6 +1079,11 @@ procedure TLZMACompressor.DoFinish;
 begin
   StartEncode;
 
+  { Ensure prior InputBuffer updates are made visible before setting
+    NoMoreInput. (This isn't actually needed right now because there's
+    already a full barrier inside RingBufferWrite. But that's an
+    implementation detail.) }
+  MemoryBarrier;
   FShared.NoMoreInput := True;
   while not FEncodeFinished do begin
     SatisfyWorkerWaitOnInput;
@@ -1094,6 +1106,11 @@ procedure TLZMACompressor.DoFinish;
       [FShared.EncodeResult]);
   end;
 
+  { Encoding was successful; verify that all input was consumed }
+  if FShared.InputBuffer.Count <> 0 then
+    LZMAInternalErrorFmt('Finish: Input buffer is not empty (%d)',
+      [FShared.InputBuffer.Count]);
+
   FEncodeStarted := False;
 end;
 

diff --git a/Projects/Src/Compression.LZMACompressor/islzma/islzma_exe.c b/Projects/Src/Compression.LZMACompressor/islzma/islzma_exe.c
@@ -169,6 +169,15 @@ static HRESULT FillBuffer(const BOOL AWrite, void *Data, size_t Size,
 		if (AWrite) {
 			Bytes = RingBufferWrite(&FShared->OutputBuffer, P, LimitedSize);
 		} else {
+			if (FShared->NoMoreInput) {
+				/* If NoMoreInput=True and *then* we see that the input buffer is
+				   empty (ordering matters!), we know that all input has been
+				   processed and that the input buffer will stay empty */
+				MemoryBarrier();
+				if (FShared->InputBuffer.Count == 0) {
+					break;
+				}
+			}
 			Bytes = RingBufferRead(&FShared->InputBuffer, P, LimitedSize);
 		}
 		if (Bytes == 0) {
@@ -182,9 +191,6 @@ static HRESULT FillBuffer(const BOOL AWrite, void *Data, size_t Size,
 				}
 			} else {
 				/* Input buffer empty; wait for the main thread to fill it */
-				if (FShared->NoMoreInput) {
-					break;
-				}
 				Result = WakeMainAndWaitUntil(
 					THandle32ToHandle(FEvents->WorkerWaitingOnInputEvent),
 					THandle32ToHandle(FEvents->EndWaitOnInputEvent));