View Issue Details

IDProjectCategoryView StatusLast Update
0034408LazarusWidgetsetpublic2021-05-11 09:20
ReporterBartek Dajewski Assigned ToJuha Manninen  
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Product Version1.8.4 
Summary0034408: VIRTUAL_VMT_COUNT limit exceeded / insufficient memory size allocated by RegisterWSComponent
DescriptionHello,
RegisterWSComponent allocates fixed size memory for any class, regardless of its VMT size. There is const value VIRTUAL_VMT_COUNT = 128, which is not enough if any widgetset class has more virtual methods. For example latest version of http://wiki.freepascal.org/lzRichEdit has 130 VMT entries, so last two of them (SetSelText and SetZoomState) cannot be used because of the SIGSEGV exception.
Steps To ReproduceSet the SelText or ZoomState property of the RichBox component.
Program stops with SIGSEGV / Access violation exception.
Additional InformationAttached patch replaces constant values used by RegisterWSComponent with variables and determines correct VMT size of each registered class.
I've also had to make slight modifications to lzRichEdit source for testing because of compilation errors. I'll try to attach these changes as another attachment.
TagsNo tags attached.
Fixed in Revisionr59294, r65105
LazTarget-
Widgetset
Attached Files

Activities

Bartek Dajewski

2018-10-11 23:18

reporter  

WSLCLClasses_VVMT_COUNT.diff (3,345 bytes)   
Index: lazarus/lcl/widgetset/wslclclasses.pp
===================================================================
--- lazarus/lcl/widgetset/wslclclasses.pp	(wersja 59284)
+++ lazarus/lcl/widgetset/wslclclasses.pp	(kopia robocza)
@@ -115,14 +115,6 @@
   end;
 
 const
-  // To my knowledge there is no way to tell the size of the
-  // VMT of a given class.
-  // Assume we have no more than 100 virtual entries
-  // 12.10.2013 - changed to 128, since we cannot add more methods in ws classes.zeljko.
-  VIRTUAL_VMT_COUNT = 128;
-  VIRTUAL_VMT_SIZE = vmtMethodStart + VIRTUAL_VMT_COUNT * SizeOf(Pointer);
-
-const
   // vmtAutoTable is something Delphi 2 and not used, we 'borrow' the vmt entry
   vmtWSPrivate = vmtAutoTable;
 
@@ -255,7 +247,9 @@
     SearchAddr: Pointer;
     n, idx: Integer;
     WSPrivate, OrgPrivate: TClass;
-    Processed: array[0..VIRTUAL_VMT_COUNT-1] of Boolean;
+    Processed: array of Boolean;
+    VvmtCount,
+    VvmtSize : Integer;
     {$IFDEF VerboseWSRegistration}
     Indent: String;
     {$ENDIF}
@@ -264,9 +258,16 @@
     then WSPrivate := TWSPrivate
     else WSPrivate := AWSPrivate;
 
+    // Determine VMT count and size => http://wiki.freepascal.org/Compiler-generated_data_and_data_structures
+    VvmtCount := 0;
+    Vvmt := Pointer(ANode^.WSClass) + vmtMethodStart; // AWSComponent is equal to ANode^.WSClass;
+    while (Vvmt^[VvmtCount] <> nil) do
+      Inc(VvmtCount);
+    VvmtSize := vmtMethodStart + VvmtCount * SizeOf(Pointer);
+
     if ANode^.VClass = nil
     then begin
-      ANode^.VClass := GetMem(VIRTUAL_VMT_SIZE)
+      ANode^.VClass := GetMem(VvmtSize)
     end
     else begin
       // keep original WSPrivate (only when different than default class)
@@ -282,8 +283,7 @@
     end;
 
     // Initially copy the WSClass
-    // Tricky part, the source may get beyond read mem limit
-    Move(Pointer(ANode^.WSClass)^, ANode^.VClass^, VIRTUAL_VMT_SIZE);
+    Move(Pointer(ANode^.WSClass)^, ANode^.VClass^, VvmtSize);
 
     // Set WSPrivate class
     ParentWSNode := FindParentWSClassNode(ANode);
@@ -321,6 +321,7 @@
 
     Vvmt := ANode^.VClass + vmtMethodStart;
     Pvmt := ParentWSNode^.VClass + vmtMethodStart;
+    SetLength(Processed, VvmtCount);
     FillChar(Processed[0], SizeOf(Processed), 0);
 
     while CommonClass <> nil do
@@ -334,7 +335,7 @@
         {$ENDIF}
 
         Cvmt := Pointer(CommonClass) + vmtMethodStart;
-        Assert(Cmnt^.Count < VIRTUAL_VMT_COUNT, 'MethodTable count is larger than assumed VIRTUAL_VMT_COUNT');
+        Assert(Cmnt^.Count < VvmtCount, 'MethodTable count is larger than determined VvmtCount');
 
         // Loop through the VMT to see what is overridden
         for n := 0 to Cmnt^.Count - 1 do
@@ -344,7 +345,7 @@
           DebugLn('%sSearch: %s (%p)', [Indent, Cmnt^.Entries[n].Name^, SearchAddr]);
           {$ENDIF}
 
