We recently ran into a nasty little bug in the Delphi XE2 compiler, that arises with a complex set of conditions:
- A record with a string property that has a get function;
- A call to this getter that has a constant string appended to the result;
- This result passed directly to another arbitrary function.
When these conditions are met (see code below for examples), the compiler generates code that crashes.
Reproducing the bug
The following program is enough to reproduce the bug.
program ustrcatbug;
type
TWrappedString = record
private
FValue: string;
function GetValue: string;
public
property Value: string read GetValue;
end;
{ TWrappedString }
function TWrappedString.GetValue: string;
begin
Result := FValue;
end;
function GetWrappedString: TWrappedString;
begin
Result.FValue := 'Something';
end;
begin
writeln(GetWrappedString.Value + ' Else');
end.
Looking at the last line of code from that sample in the disassembler, we see the following code:
ustrcatbug.dpr.28: writeln(GetWrappedString.Value + ' Else'); 0040712A 8D45EC lea eax,[ebp-$14] 0040712D E816E9FFFF call GetWrappedString 00407132 8D55EC lea edx,[ebp-$14] 00407135 B8C0BB4000 mov eax,$0040bbc0 0040713A 8B0DE8594000 mov ecx,[$004059e8] 00407140 E883D8FFFF call @CopyRecord 00407145 8D55E8 lea edx,[ebp-$18] 00407148 B8C0BB4000 mov eax,$0040bbc0 0040714D E8D6E8FFFF call TWrappedString.GetValue 00407152 8D45E8 lea eax,[ebp-$18] 00407155 BAB4714000 mov edx,$004071b4 0040715A E899D6FFFF call @UStrCat 0040715F 8B10 mov edx,[eax] 00407161 A12C884000 mov eax,[$0040882c] 00407166 E86DC6FFFF call @Write0UString 0040716B E868C7FFFF call @WriteLn 00407170 E867BCFFFF call @_IOTest ustrcatbug.dpr.29: end. 00407175 33C0 xor eax,eax
The problem arises with these two lines:
0040715A E899D6FFFF call @UStrCat 0040715F 8B10 mov edx,[eax]
The problem is that eax is not preserved through function calls with Delphi’s default register calling convention. And, as _UStrCat is a procedure, we can make no assumptions about the return value (which is passed in eax):
procedure _UStrCat(var Dest: UnicodeString; const Source: UnicodeString);
The issue does not arise if we avoid using the property:
writeln(GetWrappedString.GetValue + ' Else');
From this, the compiler generates:
ustrcatbug.dpr.28: writeln(GetWrappedString.GetValue + ' Else'); 0040712A 8D45E8 lea eax,[ebp-$18] 0040712D E816E9FFFF call GetWrappedString 00407132 8D55E8 lea edx,[ebp-$18] 00407135 B8C0BB4000 mov eax,$0040bbc0 0040713A 8B0DE8594000 mov ecx,[$004059e8] 00407140 E883D8FFFF call @CopyRecord 00407145 B8C0BB4000 mov eax,$0040bbc0 0040714A 8D55EC lea edx,[ebp-$14] 0040714D E8D6E8FFFF call TWrappedString.GetValue 00407152 8D45EC lea eax,[ebp-$14] 00407155 BAB4714000 mov edx,$004071b4 0040715A E899D6FFFF call @UStrCat 0040715F 8B55EC mov edx,[ebp-$14] 00407162 A12C884000 mov eax,[$0040882c] 00407167 E86CC6FFFF call @Write0UString 0040716C E867C7FFFF call @WriteLn 00407171 E866BCFFFF call @_IOTest ustrcatbug.dpr.29: end. 00407176 33C0 xor eax,eax
Where we now see that edx is reloaded from the stack, as it should be:
0040715A E899D6FFFF call @UStrCat 0040715F 8B55EC mov edx,[ebp-$14]
Workarounds
There are a number of possible workarounds:
- Upgrade to Delphi XE7 – this problem appears to be resolved, though I could not find a QC or RSP report relating to the bug when I searched. Moving to XE7 is not an option for us in the short term: too many code changes, too many unknowns.
- Don’t use properties in records. This means changing code to call functions instead: no big deal for the
Get, but annoying for theSethalf of the function pair. - Patch around the problem by modifying
UStrCatto return the address of theDestparameter.
In the end, I wrote option (3) but we went with option (2) in our code base.
Because UStrCat is implemented in System.pas, it’s difficult to build your own version of the unit. One way to skin the option 3 UStrCat is to copy the implementation of UStrCat and its dependencies (a bunch of memory and string manipulation functions), and monkeypatch at runtime.
In the copied functions, we need to preserve eax through the function calls. This results in the addition of 4 lines of assembly to the UStrCat and UStrAsg functions, pushing the eax register onto the stack and popping it before exit. I haven’t reproduced the code here because the original is copyrighted to Embarcadero, but here are the changes required:
- In
UStrCat, Addpush eaxjust after the conditional jump toUStrAsg, andpop eaxjust before theretinstruction. - In
UStrAsg, wrap the call toFreeMemwith apush eaxandpop eax.
The patch function is also pretty straightforward:
procedure MonkeyPatch(OldProc, NewProc: PBYTE); var pBase, p: PBYTE; oldProtect: Cardinal; begin p := OldProc; pBase := p; // Allow writes to this small bit of the code section VirtualProtect(pBase, 5, PAGE_EXECUTE_WRITECOPY, oldProtect); // First write the long jmp instruction. p := pBase; p^ := $E9; // long jmp opcode Inc(p); PDWord(p)^ := DWORD(NewProc) - DWORD(p) - 4; // address to jump to, relative to EIP // Finally, protect that memory again now that we are finished with it VirtualProtect(pBase, 5, oldProtect, oldProtect); end; function GetUStrCatAddr: Pointer; assembler; asm lea eax,System.@UStrCat end; initialization MonkeyPatch(GetUStrCatAddr, @_UStrCatMonkey); end.
This solution does give me the heebie jeebies, because we are patching the symptoms of the problem as we’ve seen them arise, without being able to either understand or address the root cause within the compiler. It’s not really possible to guarantee that there won’t be some other code path that causes this solution to come unstuck without really digging deep into the compiler’s code generation.
As noted, this problem appears to be resolved in Delphi XE5 or possibly earlier; however it is unclear if the root cause of the problem has been addressed, or we just got lucky. The issue has been reported as RSP-10255.
Security flaws like 
