View Issue Details

IDProjectCategoryView StatusLast Update
0006035FPCCompilerpublic2018-10-04 16:11
ReporterMartin Schreiber Assigned ToJonas Maebe  
PrioritynormalSeveritymajorReproducibilityalways
Status closedResolutionno change required 
OSAll 
Summary0006035: AV by interface access
DescriptionAccessing a interface by typeconversion from untyped pointer object field results in AV.

Testresults:

Delphi 7:

*** global variable
testproc called
*** object field
testproc called

FPC win32:

*** global variable
testproc called
*** object field
addref called
testproc called
An unhandled exception occurred at $00000000 :
EAccessViolation : Access violation
  $00000000
  $00401233

Kylix:

*** global variable
testproc called
*** object field
testproc called

FPC linux:

*** global variable
testproc called
*** object field
addref called
testproc called
An unhandled exception occurred at $BFFFF21C :
EAccessViolation : Access violation
Additional InformationReporter: Martin Schreiber
EMail:
TagsNo tags attached.
Fixed in Revision13310
FPCOldBugId4086
FPCTarget
Attached Files

Relationships

related to 0009472 closedYuriy Sydorov "as" increase .RefCount in INTERFACE 
has duplicate 0016578 closedJonas Maebe Implicit create interface references 
related to 0014019 closedJonas Maebe Reference counting fails for result values 
related to 0030409 resolvedMichael Van Canneyt FPC delphi mode behaviour does not honour Delphi documented object lifetimes 

Activities

2005-06-14 12:00

 

code.pp (1,441 bytes)   
program project1;
{$ifdef FPC}
{$mode objfpc}{$H+}
{$else}
{$apptype console}
{$endif}

uses
 Classes,SysUtils;

type

 itest = interface
  procedure testproc;
 end;
 
 ttestclass1 = class(tobject,itest)
  public
   function queryinterface(const guid: tguid; out obj): hresult; stdcall;
   function _addref: integer; stdcall;
   function _release: integer; stdcall;
   procedure testproc;
 end;

 ttestclass2 = class
  public
   intf: pointer;
 end;
 
{ ttestclass1 }

function ttestclass1.queryinterface(const guid: tguid; out obj): hresult; stdcall;
begin
 result:= integer(e_nointerface);
end;

function ttestclass1._addref: integer; stdcall;
begin
 writeln('addref called');
 result:= -1;
end;

function ttestclass1._release: integer; stdcall;
begin
 writeln('release called');
 result:= -1;
end;

procedure ttestclass1.testproc;
begin
 writeln('testproc called');
end;

var
 po1: pointer;
 test1: ttestclass1;
 test2: ttestclass2;

begin
 test1:= ttestclass1.create;
 test2:= ttestclass2.create;
 writeln('*** global variable');
 po1:= pointer(itest(test1));
 itest(po1).testproc;
 writeln('*** object field');
 test2.intf:= pointer(itest(test1));
 itest(test2.intf).testproc;

 test1.free;
 test2.free;
end.

code.pp (1,441 bytes)   

2006-01-04 12:00

  ~0006862

Fixed

Jonas Maebe

2009-06-21 11:04

manager   ~0028656

Actually, I think that test program is wrong. It is the same problem as in 0009472 : your program assumes that no temporary interface variables are created by the compiler in the current scope and therefore crashes in case it does.

In the particular example you gave, Delphi did not create temps and FPC did. However, you can modify the program so it also crashes when compiled with Delphi:

program project1;
{$ifdef FPC}
{$mode objfpc}{$H+}
{$else}
{$apptype console}
{$endif}

uses
 Classes,SysUtils;

type

 itest = interface
  procedure testproc;
 end;
 
 ttestclass1 = class(tobject,itest)
  public
   procedure FreeInstance; override;
   function queryinterface(const guid: tguid; out obj): hresult; stdcall;
   function _addref: integer; stdcall;
   function _release: integer; stdcall;
   procedure testproc;
 end;

 ttestclass2 = class
  public
   intf: pointer;
   function getinf: itest;
 end;
 
{ ttestclass1 }

procedure ttestclass1.freeinstance;
begin
// FillChar(Pointer(Self)^, InstanceSize, 0);
  inherited FreeInstance;
end;

function ttestclass1.queryinterface(const guid: tguid; out obj): hresult; stdcall;
begin
 result:= integer(e_nointerface);
end;

function ttestclass1._addref: integer; stdcall;
begin
 writeln('addref called');
// result:= inherited _addref;
 result:= -1;
end;

function ttestclass1._release: integer; stdcall;
begin
 writeln('release called');
// result:= inherited _release;
 result:= -1;
end;

procedure ttestclass1.testproc;
begin
 writeln('testproc called');
end;

{ ttestclass2 }

function ttestclass2.getinf: itest;
begin
  result:=itest(intf);
end;


var
 po1, po2, po3: pointer;
 test1: ttestclass1;
 test2: ttestclass2;
 i: itest;
begin
  test1:= ttestclass1.create;
  test2:= ttestclass2.create;
  test2.intf:= pointer(itest(test1));

  i:=test2.getinf; // 1
  i.testproc;

  test1.free;
  test2.free;

  { make sure the memory freed by test1 and test2 is invalidated }
  getmem(po2,test2.instancesize);
  getmem(po1,test1.instancesize);
  fillchar(po1^,test1.instancesize,0);
  fillchar(po2^,test2.instancesize,0);
  freemem(po1);
  freemem(po2);

  { here you'll get a crash, because Delphi created a temp for
    the line "// 1" above and it will free it now
  }