-          for idx := 0 to VIRTUAL_VMT_COUNT - 1 do
+          for idx := 0 to VvmtCount - 1 do
           begin
             if Cvmt^[idx] = SearchAddr
             then begin
@@ -372,7 +373,7 @@
 
               Break;
             end;
-            if idx = VIRTUAL_VMT_COUNT - 1
+            if idx = VvmtCount - 1
             then begin
               DebugLn('[WARNING] VMT entry "', Cmnt^.Entries[n].Name^, '" not found in "', CommonClass.ClassName, '"');
               Break;
WSLCLClasses_VVMT_COUNT.diff (3,345 bytes)   

Bartek Dajewski

2018-10-11 23:19

reporter  

lzRichEditChanges.zip (10,068 bytes)

Juha Manninen

2018-10-13 11:59

developer   ~0111389

This is an impressive patch. I have tested it and didn't see any problems.
Applied, thanks.

Maybe you would like to work on removing the whole "multiple inheritance" VMT hack? The solution details must be discussed in Lazarus mailing list.

Bartek Dajewski

2018-10-16 22:49

reporter   ~0111432

Thank you. I thought it was not such a big deal :-)

And yes, I would like to work on anything related to Lazarus. Even though I don't have too much free time, I would love to try :-) Could you point me to any thread about removing Virtual-VMT hack in mailing list? I've tried to find similar discussions in http://lists.lazarus-ide.org/pipermail/lazarus/, but with no luck. Am I looking in wrong place?

Regards
Bartek

Bartek Dajewski

2021-05-10 23:15

reporter   ~0130816

Last edited: 2021-05-10 23:17

View 2 revisions

There is still one more thing to do, when type of Processed variable changed from regular to dynamic array: initializing its elements with FillChar should be discarded. I mean procedure CreateVClass in WSLCLClasses.pas.
First of all, there is an error in second argument to FillChar:
   SetLength(Processed, VvmtCount);
   FillChar(Processed[0], SizeOf(Processed), 0);
SizeOf is improperly used instead of VvmtCount. Fortunately, I believe, it is harmless due to memory alignment :-)
Furthermore, it is useless because SetLength does the job (https://www.freepascal.org/docs-html/rtl/system/setlength.html)
WSLCLClasses_VVMT_COUNT_suplement.diff (509 bytes)   
Index: lcl/widgetset/wslclclasses.pp
===================================================================
--- lcl/widgetset/wslclclasses.pp	(wersja 65096)
+++ lcl/widgetset/wslclclasses.pp	(kopia robocza)
@@ -335,7 +335,6 @@
   Vvmt := ANode^.VClass + vmtMethodStart;
   Pvmt := ParentWSNode^.VClass + vmtMethodStart;
   SetLength(Processed, VvmtCount);
-  FillChar(Processed[0], SizeOf(Processed), 0);
 
   while CommonClass <> nil do begin
     Cmnt := PPointer(Pointer(CommonClass) + vmtMethodTable)^;

Juha Manninen

2021-05-11 06:43

developer   ~0130817

Please reopen the issue in situations like this. Fortunately I noticed your comment and patch now.

Juha Manninen

2021-05-11 06:47

developer   ~0130818

Yes, looks good and valid. Applied, thanks.

Bartek Dajewski

2021-05-11 09:20

reporter   ~0130821

Oops! I neglected to reopen the issue.
Fortunately, thanks to your perceptiveness, everything is fine now :-)

Issue History

Date Modified Username Field Change
2018-10-11 23:18 Bartek Dajewski New Issue
2018-10-11 23:18 Bartek Dajewski File Added: WSLCLClasses_VVMT_COUNT.diff
2018-10-11 23:19 Bartek Dajewski File Added: lzRichEditChanges.zip
2018-10-13 11:56 Juha Manninen Assigned To => Juha Manninen
2018-10-13 11:56 Juha Manninen Status new => assigned
2018-10-13 11:59 Juha Manninen Fixed in Revision => r59294
2018-10-13 11:59 Juha Manninen LazTarget => -
2018-10-13 11:59 Juha Manninen Note Added: 0111389
2018-10-13 11:59 Juha Manninen Status assigned => resolved
2018-10-13 11:59 Juha Manninen Resolution open => fixed
2018-10-16 22:49 Bartek Dajewski Note Added: 0111432
2021-05-10 23:15 Bartek Dajewski Note Added: 0130816
2021-05-10 23:15 Bartek Dajewski File Added: WSLCLClasses_VVMT_COUNT_suplement.diff
2021-05-10 23:17 Bartek Dajewski Note Edited: 0130816 View Revisions
2021-05-11 06:43 Juha Manninen Status resolved => assigned
2021-05-11 06:43 Juha Manninen Resolution fixed => open
2021-05-11 06:43 Juha Manninen Note Added: 0130817
2021-05-11 06:47 Juha Manninen Status assigned => resolved
2021-05-11 06:47 Juha Manninen Resolution open => fixed
2021-05-11 06:47 Juha Manninen Fixed in Revision r59294 => r59294, r65105
2021-05-11 06:47 Juha Manninen Note Added: 0130818
2021-05-11 09:20 Bartek Dajewski Note Added: 0130821