diff --git a/rtl/inc/dynarr.inc b/rtl/inc/dynarr.inc index 9e9517d372..0d7b06908b 100644 --- a/rtl/inc/dynarr.inc +++ b/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; + eletypemngd:=pdynarraytypedata(ti)^.elType; + if assigned(eletypemngd) then + eletypemngd:=PPointer(eletypemngd)^; - { determine new memory size } - if assigned(p) then - newhigh:=realp^.high+count - else - newhigh:=count-1; - size:=elesize*(newhigh+1)+sizeof(tdynarray); - - 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); + newp:=getmem(elesize*newlen+sizeof(tdynarray)); - { 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); + { 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); - { increment ref. count of managed members } - if assigned(eletypemngd) then - int_AddRefArray(pointer(newp)+sizeof(tdynarray),eletypemngd,newhigh+1); + { increment ref. count of managed members } + if assigned(eletypemngd) then + int_AddRefArray(newp+1,eletypemngd,newlen); - { a declock(ref. count) isn't enough here } - { it could be that the in MT environments } - { in the mean time the refcount was } - { decremented } + { 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); - - { 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); - - { move the inserted data to the destination } - move(data^,(p+source*elesize)^,count*elesize); - - { increase reference counts of inserted elements } - if assigned(eletypemngd) then - int_AddRefArray(p+source*elesize,eletypemngd,count); - - 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); - { insert data } - move(data^,(pointer(newp)+sizeof(tdynarray))^,count*elesize); + { 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; + + { 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; diff --git a/tests/webtbs/tw40417.pp b/tests/webtbs/tw40417.pp new file mode 100644 index 0000000000..162d8ee107 --- /dev/null +++ b/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(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.