end.


So I think the "fix" for this bug should be reverted, because it breaks other code which does not make any assumptions about implementation details (and which therefore should always work).

It would be possible to "fix" this issue in another way (which does not break 0014019), but then we are entering the realm of emulating the Delphi code generator in how it handles undefined behaviour, and that's
a) not FPC's goal
b) something which would suck up a lot of time, be very hard to get right, and with little payoff
c) something which may result in incompatibilities with future Delphi versions if they change such behaviour themselves

Martin Schreiber

2009-06-21 12:02

reporter   ~0028657

Last edited: 2009-06-21 12:04

I don't remember and understand the details, but AFAIK the construct
pointervariable:= pointer(isomeinterface(objectinstance));
is often used in Delphi in order to get an interface-pointer without _addref, mostly because of performance reasons for interfaces which return -1 for _addref/_release anyway.
It would be nice if we could do the same in FPC although in FPC we have corba-style interfaces for that purpose, i am not sure if getinterface() works now for corba-style interfaces?

Jonas Maebe

2009-06-21 12:14

manager   ~0028658

> I don't remember and understand the details, but AFAIK the construct
> pointervariable:= pointer(isomeinterface(objectinstance));
> is often used in Delphi in order to get an interface-pointer without _addref

That works and will also always work in FPC. The problem is this statement in your original test program:

   itest(test2.intf).testproc;

Delphi does not create a temporary interface reference to evaluate this expression, while FPC did. If the compiler uses a temp here, then this temp needs to be finalised at the end of the current scope to avoid memory leaks. Since you however explicitly free the class instance with which this temporary interface reference is associated before the end of the current scope, the program crashes (or at least can crash) when the compiler afterwards implicitly finalises the temp in the exit code.

In my program, I've added an expression for which Delphi also creates a temp (the "i:=test2.getinf"), and therefore the program also crashes when compiled with Delphi in that case.

I think that it is wrong to rely on whether or not a temporary interface is created by the compiler depending on the used expressions, because this is an implementation detail. The program is therefore buggy in my opinion, because by freeing the class instances and overriding the reference counting mechanism, the temporary interface references created by the compiler may be invalid when they are freed.

Jonas Maebe

2009-06-21 12:24

manager   ~0028660

> itest(test2.intf).testproc;
>
> Delphi does not create a temporary interface reference to evaluate
> this expression, while FPC did.

Well, FPC still does. But the "fix" for this bug was setting the type of that temp to "voidpointer", so the compiler did not try to increase its reference count when initialising it, nor finalise it. The result is that programs like in 0014019 now crash though.

Martin Schreiber

2009-06-21 13:49

reporter   ~0028667

> I think that it is wrong to rely on whether or not a temporary interface is
> created by the compiler depending on the used expressions, because this is
> an implementation detail.

Agreed, I mean to remember now, that even in Delphi RTL explicit freeing of -1 _addref/_release class instances did not crash if the freed memory did not change only what results in the conclusion, that interfaces for -1 _addref/_release classes which need explicit free (for example all TComponent descendents!!!) can't be used without extreme precaution and which renders reference counted non corba-style interfaces pretty useless for most of the components.
Hard to believe.

Jonas Maebe

2009-06-21 14:18

manager   ~0028669

Ok, thanks. I've reverted the "fix" for this issue, which fixes 0014019.

Issue History

Date Modified Username Field Change
2009-06-21 00:40 Jonas Maebe Relationship added related to 0014019
2009-06-21 00:41 Jonas Maebe Reporter FPC core team => Martin Schreiber
2009-06-21 00:41 Jonas Maebe Additional Information Updated
2009-06-21 00:43 Jonas Maebe Fixed in Revision => 2165
2009-06-21 11:04 Jonas Maebe Status closed => feedback
2009-06-21 11:04 Jonas Maebe Resolution fixed => reopened
2009-06-21 11:04 Jonas Maebe Note Added: 0028656
2009-06-21 11:05 Jonas Maebe Relationship added related to 0009472
2009-06-21 12:02 Martin Schreiber Note Added: 0028657
2009-06-21 12:04 Martin Schreiber Note Edited: 0028657
2009-06-21 12:14 Jonas Maebe Note Added: 0028658
2009-06-21 12:24 Jonas Maebe Note Added: 0028660
2009-06-21 13:49 Martin Schreiber Note Added: 0028667
2009-06-21 14:18 Jonas Maebe Fixed in Revision 2165 => 13310
2009-06-21 14:18 Jonas Maebe Status feedback => resolved
2009-06-21 14:18 Jonas Maebe Fixed in Version 2.0.4 =>
2009-06-21 14:18 Jonas Maebe Resolution reopened => no change required
2009-06-21 14:18 Jonas Maebe Note Added: 0028669
2010-03-06 21:28 Jonas Maebe Status resolved => closed
2010-05-27 11:40 Jonas Maebe Relationship added has duplicate 0016578
2018-10-04 16:11 Michael Van Canneyt Relationship added related to 0030409