View Issue Details

IDProjectCategoryView StatusLast Update
0035331FPCRTLpublic2020-06-24 13:33
ReporterSerge Anvarov Assigned ToMichael Van Canneyt  
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Product Version3.3.1 
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

Activities

Serge Anvarov

2019-04-06 16:27

reporter  

Serge Anvarov

2019-04-06 16: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 11:36

administrator   ~0115347

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

Many thanks !

Serge Anvarov

2020-06-24 13:33

reporter   ~0123567

In 3.2.0

Issue History

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