View Issue Details

IDProjectCategoryView StatusLast Update
0032649FPCRTLpublic2017-12-30 16:52
ReporterCCRDudeAssigned ToMarco van de Voort 
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Platformx64OSWindowsOS Version10.0.16299.19
Product Version3.1.1Product Build3.1.1-r37559 
Target VersionFixed in Version3.1.1 
Summary0032649: WideStringReplace returns invalid output
DescriptionWideStringReplace takes valid input, pattern and replacer widestrings and returns broken widestring.

The output from my test program is:

Input: <$LOCALAPPDATA>\CompanyName\ProductName\
Pattern: <$LOCALAPPDATA>
Replace: C:\Users\UserName\AppData\Local
Output: C:\Users\UserNa????????????????\CompanyName?????????????????????????????

Expected output would be: C:\Users\UserName\AppData\Local\CompanyName\ProductName\
Steps To ReproduceCompile and execute the following test program using an up to date FPC 3.1.1 trunk compiler:
-----------------------------------------------------------------
program WideStringReplaceTest;

uses
   SysUtils;

const
   SIn: WideString = '<$LOCALAPPDATA>\CompanyName\ProductName\';
   SPattern: WideString = '<$LOCALAPPDATA>';
   SReplace: WideString = 'C:\Users\UserName\AppData\Local';

var
   sOut: WideString;
begin
   sOut := WideStringReplace(SIn, SPattern, SReplace, [rfReplaceAll, rfIgnoreCase]);
   WriteLn('Input: ' + SIn);
   WriteLn('Pattern: ' + SPattern);
   WriteLn('Replace: ' + SReplace);
   WriteLn('Output: ' + sOut);
   WriteLn('Done.');
end.
Additional InformationThis works in the official Lazarus 1.6.4 + FPC 3.0.2 build.

It does not work in Lazarus trunk + FPC 3.1.

In all other builds I have for testing, it fails as well.

Using fpcupdeluxe, I verified it still fails with a fresh checkout into a fresh folder (Lazarus trunk & FPC trunk). I know Lazarus is not of importance here, but it was the quickest way for me to test.

There is a previous report (but no answers) about garbaged output, which might be related:
http://lists.freepascal.org/fpc-pascal/2017-March/050555.html

