Skip to content

Fix dangling btPChar values: the slot now owns its content #316

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
TetzkatLipHoka wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix dangling btPChar values: the slot now owns its content #316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions Source/InvokeCall.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,7 @@ var SysCalConv : TCallConv;
ctx: TRTTIContext;
RttiType : TRttiType;
ResValue : TValue;
BorrowList: TPSPCharBorrowList;
begin
Result := False;
IsStatic := _Self = nil;
Expand Down Expand Up @@ -422,7 +423,12 @@ begin
btU8, btS8, btU16, btS16, btU32, btS32, btSingle, btDouble, btExtended, btString, btPChar, btChar, btCurrency,
btUnicodeString
{$IFNDEF PS_NOINT64}, bts64, btU64{$ENDIF}:
Arg := TValue.From<Pointer>( Pointer(fvar.dta) );
begin
// the callee may overwrite P-Char slots (directly or inside
// records/static arrays); borrow their ownership around the call
PSBorrowPCharOwnership(fvar.aType, fvar.dta, BorrowList);
Arg := TValue.From<Pointer>( Pointer(fvar.dta) );
end;
else
begin
Exit;
Expand All @@ -443,6 +449,7 @@ begin
{$ENDIF CPUX86}

IsConstr := (Integer(CallingConv) and 64) <> 0;
try
if not assigned(res) then
begin
Invoke(Address, Args, SysCalConv, nil); { ignore return }
Expand Down Expand Up @@ -492,11 +499,11 @@ begin
//13
btPointer: res.dta := Pointer(Invoke(Address,Args,SysCalConv,TypeInfo(Pointer),IsStatic).AsType<Pointer>);
//14
btPChar:
btPChar: // the slot owns its content: copy the returned data
{$IFDEF FPC}
ptbtPChar(res.dta)^ := tbtPChar(Invoke(Address,Args,SysCalConv,TypeInfo(tbtPChar),IsStatic).AsOrdinal);
tbtstring(res.dta^) := tbtstring(tbtPChar(Invoke(Address,Args,SysCalConv,TypeInfo(tbtPChar),IsStatic).AsOrdinal));
{$ELSE}
ptbtPChar(res.dta)^ := tbtPChar(Invoke(Address,Args,SysCalConv,TypeInfo(tbtPChar),IsStatic).AsType<tbtPChar>());
tbtstring(res.dta^) := tbtstring(Invoke(Address,Args,SysCalConv,TypeInfo(tbtPChar),IsStatic).AsType<tbtPChar>());
{$ENDIF}
//15
//btResourcePointer
Expand Down Expand Up @@ -606,6 +613,9 @@ begin
Exit;
end; { case }
end; //assigned(res)
finally
PSReownPCharOwnership(BorrowList);
end;
SetLength(Args, 0);
SetLength(old_Args, 0);
SetLength(old_Args2, 0);
Expand Down
86 changes: 82 additions & 4 deletions Source/uPSRuntime.pas
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -893,6 +893,19 @@ function ResultAsRegister(b: TPSTypeRec): Boolean;

function PSGetRecField(const avar: TPSVariantIFC; Fieldno: Longint): TPSVariantIFC;
function PSGetArrayField(const avar: TPSVariantIFC; Fieldno: Longint): TPSVariantIFC;
type
{ btPChar slots own their content as a managed string; before an external
call may write through or replace a var-param p-char, its reference is
borrowed and re-owned afterwards }
TPSPCharBorrowEntry = record
Slot: Pointer; // address of the P-Char slot
Holder: Pointer; // borrowed string reference (owned while borrowed)
end;
TPSPCharBorrowList = array of TPSPCharBorrowEntry;

procedure PSBorrowPCharOwnership(aType: TPSTypeRec; Dta: Pointer; var List: TPSPCharBorrowList);
procedure PSReownPCharOwnership(var List: TPSPCharBorrowList);

function NewTPSVariantRecordIFC(avar: PPSVariant; Fieldno: Longint): TPSVariantIFC;

function NewTPSVariantIFC(avar: PPSVariant; varparam: boolean): TPSVariantIFC;
Expand Down Expand Up @@ -1878,7 +1891,7 @@ procedure InitializeVariant(p: Pointer; aType: TPSTypeRec);
procedure DestroyHeapVariant2(v: Pointer; aType: TPSTypeRec); forward;

