Stack overflow in TList<T> with for xyz in... syntax

[Steve Schafer]

The following code results in a stack overflow (the workaround is to iterate through the Items the old-fashioned way):

uses Spring.Collections, Spring.Collections.Lists;

type IFoo = interface(IInterface) ['{A765FA2A-B4B6-4D69-9772-DF090F319955}'] end;

TFoo = class(TInterfacedObject, IFoo) end;

TFooList = class(TList<IFoo>, IEnumerable<IFoo>) protected procedure Clear; override; end;

procedure TFooList.Clear; var foo: IFoo; begin for foo in Self do { nothing }; inherited Clear; end;

procedure WillExplodeIfCalled; var e: IEnumerable<IFoo>; begin e := TFooList.create([TFoo.create as IFoo]); end;

-Steve

[Arioch The]

simplified example - the type of elements does not matter.

program s4d_53;
{$APPTYPE CONSOLE}

uses
  System.SysUtils, Spring.Collections, Spring.Collections.Lists;

type
   TFooList = class(TList<Byte>, IEnumerable<Byte>)
      protected procedure Clear; override;
   end;

procedure TFooList.Clear;
var foo: Byte;
begin
  for foo in Self
     do { nothing };
  inherited Clear;
end;

procedure WillExplodeIfCalled;
var e: IEnumerable<Byte>;
begin
  e := TFooList.create([ 0 ]);
end;

begin
  try
    { TODO -oUser -cConsole Main : Insert code here }
    WillExplodeIfCalled;
  except
    on E: Exception do
      Writeln(E.ClassName, ': ', E.Message);
  end;
end.

The issue stems from two reasons:

1) TCollectionBase destructor calls an externally overridable procedure Clear. It better be split into two

type TCollectionBase =
  //...

strict private procedure InternalClear; static;
begin
  // is knowingly called from the destructing sequence, 
  // is guaranteed to do nothing dangerous or unpredictable over half-dead collection
  // can not be modified/hooked from outside by non-expecting library user
end;

protected procedure Clear, virtual;
begin
   InternalClear;
   // can be overrrode, but is never called during destructing sequence
end; 

2) TCollectionBase.TEnumerator does mess with refcounting.

• TCollectionBase.BeforeDestruction should set the flag "IsDestructing"
• TCollectionBase._AddRef should throw exception, if someone tries to acquire a refcounted link to the object being destroyed - there is no way to prevent destruction, which _AddRef should do then.
• TCollectionBase.GetEnumerator should copy the TCollectionBase.IsDestructing into TCollectionBase.TEnumerator.CollectionIsDestructing flag
• TCollectionBase.TEnumerator should check TCollectionBase.TEnumerator.CollectionIsDestructing in its constructor and destructor, and never call _AddRef and _Release over half-dead collections

[Stefan Glienke]

Thank you both for your report.

That bug is related to changing the RefCount during destruction and exists since introduction of TInterfacedObject.

type
  TTest = class(TInterfacedObject)
  public
    destructor Destroy; override;
  end;

destructor TTest.Destroy;
var
  intf: IInterface;
begin
  intf := Self;
  inherited;
end;

procedure WillCauseRecursion;
var
  intf: IInterface;
begin
  intf := TTest.Create;
end;

It has been fixed in XE7 and does not affect the code in the library as is and can easily avoided by not using a for in loop in your Clear override. For this reason we won't provide some fix for that bug.

If you want to do something to your items when they get removed you can also override the Changed method.

[Arioch The]

Does S4D comes with a documentation which Pascal syntax elements is prohibited when using S4D ? Is it a good design to prohibit using of Pascal syntactic constructs in general-use mthods of the library?

.Clear is not a special method - it is a common-use method.

Delphi does not fix somethign as it does not have this bug at all, at least not XE2

rogram s4d_53;
{$APPTYPE CONSOLE}

uses
  System.SysUtils,
  Spring.Collections, Spring.Collections.Lists,
  System.Generics.Collections;