I can try to debug the code tomorrow.
TagsNo tags attached.
Fixed in Revision37882
FPCOldBugId
FPCTarget
Attached Files
  • project1.tar.gz (1,115 bytes)
  • syssr.inc.diff (1,745 bytes)
    diff --git rtl/objpas/sysutils/syssr.inc rtl/objpas/sysutils/syssr.inc
    index b5cdb6f876..5205722b21 100644
    --- rtl/objpas/sysutils/syssr.inc
    +++ rtl/objpas/sysutils/syssr.inc
    @@ -4,6 +4,11 @@ var
       c,d: SRPChar ;
       
     begin
    +  Result:='';
    +  c:= NIL; d:=NIL;
    +  OldPat:='';
    +  Srch:='';
    +
       PatLength:=Length(OldPattern);
       if PatLength=0 then begin
         Result:=S;
    @@ -26,7 +31,7 @@ begin
         repeat
           P:=Pos(OldPat,Srch,P);
           if P>0 then begin
    -        move(NewPattern[1],Result[P],PatLength);
    +        move(NewPattern[1],Result[P],PatLength*SizeOf(SRChar));
             if not (rfReplaceAll in Flags) then exit;
             inc(P,PatLength);
           end;
    @@ -49,7 +54,7 @@ begin
           exit;
         end;
         NewPatLength:=Length(NewPattern);
    -    SetLength(Result,Length(S)+PatCount*(NewPatLength-PatLength)*SizeOf(SRChar));
    +    SetLength(Result,Length(S)+PatCount*(NewPatLength-PatLength));
         P:=1; PrevP:=0;
         c:=SRPChar(Result); d:=SRPChar(S);
         repeat
    @@ -57,12 +62,12 @@ begin
           if P>0 then begin
             Cnt:=P-PrevP-1;
             if Cnt>0 then begin
    -          Move(d^,c^,Cnt);
    +          Move(d^,c^,Cnt*SizeOf(SRChar));
               inc(c,Cnt);
               inc(d,Cnt);
             end;
             if NewPatLength>0 then begin
    -          Move(NewPattern[1],c^,NewPatLength);
    +          Move(NewPattern[1],c^,NewPatLength*SizeOf(SRChar));
               inc(c,NewPatLength);
             end;
             inc(P,PatLength);
    @@ -72,7 +77,9 @@ begin
           end;
         until p=0;
         Cnt:=Length(S)-PrevP;
    -    if Cnt>0 then Move(d^,c^,Cnt);
    +    if Cnt>0 then Move(d^,c^,Cnt*SizeOf(SRChar));
    +    inc(c,Cnt);
    +    c^ := SRChar(#0);
       end;
     end;
     
    @@ -113,4 +120,4 @@ begin
           end;
         end;
     end;
    -*)
    \ No newline at end of file
    +*)
    
    syssr.inc.diff (1,745 bytes)
  • syssr2.inc.diff (1,705 bytes)
    diff --git rtl/objpas/sysutils/syssr.inc rtl/objpas/sysutils/syssr.inc
    index b5cdb6f876..d7c78fb8a2 100644
    --- rtl/objpas/sysutils/syssr.inc
    +++ rtl/objpas/sysutils/syssr.inc
    @@ -4,6 +4,11 @@ var
       c,d: SRPChar ;
       
     begin
    +  Result:='';
    +  c:= NIL; d:=NIL;
    +  OldPat:='';
    +  Srch:='';
    +
       PatLength:=Length(OldPattern);
       if PatLength=0 then begin
         Result:=S;
    @@ -26,7 +31,7 @@ begin
         repeat
           P:=Pos(OldPat,Srch,P);
           if P>0 then begin
    -        move(NewPattern[1],Result[P],PatLength);
    +        move(NewPattern[1],Result[P],PatLength*SizeOf(SRChar));
             if not (rfReplaceAll in Flags) then exit;
             inc(P,PatLength);
           end;
    @@ -49,7 +54,7 @@ begin
           exit;
         end;
         NewPatLength:=Length(NewPattern);
    -    SetLength(Result,Length(S)+PatCount*(NewPatLength-PatLength)*SizeOf(SRChar));
    +    SetLength(Result,Length(S)+PatCount*(NewPatLength-PatLength));
         P:=1; PrevP:=0;
         c:=SRPChar(Result); d:=SRPChar(S);
         repeat
    @@ -57,12 +62,12 @@ begin
           if P>0 then begin
             Cnt:=P-PrevP-1;
             if Cnt>0 then begin
    -          Move(d^,c^,Cnt);
    +          Move(d^,c^,Cnt*SizeOf(SRChar));
               inc(c,Cnt);
               inc(d,Cnt);
             end;
             if NewPatLength>0 then begin
    -          Move(NewPattern[1],c^,NewPatLength);
    +          Move(NewPattern[1],c^,NewPatLength*SizeOf(SRChar));
               inc(c,NewPatLength);
             end;
             inc(P,PatLength);
    @@ -72,7 +77,7 @@ begin
           end;
         until p=0;
         Cnt:=Length(S)-PrevP;
    -    if Cnt>0 then Move(d^,c^,Cnt);
    +    if Cnt>0 then Move(d^,c^,Cnt*SizeOf(SRChar));
       end;
     end;
     
    @@ -113,4 +118,4 @@ begin
           end;
         end;
     end;
    -*)
    \ No newline at end of file
    +*)
    
    syssr2.inc.diff (1,705 bytes)

Activities

Bart Broersma

2017-11-06 19:41

reporter   ~0103918

FWIW: 3.0.4RC1 behaves correctly.

Cyrax

2017-11-07 04:12

reporter  

project1.tar.gz (1,115 bytes)

Cyrax

2017-11-07 04:12

reporter  