const
NeedFinalization = [btStaticArray, btRecord, btArray, btPointer, btVariant {$IFNDEF PS_NOINTERFACES}, btInterface{$ENDIF}, btString {$IFNDEF PS_NOWIDESTRING}, btUnicodestring,btWideString{$ENDIF}];
NeedFinalization = [btStaticArray, btRecord, btArray, btPointer, btVariant {$IFNDEF PS_NOINTERFACES}, btInterface{$ENDIF}, btString, btPChar {$IFNDEF PS_NOWIDESTRING}, btUnicodestring,btWideString{$ENDIF}];

type
TDynArrayRecHeader = packed record
Expand Down Expand Up @@ -1944,7 +1957,10 @@ procedure FinalizeVariant(p: Pointer; aType: TPSTypeRec);
darr: PDynArrayRec;
begin
case aType.BaseType of
btString: tbtString(p^) := '';
{ btPChar slots own their content as a string: the payload pointer IS the
p-char value (readers like the comparison operators already treat the
slot as tbtString) }
btString, btPChar: tbtString(p^) := '';
{$IFNDEF PS_NOWIDESTRING}
btWideString: tbtwidestring(p^) := '';
btUnicodeString: tbtunicodestring(p^) := '';
Expand Down Expand Up @@ -2035,6 +2051,55 @@ function CreateHeapVariant2(aType: TPSTypeRec): Pointer;
InitializeVariant(Result, aType);
end;

procedure PSBorrowPCharOwnership(aType: TPSTypeRec; Dta: Pointer; var List: TPSPCharBorrowList);
var
i, n: Longint;
begin
if (aType = nil) or (Dta = nil) then
Exit;
case aType.BaseType of
btPChar:
begin
n := Length(List);
SetLength(List, n + 1);
List[n].Slot := Dta;
List[n].Holder := Pointer(Dta^); // steal the reference; the slot keeps the raw pointer
end;
btRecord:
for i := 0 to TPSTypeRec_Record(aType).FieldTypes.Count - 1 do
PSBorrowPCharOwnership(TPSTypeRec_Record(aType).FieldTypes[i],
Pointer(IPointer(Dta) + IPointer(TPSTypeRec_Record(aType).RealFieldOffsets[i])), List);
btStaticArray:
for i := 0 to TPSTypeRec_StaticArray(aType).Size - 1 do
PSBorrowPCharOwnership(TPSTypeRec_StaticArray(aType).ArrayType,
Pointer(IPointer(Dta) + IPointer(Cardinal(i) * TPSTypeRec_StaticArray(aType).ArrayType.RealSize)), List);
end;
end;

procedure PSReownPCharOwnership(var List: TPSPCharBorrowList);
var
i: Longint;
s: Pointer;
begin
for i := 0 to Length(List) - 1 do
with List[i] do
begin
if Pointer(Slot^) = Holder then
// untouched by the callee: the slot silently keeps its reference
Holder := nil
else
begin
// the callee stored a foreign pointer: copy its data into an owned
// string and release the original (still owned via the holder)
s := nil;
tbtstring(s) := tbtstring(pansichar(Slot^));
tbtstring(Holder) := '';
Pointer(Slot^) := s; // transfer ownership of the copy into the slot
end;
end;
SetLength(List, 0);
end;

