fcl-base: blowfish2: AV error in BlowFish2 base class destructor

Summary

A destructor for TBlowFish2Stream, which is a base stream for both encryption and decryption stream, contains an error causing a memory protection error - making it impossible to propertly dispose either of the streams.

System Information

• Operating system: Any
• Processor architecture: Any
• Compiler version: Free Pascal Compiler version 3.3.1-17647-gddd846ede4 [2025/03/16] for x86_64
• Device: Any

Steps to reproduce

Just instantiate and then try to free a TBlowFish2EncryptStream or TBlowFish2DeCryptStream instance.

Example Project

program BlowfishBug;

{$mode objfpc}{$H+}

uses
  Classes, SysUtils, BlowFish2;

function EncryptString(const AKey: UTF8String; const AString: String): TBytes;
var
  EncryptStream: TBlowFish2EncryptStream;
  DestStream: TBytesStream;
  EncryptedLength: Integer;
begin
  DestStream := TBytesStream.Create;
  DestStream.Size := 1024 * 8;

  EncryptStream := TBlowFish2EncryptStream.Create(AKey, DestStream);
  EncryptStream.WriteAnsiString(AString);

  { AV here - same for Destroy or FreeAndNil }
  EncryptStream.Free;

  Result := DestStream.Bytes;
  EncryptedLength := DestStream.Position;
  DestStream.Free;

  SetLength(Result, EncryptedLength);
end;

function DecryptString(const AKey: UTF8String; ABytes: TBytes): String;
var
  DecryptStream: TBlowFish2DeCryptStream;
  SourceStream: TBytesStream;
begin
  SourceStream := TBytesStream.Create(ABytes);
  DecryptStream := TBlowFish2DeCryptStream.Create(AKey, SourceStream);
  Result := DecryptStream.ReadAnsiString;

  { AV here - same for Destroy or FreeAndNil }
  DecryptStream.Free;

  SourceStream.Free;
end;

const
  Key = 'My Secret Key';

var
  InStr, OutStr: String;
  Encrypted: TBytes;

begin
  InStr := 'Free Pascal is a mature, versatile, open source Pascal compiler.';
  Writeln('In : ', InStr);

  Encrypted := EncryptString(Key, InStr);
  Writeln('Encrypted length: ', Length(Encrypted));

  OutStr := DecryptString(Key, Encrypted);
  Writeln('Out: ', OutStr);
end.

What is the current bug behavior?

Calling the stream destructor results in Access Violation.

What is the expected (correct) behavior?

The stream is disposed properly.

Possible fixes

Offending line here https://gitlab.com/freepascal.org/fpc/source/-/blob/ddd846ede494e86e85832d1bb3a1043c4196bea2/packages/fcl-base/src/blowfish2.pp#L1050

The destructor calls FreeAndNil for FBF property which is a mistake, because FBF is not a TObject descendant but a TBlowfish2Context record instance. Therefore FreeAndNil needs to be removed, then since it was the only operation in the destructor - the destructor itself is no longer needed.

Patch blowfish2.pp.patch

Merge request !974 (merged)