View Issue Details

IDProjectCategoryView StatusLast Update
0035331FPCRTLpublic2019-04-09 13:36
ReporterSerge AnvarovAssigned ToMichael Van Canneyt 
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Product Version3.3.1Product Build 
Target Version3.2.0Fixed in Version3.3.1 
Summary0035331: Patch. Functions TStrings.GetNextLine and TStrings.GetNextLineBreak does not check parameters
DescriptionParameters: (const Value: string; var S: string; var P: Integer): Boolean;
Fail when:
1. P < 0
2. Value and S are the same variable
The functions are not private, so they can be called with these parameters.
Steps To ReproduceThe test is included (also works for version 3.0.4).
Additional InformationThe patch assumes a new version of "System.Pos".
The new implementation not only corrects these errors, but also eliminates conversions to and from PChar, because there is no difference in speed between Data[0] and Data[Index] on modern processors.
The new implementation also eliminates implicit generation of the "try finally" block, which improves performance.
TagsNo tags attached.
Fixed in Revision41853
FPCOldBugId
FPCTarget
Attached Files
  • StringsNextLineTest.lpr (4,792 bytes)
  • Strings.GetNextLine.diff (3,449 bytes)
    Index: rtl/objpas/classes/stringl.inc
    ===================================================================
    --- rtl/objpas/classes/stringl.inc	(revision 41845)
    +++ rtl/objpas/classes/stringl.inc	(working copy)
    @@ -609,58 +609,69 @@
     end;
     
     Class Function TStrings.GetNextLine (Const Value : String; Var S : String; Var P : Integer) : Boolean;
    -
    -Var 
    -  PS : PChar;
    -  IP,L : Integer;
    -  
    +var
    +  LengthOfValue: SizeInt;
    +  StartPos, FuturePos: SizeInt;
     begin
    -  L:=Length(Value);
    -  S:='';
    -  Result:=False;
    -  If ((L-P)<0) then 
    -    exit;
    -  if ((L-P)=0) and (not (value[P] in [#10,#13])) Then
    -    Begin
    -      s:=value[P];
    -      inc(P);
    -      Exit(True);
    -    End;
    -  PS:=PChar(Value)+P-1;
    -  IP:=P;
    -  While ((L-P)>=0) and (not (PS^ in [#10,#13])) do 
    -    begin
    -    P:=P+1;
    -    Inc(PS);
    -    end;
    -  SetLength (S,P-IP);
    -  System.Move (Value[IP],Pointer(S)^,P-IP);
    -  If (P<=L) and (Value[P]=#13) then 
    -    Inc(P);
    -  If (P<=L) and (Value[P]=#10) then
    -    Inc(P); // Point to character after #10(#13)
    -  Result:=True;
    +  LengthOfValue := Length(Value);
    +  StartPos := P;
    +  if (StartPos <= 0) or (StartPos > LengthOfValue) then // True for LengthOfValue <= 0
    +  begin
    +    S := '';
    +    Exit(False);
    +  end;
    +  FuturePos := StartPos;
    +  while (FuturePos <= LengthOfValue) and not (Value[FuturePos] in [#10, #13]) do
    +    Inc(FuturePos);
    +  // If we use S := Copy(Value, StartPos, FuturePos - StartPos); then compiler
    +  // generate TempS := Copy(...); S := TempS to eliminate side effects and
    +  // implicit "try finally" for TempS finalization
    +  // When we use SetString then no TempS, no try finally generated,
    +  // but we must check case when Value and S is same (side effects)
    +  if Pointer(S) = Pointer(Value) then
    +    System.Delete(S, FuturePos, High(FuturePos))
    +  else
    +  begin
    +    SetString(S, @Value[StartPos], FuturePos - StartPos);
    +    if (FuturePos <= LengthOfValue) and (Value[FuturePos] = #13) then
    +      Inc(FuturePos);
    +    if (FuturePos <= LengthOfValue) and (Value[FuturePos] = #10) then
    +      Inc(FuturePos);
    +  end;
    +  P := FuturePos;
    +  Result := True;
     end;
     
     Function TStrings.GetNextLineBreak (Const Value : String; Var S : String; Var P : Integer) : Boolean;
    -
    -Var
    -  PS,PC,PP : PChar;
    -
    +var
    +  StartPos, FuturePos: SizeInt;
     begin
    -  S:='';
    -  Result:=False;
    -  If ((Length(Value)-P)<0) then
    -    exit;
    -  PS:=@Value[P];
    -  PC:=PS;
    -  PP:=AnsiStrPos(PS,PChar(FLineBreak));
    -  // Stop on #0.
    -  While (PC^<>#0) and (PC<>PP) do
    -    Inc(PC);
    -  P:=P+(PC-PS)+Length(FLineBreak);
    -  SetString(S,PS,PC-PS);
    -  Result:=True;
    +  StartPos := P;
    +  if (StartPos <= 0) or (StartPos > Length(Value)) then // True for Length <= 0
    +  begin
    +    S := '';
    +    Exit(False);
    +  end;
    +  FuturePos := Pos(FLineBreak, Value, StartPos); // Use PosEx in old RTL
    +  // Why we don't use Copy but use SetString read in GetNextLine
    +  if FuturePos = 0 then // No line breaks
    +  begin
    +    FuturePos := Length(Value) + 1;
    +    if Pointer(S) = Pointer(Value) then
    +      // Nothing to do
    +    else
    +      SetString(S, @Value[StartPos], FuturePos - StartPos)
    +  end
    +  else
    +    if Pointer(S) = Pointer(Value) then
    +      System.Delete(S, FuturePos, High(FuturePos))
    +    else
    +    begin
    +      SetString(S, @Value[StartPos], FuturePos - StartPos);
    +      Inc(FuturePos, Length(FLineBreak));
    +    end;
    +  P := FuturePos;
    +  Result := True;
     end;
     
     Procedure TStrings.DoSetTextStr(const Value: string; DoClear : Boolean);
    
    Strings.GetNextLine.diff (3,449 bytes)

Activities

Serge Anvarov

2019-04-06 18:27

reporter  

StringsNextLineTest.lpr (4,792 bytes)

Serge Anvarov

2019-04-06 18:27

reporter  

Strings.GetNextLine.diff (3,449 bytes)
Index: rtl/objpas/classes/stringl.inc
===================================================================
--- rtl/objpas/classes/stringl.inc	(revision 41845)
+++ rtl/objpas/classes/stringl.inc	(working copy)
@@ -609,58 +609,69 @@
 end;
 
 Class Function TStrings.GetNextLine (Const Value : String; Var S : String; Var P : Integer) : Boolean;
-
-Var 
-  PS : PChar;
-  IP,L : Integer;
-  
+var
+  LengthOfValue: SizeInt;
+  StartPos, FuturePos: SizeInt;
 begin
-  L:=Length(Value);
-  S:='';
-  Result:=False;
-  If ((L-P)<0) then 
-    exit;
-  if ((L-P)=0) and (not (value[P] in [#10,#13])) Then
-    Begin
-      s:=value[P];
-      inc(P);
-      Exit(True);
-    End;
-  PS:=PChar(Value)+P-1;
-  IP:=P;
-  While ((L-P)>=0) and (not (PS^ in [#10,#13])) do 
-    begin
-    P:=P+1;
-    Inc(PS);
-    end;
-  SetLength (S,P-IP);
-  System.Move (Value[IP],Pointer(S)^,P-IP);
-  If (P<=L) and (Value[P]=#13) then 
-    Inc(P);
-  If (P<=L) and (Value[P]=#10) then
-    Inc(P); // Point to character after #10(#13)
-  Result:=True;
+  LengthOfValue := Length(Value);
+  StartPos := P;
+  if (StartPos <= 0) or (StartPos > LengthOfValue) then // True for LengthOfValue <= 0
+  begin
+    S := '';
+    Exit(False);
+  end;
+  FuturePos := StartPos;
+  while (FuturePos <= LengthOfValue) and not (Value[FuturePos] in [#10, #13]) do
+    Inc(FuturePos);
+  // If we use S := Copy(Value, StartPos, FuturePos - StartPos); then compiler
+  // generate TempS := Copy(...); S := TempS to eliminate side effects and
+  // implicit "try finally" for TempS finalization
+  // When we use SetString then no TempS, no try finally generated,
+  // but we must check case when Value and S is same (side effects)
+  if Pointer(S) = Pointer(Value) then
+    System.Delete(S, FuturePos, High(FuturePos))
+  else
+  begin
+    SetString(S, @Value[StartPos], FuturePos - StartPos);
+    if (FuturePos <= LengthOfValue) and (Value[FuturePos] = #13) then
+      Inc(FuturePos);
+    if (FuturePos <= LengthOfValue) and (Value[FuturePos] = #10) then
+      Inc(FuturePos);
+  end;
+  P := FuturePos;
+  Result := True;
 end;
 
 Function TStrings.GetNextLineBreak (Const Value : String; Var S : String; Var P : Integer) : Boolean;
-
-Var
-  PS,PC,PP : PChar;
-
+var
+  StartPos, FuturePos: SizeInt;
 begin
-  S:='';
-  Result:=False;
-  If ((Length(Value)-P)<0) then
-    exit;
-  PS:=@Value[P];
-  PC:=PS;
-  PP:=AnsiStrPos(PS,PChar(FLineBreak));
-  // Stop on #0.
-  While (PC^<>#0) and (PC<>PP) do
-    Inc(PC);
-  P:=P+(PC-PS)+Length(FLineBreak);
-  SetString(S,PS,PC-PS);
-  Result:=True;
+  StartPos := P;
+  if (StartPos <= 0) or (StartPos > Length(Value)) then // True for Length <= 0
+  begin
+    S := '';
+    Exit(False);
+  end;
+  FuturePos := Pos(FLineBreak, Value, StartPos); // Use PosEx in old RTL
+  // Why we don't use Copy but use SetString read in GetNextLine
+  if FuturePos = 0 then // No line breaks
+  begin
+    FuturePos := Length(Value) + 1;
+    if Pointer(S) = Pointer(Value) then
+      // Nothing to do
+    else
+      SetString(S, @Value[StartPos], FuturePos - StartPos)
+  end
+  else
+    if Pointer(S) = Pointer(Value) then
+      System.Delete(S, FuturePos, High(FuturePos))
+    else
+    begin
+      SetString(S, @Value[StartPos], FuturePos - StartPos);
+      Inc(FuturePos, Length(FLineBreak));
+    end;
+  P := FuturePos;
+  Result := True;
 end;
 
 Procedure TStrings.DoSetTextStr(const Value: string; DoClear : Boolean);
Strings.GetNextLine.diff (3,449 bytes)

Michael Van Canneyt

2019-04-09 13:36

administrator   ~0115347

Checked and applied (had to patch manually, patch does not apply cleanly)

Many thanks !

Issue History

Date Modified Username Field Change
2019-04-06 18:27 Serge Anvarov New Issue
2019-04-06 18:27 Serge Anvarov File Added: StringsNextLineTest.lpr
2019-04-06 18:27 Serge Anvarov File Added: Strings.GetNextLine.diff
2019-04-06 18:32 Michael Van Canneyt Assigned To => Michael Van Canneyt
2019-04-06 18:32 Michael Van Canneyt Status new => assigned
2019-04-09 13:36 Michael Van Canneyt Fixed in Revision => 41853
2019-04-09 13:36 Michael Van Canneyt Note Added: 0115347
2019-04-09 13:36 Michael Van Canneyt Status assigned => resolved
2019-04-09 13:36 Michael Van Canneyt Fixed in Version => 3.3.1
2019-04-09 13:36 Michael Van Canneyt Resolution open => fixed
2019-04-09 13:36 Michael Van Canneyt Target Version => 3.2.0