procedure DestroyHeapVariant2(v: Pointer; aType: TPSTypeRec);
begin
if v = nil then exit;
Expand Down Expand Up @@ -4122,13 +4187,20 @@ function CopyArrayContents(dest, src: Pointer; Len: Longint; aType: TPSTypeRec):
Dest := Pointer(IPointer(Dest) + PointerSize);
Src := Pointer(IPointer(Src) + PointerSize);
end;
btClass, btpchar:
btClass:
for i := 0 to Len -1 do
begin
Pointer(Dest^) := Pointer(Src^);
Dest := Pointer(IPointer(Dest) + PointerSize);
Src := Pointer(IPointer(Src) + PointerSize);
end;
btpchar:
for i := 0 to Len -1 do
begin
tbtstring(Dest^) := tbtstring(Src^); // owned: reference copy
Dest := Pointer(IPointer(Dest) + PointerSize);
Src := Pointer(IPointer(Src) + PointerSize);
end;
btU32, btS32, btSingle:
for i := 0 to Len -1 do
begin
Expand Down Expand Up @@ -4757,7 +4829,10 @@ function TPSExec.SetVariantValue(dest, Src: Pointer; desttype, srctype: TPSTypeR
end;
end;
btCurrency: tbtcurrency(Dest^) := PSGetCurrency(Src, srctype);
btPChar: pansichar(dest^) := pansichar(PSGetAnsiString(Src, srctype));
{ do NOT take the payload pointer of a temporary: store a managed copy.
The old code left dest pointing into a string that was freed right
after this statement. }
btPChar: tbtstring(dest^) := PSGetAnsiString(Src, srctype);
btString:
tbtstring(dest^) := PSGetAnsiString(Src, srctype);
btChar: tbtchar(dest^) := PSGetAnsiChar(Src, srctype);
Expand Down Expand Up @@ -11801,6 +11876,9 @@ function ClassCallProcEventPropertyHelper(Caller: TPSExec; p: TPSExternalProcRec
Pointer(Pointer((IPointer(n.dta)+PointerSize))^) := data.Data;
Pointer(Pointer((IPointer(n.dta)+PointerSize2))^) := data.Code;
end;
// n2 is a btPChar variant abused as a raw pointer carrier (@data);
// clear it before destroy - finalization would treat it as a string
PPSVariantDynamicArray(n2)^.Data := nil;
DestroyHeapVariant(n2);
DisposePPSVariantIFCList(Params);
end;
Expand Down
7 changes: 6 additions & 1 deletion Source/x64.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,6 +519,7 @@ _XMM0: Double;
{$ENDIF}
RegUsage: Byte;
CallData: TPSList;
BorrowList: TPSPCharBorrowList;
I: Integer;
pp: ^Byte;
IsSafeCall: Boolean;
Expand Down Expand Up @@ -735,6 +736,9 @@ _XMM0: Double;
btS16, btU32, btS32, btSingle, btDouble, btExtended, btString, btPChar, btChar, btCurrency
{$IFNDEF PS_NOINT64}, bts64, btU64{$ENDIF}:
begin
// the callee may overwrite P-Char slots (directly or inside
// records/static arrays); borrow their ownership around the call
PSBorrowPCharOwnership(fvar.aType, fvar.Dta, BorrowList);
Varptr := fvar.Dta;
end;
else begin
Expand Down Expand Up @@ -1027,7 +1031,7 @@ begin
{$IFNDEF PS_NOWIDESTRING} btWideChar, {$ENDIF} btu16, bts16: tbtu16(res.dta^) := _RAX;
btClass : IPointer(res.dta^) := _RAX;
btu32,bts32: tbtu32(res.dta^) := _RAX;
btPChar: pansichar(res.dta^) := Pansichar(_RAX);
btPChar: tbtstring(res.dta^) := tbtstring(Pansichar(_RAX)); // owned: copy
{$IFNDEF PS_NOINT64}
bts64: tbts64(res.dta^) := Int64(_RAX);
btU64: tbtu64(res.dta^) := UInt64(_RAX);
Expand Down Expand Up @@ -1064,6 +1068,7 @@ begin
end;
Result := True;
finally
PSReownPCharOwnership(BorrowList);
if CallData <> nil then begin
for i := CallData.Count -1 downto 0 do
begin
Expand Down
13 changes: 9 additions & 4 deletions Source/x86.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,7 @@ var
{$ENDIF}

EAX, EDX, ECX: Longint;
BorrowList: TPSPCharBorrowList;

function rp(p: PPSVariantIFC): PPSVariantIFC;
begin
Expand Down Expand Up @@ -367,6 +368,9 @@ var
btS16, btU32, btS32, btSingle, btDouble, btExtended, btString, btPChar, btChar, btCurrency
{$IFNDEF PS_NOINT64}, bts64, btU64{$ENDIF}:
begin
// the callee may overwrite P-Char slots (directly or inside
// records/static arrays); borrow their ownership around the call
PSBorrowPCharOwnership(fvar.aType, fvar.Dta, BorrowList);
Varptr := fvar.Dta;
end;
else begin
Expand Down Expand Up @@ -726,7 +730,7 @@ begin
end;

btu32,bts32{$IFDEF FPC},btArray{$ENDIF}: tbtu32(res.dta^) := RealCall_Register(Address, EAX, EDX, ECX, @Stack[Length(Stack)-3], Length(Stack) div 4, 4, nil);
btPChar: pansichar(res.dta^) := Pansichar(RealCall_Register(Address, EAX, EDX, ECX, @Stack[Length(Stack)-3], Length(Stack) div 4, 4, nil));
btPChar: tbtstring(res.dta^) := tbtstring(Pansichar(RealCall_Register(Address, EAX, EDX, ECX, @Stack[Length(Stack)-3], Length(Stack) div 4, 4, nil))); // owned: copy
{$IFNDEF PS_NOINT64}bts64, btU64:
begin
EAX := RealCall_Register(Address, EAX, EDX, ECX, @Stack[Length(Stack)-3], Length(Stack) div 4, 4, @EDX);
Expand Down Expand Up @@ -809,7 +813,7 @@ begin
btChar, btU8, btS8: tbtu8(res^.Dta^) := RealCall_Other(Address, @Stack[Length(Stack)-3], Length(Stack) div 4, 1, nil);
{$IFNDEF PS_NOWIDESTRING}btWideChar, {$ENDIF}btu16, bts16: tbtu16(res^.Dta^) := RealCall_Other(Address, @Stack[Length(Stack)-3], Length(Stack) div 4, 2, nil);
btClass, btu32, bts32: tbtu32(res^.Dta^):= RealCall_Other(Address, @Stack[Length(Stack)-3], Length(Stack) div 4, 4, nil);
btPChar: pansichar(res^.dta^) := Pansichar(RealCall_Other(Address, @Stack[Length(Stack)-3], Length(Stack) div 4, 4, nil));
btPChar: tbtstring(res^.dta^) := tbtstring(Pansichar(RealCall_Other(Address, @Stack[Length(Stack)-3], Length(Stack) div 4, 4, nil))); // owned: copy
{$IFNDEF PS_NOINT64}bts64, btU64:
begin
EAX := RealCall_Other(Address, @Stack[Length(Stack)-3], Length(Stack) div 4, 4, @EDX);
Expand Down Expand Up @@ -899,7 +903,7 @@ begin
btCHar, btU8, btS8: tbtu8(res^.dta^) := RealCall_Cdecl(Address, @Stack[Length(Stack)-3], Length(Stack) div 4, 1, nil);
{$IFNDEF PS_NOWIDESTRING}btWideChar, {$ENDIF}btu16, bts16: tbtu16(res^.dta^) := RealCall_Cdecl(Address, @Stack[Length(Stack)-3], Length(Stack) div 4, 2, nil);
btClass, btu32, bts32: tbtu32(res^.dta^) := RealCall_Cdecl(Address, @Stack[Length(Stack)-3], Length(Stack) div 4, 4, nil);
btPChar: pansichar(res^.dta^) := Pansichar(RealCall_Cdecl(Address, @Stack[Length(Stack)-3], Length(Stack) div 4, 4, nil));
btPChar: tbtstring(res^.dta^) := tbtstring(Pansichar(RealCall_Cdecl(Address, @Stack[Length(Stack)-3], Length(Stack) div 4, 4, nil))); // owned: copy
{$IFNDEF PS_NOINT64}bts64, btU64:
begin
EAX := RealCall_CDecl(Address, @Stack[Length(Stack)-3], Length(Stack) div 4, 4, @EDX);
Expand Down Expand Up @@ -970,7 +974,7 @@ begin
btChar, btU8, btS8: tbtu8(res^.dta^) := RealCall_Other(Address, @Stack[Length(Stack)-3], Length(Stack) div 4, 1, nil);
{$IFNDEF PS_NOWIDESTRING}btWideChar, {$ENDIF}btu16, bts16: tbtu16(res^.dta^) := RealCall_Other(Address, @Stack[Length(Stack)-3], Length(Stack) div 4, 2, nil);
btclass, btu32, bts32: tbtu32(res^.dta^) := RealCall_Other(Address, @Stack[Length(Stack)-3], Length(Stack) div 4, 4, nil);
btPChar: pansichar(res^.dta^) := Pansichar(RealCall_Other(Address, @Stack[Length(Stack)-3], Length(Stack) div 4, 4, nil));
btPChar: tbtstring(res^.dta^) := tbtstring(Pansichar(RealCall_Other(Address, @Stack[Length(Stack)-3], Length(Stack) div 4, 4, nil))); // owned: copy
{$IFNDEF PS_NOINT64}bts64, btU64:
begin
EAX := RealCall_Other(Address, @Stack[Length(Stack)-3], Length(Stack) div 4, 4, @EDX);
Expand Down Expand Up @@ -1025,6 +1029,7 @@ begin
end;
end;
finally
PSReownPCharOwnership(BorrowList);
for i := CallData.Count -1 downto 0 do
begin
pp := CallData[i];
Expand Down