View Issue Details

IDProjectCategoryView StatusLast Update
0035027FPCRTLpublic2019-07-19 09:00
ReporterMartin FriebeAssigned ToSven Barth 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Platform64bit IntelOSwin 10OS Version10
Product Version3.2.0Product Build 
Target VersionFixed in Version3.3.1 
Summary0035027: TThread.Synchronize - undefined behaviour / crash
DescriptionFrom a review of the source, there may be issue:

I found and tested the issue with 3.0.4 on win32. But a review of 3.2 branch shows that the problem is still there.


TThread.Synchronize
will call
  ThreadQueueAppend(AThread.FSynchronizeEntry);

AThread.FSynchronizeEntry is the re-usable entry
(which is fine, only one synchronize call can be active in any given thread. (Unless you are in a synchronize event, and call MyWaitingThread.synchronize from inside the event....))

ThreadQueueAppend puts the FSynchronizeEntry at the end of the queue.
It does NOT clear FSynchronizeEntry^.Next, it expects it to be nil already. => TThread.Synchronize will reset Next to nil *after* ThreadQueueAppend returns

ThreadQueueAppend waits for the synchronize call to finish.

But, if the sync event threw an exception, this exception is returned to the thread.
ThreadQueueAppend checks if an exception was returned and raises it.

In case of such an exception TThread.Synchronize will never get to execute
  AThread.FSynchronizeEntry^.Next := Nil;

The next time that thread calls TThread.Synchronize, the Next pointer is dirty.

--------------------
The code in "steps to reproduce" demonstrates this (cheaply using "sleep" to ensure threads are executed in desired order....)

MySync2 Is called only once, yet the output is
x
x2
x
x2


If Thread2 had been free'd, and the memory for its FSynchronizeEntry had been reused with other data, then this would likely cause a crash.
Steps To Reproduceprogram Project1;
{$mode objfpc}{$H+}
uses
  {$IFDEF UNIX}{$IFDEF UseCThreads} cthreads, {$ENDIF}{$ENDIF}
  Classes, sysutils;

type
  MT1= class(TThread)
    procedure Execute; override;
  private
    procedure MySync;
  end;

  { MT2 }

  MT2= class(TThread)
    procedure Execute; override;
  private
    procedure MySync2;
  end;
var
  T1: MT1;
  T2: MT2;

{ MT2 }

procedure MT2.Execute;
begin
  try
    Synchronize(@MySync2);
  except end;
  while true do sleep(100*1000);
end;

procedure MT2.MySync2;
begin
  writeln('x2 ');
  raise Exception.Create('Foo'); // prevent event^.Method from being set to nil
end;

procedure MT1.Execute;
begin
  try
    Synchronize(@MySync);
  except end;
  Sleep(10*1000);
  try
    Synchronize(@MySync);
  except end;
  while true do sleep(100*1000);
end;

procedure MT1.MySync;
begin
  writeln('x');
  raise Exception.Create('Foo'); // prevent event^.Next from being set to nil
end;

begin
  T1 := MT1.Create(False);
  Sleep(2*1000);
  T2 := MT2.Create(False);
  Sleep(2*1000);
  CheckSynchronize(30*1000);
  CheckSynchronize(30*1000);
  CheckSynchronize(30*1000);
  CheckSynchronize(30*1000);
  readln;
end.


TagsNo tags attached.
Fixed in Revision41281
FPCOldBugId0
FPCTarget
Attached Files

Activities

Sven Barth

2019-02-10 16:44

manager   ~0114012

Please test and close if okay.

Issue History

Date Modified Username Field Change
2019-02-06 22:25 Martin Friebe New Issue
2019-02-06 22:26 Martin Friebe Summary TThread.Synchronize - undifined behaviour / crash => TThread.Synchronize - undefined behaviour / crash
2019-02-10 16:44 Sven Barth Fixed in Revision => 41281
2019-02-10 16:44 Sven Barth Note Added: 0114012
2019-02-10 16:44 Sven Barth Status new => resolved
2019-02-10 16:44 Sven Barth Fixed in Version => 3.3.1
2019-02-10 16:44 Sven Barth Resolution open => fixed
2019-02-10 16:44 Sven Barth Assigned To => Sven Barth
2019-02-10 21:44 Martin Friebe Status resolved => closed