syssr.inc.diff (1,745 bytes)
diff --git rtl/objpas/sysutils/syssr.inc rtl/objpas/sysutils/syssr.inc
index b5cdb6f876..5205722b21 100644
--- rtl/objpas/sysutils/syssr.inc
+++ rtl/objpas/sysutils/syssr.inc
@@ -4,6 +4,11 @@ var
   c,d: SRPChar ;
   
 begin
+  Result:='';
+  c:= NIL; d:=NIL;
+  OldPat:='';
+  Srch:='';
+
   PatLength:=Length(OldPattern);
   if PatLength=0 then begin
     Result:=S;
@@ -26,7 +31,7 @@ begin
     repeat
       P:=Pos(OldPat,Srch,P);
       if P>0 then begin
-        move(NewPattern[1],Result[P],PatLength);
+        move(NewPattern[1],Result[P],PatLength*SizeOf(SRChar));
         if not (rfReplaceAll in Flags) then exit;
         inc(P,PatLength);
       end;
@@ -49,7 +54,7 @@ begin
       exit;
     end;
     NewPatLength:=Length(NewPattern);
-    SetLength(Result,Length(S)+PatCount*(NewPatLength-PatLength)*SizeOf(SRChar));
+    SetLength(Result,Length(S)+PatCount*(NewPatLength-PatLength));
     P:=1; PrevP:=0;
     c:=SRPChar(Result); d:=SRPChar(S);
     repeat
@@ -57,12 +62,12 @@ begin
       if P>0 then begin
         Cnt:=P-PrevP-1;
         if Cnt>0 then begin
-          Move(d^,c^,Cnt);
+          Move(d^,c^,Cnt*SizeOf(SRChar));
           inc(c,Cnt);
           inc(d,Cnt);
         end;
         if NewPatLength>0 then begin
-          Move(NewPattern[1],c^,NewPatLength);
+          Move(NewPattern[1],c^,NewPatLength*SizeOf(SRChar));
           inc(c,NewPatLength);
         end;
         inc(P,PatLength);
@@ -72,7 +77,9 @@ begin
       end;
     until p=0;
     Cnt:=Length(S)-PrevP;
-    if Cnt>0 then Move(d^,c^,Cnt);
+    if Cnt>0 then Move(d^,c^,Cnt*SizeOf(SRChar));
+    inc(c,Cnt);
+    c^ := SRChar(#0);
   end;
 end;
 
@@ -113,4 +120,4 @@ begin
       end;
     end;
 end;
-*)
\ No newline at end of file
+*)
syssr.inc.diff (1,745 bytes)

Cyrax

2017-11-07 04:13

reporter   ~0103935

Attached test project and a patch which will fix this issue.

CCRDude

2017-11-07 13:03

reporter   ~0103943

Thanks a lot!

Came back to work this morning and found this patch. Tested and verified that it works. Great!

Marco van de Voort

2017-11-11 22:49

manager   ~0104015

I think the #0 at the end is not necessary. The setlength should do that. If you need it, you probably move too much.

Cyrax

2017-11-24 20:38

reporter  

syssr2.inc.diff (1,705 bytes)
diff --git rtl/objpas/sysutils/syssr.inc rtl/objpas/sysutils/syssr.inc
index b5cdb6f876..d7c78fb8a2 100644
--- rtl/objpas/sysutils/syssr.inc
+++ rtl/objpas/sysutils/syssr.inc
@@ -4,6 +4,11 @@ var
   c,d: SRPChar ;
   
 begin
+  Result:='';
+  c:= NIL; d:=NIL;
+  OldPat:='';
+  Srch:='';
+
   PatLength:=Length(OldPattern);
   if PatLength=0 then begin
     Result:=S;
@@ -26,7 +31,7 @@ begin
     repeat
       P:=Pos(OldPat,Srch,P);
       if P>0 then begin
-        move(NewPattern[1],Result[P],PatLength);
+        move(NewPattern[1],Result[P],PatLength*SizeOf(SRChar));
         if not (rfReplaceAll in Flags) then exit;
         inc(P,PatLength);
       end;