type
   TS4DList = class( Spring.Collections.Lists.TList<Byte>,
                     Spring.Collections.IEnumerable<Byte>)
      protected procedure Clear; override;
   end;

procedure TS4DList.Clear;
var foo: Byte;
begin
  for foo in Self
     do { nothing };
  inherited Clear;
end;

procedure WillExplodeIfCalled_S4D;
var e: Spring.Collections.IEnumerable<Byte>;
begin
  e := TS4DList.create([ 0 ]);
end;

type
  TDelphiList = class( System.Generics.Collections.TList<Byte>)
     public procedure Clear; reintroduce;
     public procedure BeforeDestruction; override;
     public destructor Destroy; override;
  end;

procedure TDelphiList.Clear;
var foo: Byte;
begin
  for foo in Self
     do { nothing };
  inherited Clear;
end;

procedure TDelphiList.BeforeDestruction;
var foo: Byte;
begin
  for foo in Self
     do { nothing };
  inherited;
end;

destructor TDelphiList.Destroy;
var foo: Byte;
begin
  for foo in Self
     do { nothing };
  inherited;
end;

procedure WillNotExplodeIfCalled_DXE2;
begin
  with TDelphiList.Create do try
    Add( 0 );
  finally
    Destroy
  end;
end;

begin
  try
    WillNotExplodeIfCalled_DXE2;
    WillExplodeIfCalled_S4D;
  except
    on E: Exception do
      Writeln(E.ClassName, ': ', E.Message);
  end;
end.

[Arioch The]

If you want to do something to your items when they get removed you can also override the Changed method.

Stefan, Clear is not some implementatio ndetail, it is a public use method! In a sense it is very similar to events handler. If you destructing sequence calls user-provided events without specifying that the object is no more valid, without setting prohibition on Pascal syntax use, then it is the choice of library designer, not the choice of delphi designer.

I browse S4D sources and i see strings like

{$IFNDEF DELPHIXE_UP}
function SplitString(const s: string; delimiter: Char): TStringDynArray;


{$IFNDEF DELPHIXE2_UP}
function ReturnAddress: Pointer;
{$ENDIF}

  TRegistration = record
  private
{$IFNDEF DELPHIXE_UP}
    fRegistry: TComponentRegistry;
{$ELSE}
    fRegistry: IComponentRegistry;
{$ENDIF}


  // See SetEnvironmentVariable for comment about encoding
  // We cannot defer to SysUtils implementation since it doesn't support regional characters as well (QC123698)
{$IFDEF DELPHIXE2}
  Result := string(AnsiString(PAnsiChar(resultPointer)));
{$ELSE DELPHIXE2}
  Result := UTF8ToString(resultPointer);
{$ENDIF DELPHIXE2}

You can see, the lack of some features in pre-XE7 Delphi was never considered an excuse to keep bugs in the S4D library before...

I understand for example dropping D2009 - it is so crippled that is practicalyl unusable. But to not even try to fix the bug, especially when the fix seems to be obvious - ot os another story.

[Arioch The]

Regarding

destructor TTest.Destroy;
var
  intf: IInterface;
begin
  intf := Self;
  inherited;
end;

I really just cannot see how one can compare the destructing sequence of semi-invalid object, which is clearly taking place within the destructor and BeofreDestruction methods, and a general lfietime of the object and a general for-users-extension method like Clear. Those things are so radically different....

Also the user in this example does not do interface pointer acquiring, it only uses generic Pascal syntax of for-in loop. The unexpected fact that the library prohibit using the for-in loop within general-use methods without explicitly documentin it i cannot see as a library user fault.

If the Clear method is so fragile that it can blow up the library by using a general Pascal syntax, then it is not to be extendable by user. BTW, it is not in standard TList<T> in XE2, though it would not blow things anyway.

[Stefan Glienke]

fixed issue #53

→ <<cset 766a8b89424b>>