* patch and test by Rika: fixes self inserts in Insert(x, dynarray) and improves Insert(x, dynarray), resolves #40417

rtl/inc/dynarr.inc

@@ -485,126 +485,102 @@ procedure fpc_dynarray_delete(var p : pointer;source,count : SizeInt;pti : point
 
 procedure fpc_dynarray_insert(var p : pointer;source : SizeInt;data : pointer;count : SizeInt;pti : pointer);compilerproc;
   var
-    newhigh : tdynarrayindex;
-    size : sizeint;
-    realp,
-    newp : pdynarray;
-    ti : pointer;
-    elesize : sizeint;
-    eletype,eletypemngd : pointer;
+    newlen : tdynarrayindex;
+    elesize,dataofs : sizeint;
+    oldp,newp,realp : pdynarray;
+    ti,eletypemngd : pointer;
   begin
-    if not assigned(data) or
-        (count=0) then
+    if count=0 then
       exit;
 
-    if assigned(p) then
-      realp:=pdynarray(p-sizeof(tdynarray))
-    else
-      realp:=nil;
-    newp:=realp;
-
-    { cap insert index }
-    if assigned(p) then
+    oldp:=p;
+    if assigned(oldp) then
       begin
-        if source<0 then
-          source:=0
-        else if source>realp^.high+1 then
-          source:=realp^.high+1;
+        dec(oldp);
+        { cap insert index }
+        newlen:=oldp^.high+1;
+        if SizeUint(source)>SizeUint(newlen) then { Checks for not (0 <= source <= len), using the fact than 'newlen' is never negative. }
+          if source<0 then
+            source:=0
+          else
+            source:=newlen;
+        newlen:=newlen+count;
       end
     else
-      source:=0;
+      begin
+        source:=0;
+        newlen:=count;
+      end;
 
     { skip kind and name }
-{$ifdef VER3_0}
-     ti:=aligntoptr(Pointer(pti)+2+PByte(pti)[1]);
-{$else VER3_0}
-     ti:=aligntoqword(Pointer(pti)+2+PByte(pti)[1]);
-{$endif VER3_0}
+    ti:=aligntoqword(Pointer(pti)+2+PByte(pti)[1]);
 
     elesize:=pdynarraytypedata(ti)^.elSize;
-    eletype:=pdynarraytypedata(ti)^.elType2^;
     { only set if type needs initialization }
-    if assigned(pdynarraytypedata(ti)^.elType) then
-      eletypemngd:=pdynarraytypedata(ti)^.elType^
-    else
-      eletypemngd:=nil;
-
-    { determine new memory size }
-    if assigned(p) then
-      newhigh:=realp^.high+count
-    else
-      newhigh:=count-1;
-    size:=elesize*(newhigh+1)+sizeof(tdynarray);
+    eletypemngd:=pdynarraytypedata(ti)^.elType;
+    if assigned(eletypemngd) then
+      eletypemngd:=PPointer(eletypemngd)^;
 
-    if assigned(p) then
+    if not assigned(oldp) or (oldp^.refcount<>1) then
       begin
-        if realp^.refcount<>1 then
-          begin
-            { make an unique copy }
-            getmem(newp,size);
-            fillchar(newp^,sizeof(tdynarray),0);
-
-            { copy leading elements }
-            if source>0 then
-              move(p^,(pointer(newp)+sizeof(tdynarray))^,source*elesize);
-            { insert new elements }
-            move(data^,(pointer(newp)+sizeof(tdynarray)+source*elesize)^,count*elesize);
-            { copy trailing elements }
-            if realp^.high-source+1>0 then
-              move((p+source*elesize)^,(pointer(newp)+sizeof(tdynarray)+(source+count)*elesize)^,(realp^.high-source+1)*elesize);
-
-            { increment ref. count of managed members }
-            if assigned(eletypemngd) then
-              int_AddRefArray(pointer(newp)+sizeof(tdynarray),eletypemngd,newhigh+1);
-
-            { a declock(ref. count) isn't enough here }
-            { it could be that the in MT environments  }
-            { in the mean time the refcount was       }
-            { decremented                             }
-
-            { it is, because it doesn't really matter }
-            { if the array is now removed             }
-            fpc_dynarray_clear(p,pti);
-          end
-        else
-          begin
-            { resize the array }
-            reallocmem(realp,size);
-
-            { p might no longer be correct }
-            p:=pointer(realp)+sizeof(tdynarray);
+        newp:=getmem(elesize*newlen+sizeof(tdynarray));
 
-            { move the trailing part after the inserted data }
-            if source<=realp^.high then
-              move((p+source*elesize)^,(p+(source+count)*elesize)^,(realp^.high-source+1)*elesize);
+        { copy leading elements. No-op when not Assigned(oldp) because in this case source = 0. }
+        move(oldp[1],newp[1],source*elesize);
+        { insert new elements }
+        move(data^,(pointer(newp+1)+source*elesize)^,count*elesize);
+        { copy trailing elements. This time must be careful with not Assigned(oldp). }
+        if assigned(oldp) then
+          move((pointer(oldp+1)+source*elesize)^,(pointer(newp+1)+(source+count)*elesize)^,(oldp^.high-source+1)*elesize);
 
-            { move the inserted data to the destination }
-            move(data^,(p+source*elesize)^,count*elesize);
+        { increment ref. count of managed members }
+        if assigned(eletypemngd) then
+          int_AddRefArray(newp+1,eletypemngd,newlen);
 
-            { increase reference counts of inserted elements }
-            if assigned(eletypemngd) then
-              int_AddRefArray(p+source*elesize,eletypemngd,count);
+        { a declock(ref. count) isn't enough here }
+        { it could be that the in MT environments }
+        { in the mean time the refcount was       }
+        { decremented                             }
 
-            newp:=realp;
-          end;
+        { it is, because it doesn't really matter }
+        { if the array is now removed             }
+        fpc_dynarray_clear(p,pti);
       end
     else
       begin
-        { allocate new array }
-        getmem(newp,size);
-        fillchar(newp^,sizeof(tdynarray),0);
+        { dataofs >= 0 means that 'data' points into the source array with byte offset 'dataofs' from the header.
+          dataofs < 0 means that 'data' does not point into the array. }
+        dataofs:=-1;
+        if (data>=oldp) and (data<=pointer(oldp+1)+oldp^.high*elesize) then
+          dataofs:=data-pointer(oldp);
+
+        { resize the array }
+        realp:=oldp; { 'realp' as a 'var'-parameter avoids taking 'oldp' address. }
+        newp:=reallocmem(realp,elesize*newlen+sizeof(tdynarray));
+
+        { Fixup overlapping 'data'. }
+        if dataofs>=0 then
+          begin
+            data:=pointer(newp)+dataofs;
+            { If 'data' points into the trailing part, account for it being moved by 'count'. }
+            if data>=pointer(newp+1)+source*elesize then
+              data:=data+count*elesize;
+          end;
 
-        { insert data }
-        move(data^,(pointer(newp)+sizeof(tdynarray))^,count*elesize);
+        { move the trailing part after the inserted data }
+        move((pointer(newp+1)+source*elesize)^,(pointer(newp+1)+(source+count)*elesize)^,(newp^.high-source+1)*elesize);
+
+        { move the inserted data to the destination }
+        move(data^,(pointer(newp+1)+source*elesize)^,count*elesize);
 
         { increase reference counts of inserted elements }
         if assigned(eletypemngd) then
-          int_AddRefArray(pointer(newp)+sizeof(tdynarray),eletypemngd,count);
+          int_AddRefArray(pointer(newp+1)+source*elesize,eletypemngd,count);
       end;
 
-    p:=pointer(newp)+sizeof(tdynarray);
     newp^.refcount:=1;
-    newp^.high:=newhigh;
+    newp^.high:=newlen-1;
+    p:=newp+1;
   end;
 
 

tests/webtbs/tw40417.pp

@@ -0,0 +1,69 @@
+{$mode objfpc} {$longstrings on} {$coperators on} {$modeswitch implicitfunctionspecialization}
+{$warn 5092 off: Variable "ref" of a managed type does not seem to be initialized }
+
+generic function ToString<T>(const a: array of T): string;
+var
+	i: SizeInt;
+	es: string;
+begin
+	result := '(';
+	for i := 0 to High(a) do
+	begin
+		WriteStr(es, a[i]);
+		if i > 0 then result += ', ';
+		result += es;
+	end;
+	result += ')';
+end;
+
+const
+	WithoutWith: array[boolean] of string = ('without', 'with');
+
+var
+	somethingFailed: boolean = false;
+	a, ref: array of int32;
+	aBefore: pointer;
+	i: SizeInt;
+	tries: int32;
+	withRealloc: boolean;
+
+begin
+	// withRealloc = false:
+	// Catch a bug when inserting an array element into the same array to the left does NOT reallocate the array, shifts the element away, and uses the old pointer.
+	// Reproducible without particular prerequisites.
+	//
+	// withRealloc = true:
+	// Catch a bug when inserting an array element into the same array reallocates the array and uses the old pointer.
+	// In practice requires -gh (heaptrc) or custom memory manager that would fill the old ReallocMem area with garbage to reproduce.
+	for withRealloc in boolean do
+	begin
+		tries := 0;
+		repeat
+			if tries >= 99 then
+			begin
+				writeln('Cannot trigger an insert ' + WithoutWith[withRealloc] + ' reallocation.');
+				halt(2);
+			end;
+			SetLength(a, 5 + tries);
+			for i := 0 to High(a) do a[i] := i; // [0, 1, 2, 3, 4, ...]
+			aBefore := pointer(a);
+			Insert(a[3], a, 2); // a[3] is 3, so it must insert another 3 *before* 2: [0, 1, 3, 2, 3, 4, ...]
+			inc(tries);
+		until pointer(a) <> aBefore = withRealloc; // (:
+
+		SetLength(ref, length(a));
+		for i := 0 to High(ref) do
+			ref[i] := i + ord(i = 2) - ord(i = 3) - ord(i > 3);
+		if CompareByte(pointer(a)^, pointer(ref)^, length(a) * sizeof(a[0])) <> 0 then
+		begin
+			writeln('After inserting a[3] = 3 at position #2 ' + WithoutWith[withRealloc] + ' reallocation:' + LineEnding +
+				'a = ' + ToString(a) + ',' + LineEnding +
+				'expected:' + LineEnding +
+				'a = ' + ToString(ref) + '.' + LineEnding);
+			somethingFailed := true;
+		end;
+	end;
+
+	if somethingFailed then halt(1);
+	writeln('ok');
+end.