@@ -49,7 +54,7 @@ begin
       exit;
     end;
     NewPatLength:=Length(NewPattern);
-    SetLength(Result,Length(S)+PatCount*(NewPatLength-PatLength)*SizeOf(SRChar));
+    SetLength(Result,Length(S)+PatCount*(NewPatLength-PatLength));
     P:=1; PrevP:=0;
     c:=SRPChar(Result); d:=SRPChar(S);
     repeat
@@ -57,12 +62,12 @@ begin
       if P>0 then begin
         Cnt:=P-PrevP-1;
         if Cnt>0 then begin
-          Move(d^,c^,Cnt);
+          Move(d^,c^,Cnt*SizeOf(SRChar));
           inc(c,Cnt);
           inc(d,Cnt);
         end;
         if NewPatLength>0 then begin
-          Move(NewPattern[1],c^,NewPatLength);
+          Move(NewPattern[1],c^,NewPatLength*SizeOf(SRChar));
           inc(c,NewPatLength);
         end;
         inc(P,PatLength);
@@ -72,7 +77,7 @@ begin
       end;
     until p=0;
     Cnt:=Length(S)-PrevP;
-    if Cnt>0 then Move(d^,c^,Cnt);
+    if Cnt>0 then Move(d^,c^,Cnt*SizeOf(SRChar));
   end;
 end;
 
@@ -113,4 +118,4 @@ begin
       end;
     end;
 end;
-*)
\ No newline at end of file
+*)
syssr2.inc.diff (1,705 bytes)

Cyrax

2017-11-24 20:39

reporter   ~0104241

Attached patch without the #0 addition.

jamie philbrook

2017-11-24 22:31

reporter   ~0104244

Ok, so what happens when I decide to do PWChar(WideStringReplace(...)) without the Null at the end, which has been a normal practice for a long time now.

 There is a very good reason to have a NULL at the end of a string whether you
want to accept that or not.

SetLength should not be counting the null as a valid character but it needs to
be there.

 Short Strings are the only one's I can think of that does not require a null to ensure termination because its not a common type to be using as a Pchar cast.


 I have seen this argument about removing the null many times here, I can say
without a doubt it will cause havoc.

Thaddy de Koning

2017-11-25 11:22

reporter   ~0104251

Last edited: 2017-11-25 11:23

View 2 revisions

Yes. Removing terminating zero's will affect ALL interoperability with C and derivatives. But it is an implicit zero and that is what Marco is referring to I think..

Aslo note we have a UnicodeString... so skip the widestring...

Marco van de Voort

2017-12-30 16:52

manager   ~0105144

Sorry, lost track of this. Fixed.

Issue History

Date Modified Username Field Change
2017-11-06 18:11 CCRDude New Issue
2017-11-06 19:41 Bart Broersma Note Added: 0103918
2017-11-07 04:12 Cyrax File Added: project1.tar.gz
2017-11-07 04:12 Cyrax File Added: syssr.inc.diff
2017-11-07 04:13 Cyrax Note Added: 0103935
2017-11-07 13:03 CCRDude Note Added: 0103943
2017-11-11 22:49 Marco van de Voort Note Added: 0104015
2017-11-24 20:38 Cyrax File Added: syssr2.inc.diff
2017-11-24 20:39 Cyrax Note Added: 0104241
2017-11-24 22:31 jamie philbrook Note Added: 0104244
2017-11-25 11:22 Thaddy de Koning Note Added: 0104251
2017-11-25 11:23 Thaddy de Koning Note Edited: 0104251 View Revisions
2017-12-30 16:52 Marco van de Voort Fixed in Revision => 37882
2017-12-30 16:52 Marco van de Voort Note Added: 0105144
2017-12-30 16:52 Marco van de Voort Status new => resolved
2017-12-30 16:52 Marco van de Voort Fixed in Version => 3.1.1
2017-12-30 16:52 Marco van de Voort Resolution open => fixed
2017-12-30 16:52 Marco van de Voort Assigned To => Marco van de Voort