Repository navigation
Unsafe hard casts on interface results in Quick.Threads.pas #152
Description
Hi there -- first of all, thanks for QuickLib! We use it in production and it saves us a lot of time.
I ran into an issue with Quick.Threads.pas where several methods use hard casts like TTask(Result) or TTask(fCurrentTask) to access internal properties (ExecuteWithSync, TerminateWithSync, etc.) on interface-typed variables. These compile fine but can cause an EAccessViolation at runtime if the interface reference doesn't happen to be backed by the exact class the cast expects; for example when subclassing, mocking, or when the ref-counting/release order changes between compiler versions.
Affected locations (as of commit 5f77567)
| Line | Expression | Context |
|---|---|---|
| 1743 | TTask(fCurrentTask) |
TQueueWorker.Execute -> SetFaultPolicy |
| 1744 | TTask(fCurrentTask) |
TQueueWorker.Execute -> ExecuteWithSync check |
| 1754 | TTask(fCurrentTask) |
TQueueWorker.Execute -> TerminateWithSync check |
| 1782 | TTask(fCurrentTask) |
TQueueWorker.Execute -> second branch, same pattern |
| 1783 | TTask(fCurrentTask) |
TQueueWorker.Execute -> second branch |
| 1794 | TTask(fCurrentTask) |
TQueueWorker.Execute -> second branch |
| 1835 | TTask(Result) |
TScheduledTasks.AddTask_Sync |
| 2161 | TTask(Result) |
TScheduledTask.OnException_Sync |
| 2197 | TTask(Result) |
TScheduledTask.OnTerminated_Sync |
There's also a TScheduledTask(Result) cast at line 2179 (OnExpired_Sync) which has the same pattern.
Why it matters
A hard cast like TTask(SomeInterface) bypasses all type safety. If the underlying object isn't literally a TTask (e.g., it's a descendant that was created through a factory, or the interface reference was reassigned), the cast silently produces a wrong pointer and the next property access is an AV. This is particularly tricky to debug because the crash happens inside the worker thread, not at the call site.
Suggested fix
Replace each hard cast with a Supports() call that safely extracts the concrete class. The cast only needs to happen once per scope, and the result can be reused:
// Before (unsafe):
TTask(fCurrentTask).ExecuteWithSync
// After (safe):
var TaskObj: TTask;
...
if not Supports(fCurrentTask, TTask, TaskObj) then
raise EInvalidCast.Create('Current task is not a TTask');
TaskObj.ExecuteWithSyncFor the AddTask_Sync / OnException_Sync / OnTerminated_Sync methods where Result is assigned from AddTask (which always returns a TTask-backed interface), the pattern is the same:
// Before:
Result := AddTask(...);
TTask(Result).ExecuteWithSync := True;
// After:
var TaskObj: TTask;
...
Result := AddTask(...);
if Supports(Result, TTask, TaskObj) then
TaskObj.ExecuteWithSync := True
else
raise EInvalidCast.Create('AddTask did not return a TTask');We've been running this locally for a while with no issues. Happy to open a PR if that would be helpful.
Thanks again for maintaining the library!
Activity
exilon commentedon Apr 17, 2026
Thanks a lot, I really appreciate it! Yes, please go ahead and open a PR