All Projects FPC fpcprojects fpprofilerfpGUI Lazarus Packages PatchesLazarus CCR Mantis pas2js

View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0031176FPCRTLpublic2016-12-30 16:032017-01-25 22:24
ReporterDmitriy PomerantsevAssigned ToMichael Van Canneyt 
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Product Version3.1.1Product Build35216 
Target Version3.2.0Fixed in Version3.1.1 
Summary0031176: [patch] SysUtils.StrToBool() may fail with non latin locales
DescriptionIf a programmer is trying to convert a string into a Boolean value with a floating point in a locale where the decimal separator is ",", the function will raise an exception.
This is because inside is used the Val() procedure, which suports only the "." decimal separator. Fortunately in functions such TryStrToFloat(), this problem has been solved.
I propose a patch to resolve this issue.
Steps To ReproduceThe sample program:

{$MODE OBJFPC}{$H+}
program strtobool;
uses
  SysUtils;

begin
  FormatSettings.DecimalSeparator := ',';
  Writeln(SysUtils.StrToBool('1,0'));
end.

Result in current FPC version:

An unhandled exception occurred at $0000000100014350:
EConvertError: "1,0" is not a valid boolean.
  $0000000100014350
  $00000001000016BD main, line 8 of strtobool.lpr
  $00000001000016F6
  $000000010000DFD3
  $0000000100001682
  $0000000076FB59CD
  $00000000770EA561

Result with patch:

TRUE
TagsNo tags attached.
Fixed in Revision35331
FPCOldBugId
FPCTarget
Attached Files
  • TryStrToBool.patch (1,026 bytes) 
    Index: rtl/objpas/sysutils/sysstr.inc
===================================================================
--- rtl/objpas/sysutils/sysstr.inc	(revision 35216)
+++ rtl/objpas/sysutils/sysstr.inc	(working copy)
@@ -1969,14 +1969,17 @@
   Code: word;
 begin
   Temp:=upcase(S);
-  Val(temp,D,code);
-  Result:=true;
-  If Code=0 then
 {$ifdef FPUNONE}
-    Value:=(D<>0)
+  Result:=TryStrToInt(Temp,D);
 {$else}
-    Value:=(D<>0.0)
+  Result:=TryStrToFloat(Temp,D);
 {$endif}
+  If Result then
+{$ifdef FPUNONE}
+  Value:=(D<>0)
+{$else}
+  Value:=(D<>0.0)
+{$endif}
   else
     begin
       CheckBoolStrs;
@@ -1984,13 +1987,13 @@
         if Temp=upcase(TrueBoolStrs[I]) then
           begin
             Value:=true;
-            exit;
+            exit(true);
           end;
       for I:=low(FalseBoolStrs) to High(FalseBoolStrs) do
         if Temp=upcase(FalseBoolStrs[I]) then
           begin
             Value:=false;
-            exit;
+            exit(true);
           end;
       Result:=false;
     end;
    TryStrToBool.patch (1,026 bytes)

Activities

Dmitriy Pomerantsev

2016-12-30 16:03

reporter  

TryStrToBool.patch (1,026 bytes) 
Index: rtl/objpas/sysutils/sysstr.inc
===================================================================
--- rtl/objpas/sysutils/sysstr.inc	(revision 35216)
+++ rtl/objpas/sysutils/sysstr.inc	(working copy)
@@ -1969,14 +1969,17 @@
   Code: word;
 begin
   Temp:=upcase(S);
-  Val(temp,D,code);
-  Result:=true;
-  If Code=0 then
 {$ifdef FPUNONE}
-    Value:=(D<>0)
+  Result:=TryStrToInt(Temp,D);
 {$else}
-    Value:=(D<>0.0)
+  Result:=TryStrToFloat(Temp,D);
 {$endif}
+  If Result then
+{$ifdef FPUNONE}
+  Value:=(D<>0)
+{$else}
+  Value:=(D<>0.0)
+{$endif}
   else
     begin
       CheckBoolStrs;
@@ -1984,13 +1987,13 @@
         if Temp=upcase(TrueBoolStrs[I]) then
           begin
             Value:=true;
-            exit;
+            exit(true);
           end;
       for I:=low(FalseBoolStrs) to High(FalseBoolStrs) do
         if Temp=upcase(FalseBoolStrs[I]) then
           begin
             Value:=false;
-            exit;
+            exit(true);
           end;
       Result:=false;
     end;
TryStrToBool.patch (1,026 bytes)

Michael Van Canneyt

2016-12-30 16:47

administrator   ~0097174

Last edited: 2016-12-30 16:47

View 2 revisions

 IMHO your patch breaks backwards compatibility.

FormatSettings.DecimalSeparator:=',';
B:=StrToBool('1.0') ;
will currently set B to true.

With your patch, it will set it to False (or worse, raise an exception) because 1.0 is not a valid float.

Dmitriy Pomerantsev

2016-12-30 19:53

reporter   ~0097176

Last edited: 2016-12-30 19:53

View 2 revisions

 And Delphi agree with you. If a decimal separator is a comma, the "1.0" is not a valid number. (We will get an exception.)

If you only knew how many programmers used locale depended functions, and then wondered - why the program stopped working outside of Russia. ;-)

But this is the right way. For special cases, there are overloaded functions that accept TFormatSettings as a parameter. But not for StrToBool(). At least now.

Dmitriy Pomerantsev

2017-01-11 14:07

reporter   ~0097413

So what's next? I understand that Free Pascal's RTL sometimes gently expand Delphi RTL. For example, even in 64-bit Delphi you can't have a string longer than 2Gb, arrays of more High(Integer) elements, etc.

Maybe StrToBool() can support the floats with a dot in any locale. But it's not its job. All RTL functions must comply the same rules. All related functions are depends on FormatSettings. And once StrToBool() delegates string to float conversion to TryStrToFloat(), then TryStrToFloat() have to decide what the float number and what's not. Because every function have to do its job. And nothing else.

Perhaps, in this case is better to write in fpc-devel and ask the opinion of other developers.

Marco van de Voort

2017-01-12 10:26

manager   ~0097430

Wouldn't it then falsely interpret 1.000 (one thousand with a thousandseparator) as 1.0 ?

Dmitriy Pomerantsev

2017-01-12 12:04

reporter   ~0097433

Last edited: 2017-01-12 12:11

View 5 revisions

 *sigh* So now we have a Torvalds memcpy() situation.

Now StrToBool() doesn't work properly. You afraid that the correct fix break programs that rely on the current behavior. But the current or fixed behavior can break the correct programs.

For example with decimal separator "," and assuming "." is also decimal separator, we get False with "." or even "0." string as input. But it can be just a random strings, not a false booleans. Error may occur much later.

But the way, your example is incorrect because ThousandSeparator is for Currency type and

FormatSettings.ThousandSeparator := '.';
FormatSettings.DecimalSeparator := ',';
StrToFloat('1' + FormatSettings.ThousandSeparator + '000')

gives us

"EConvertError: '1.000' is not a valid floating point value"

both in Delphi and FPC.

StrToBool('1' + FormatSettings.ThousandSeparator + '000')

gives us

EConvertError: '1.000' is not a valid boolean value

in Delphi.

Michael Van Canneyt

2017-01-25 22:24

administrator   ~0097708

StrToBool now also accepts a localized float. The non-localized version takes priority.

It means there is now an overloaded version that accepts a TFormatSettings argument for StrToBool, TryStrToBool, StrToBoolDef.

Issue History

Date Modified Username Field Change
2016-12-30 16:03 Dmitriy Pomerantsev New Issue
2016-12-30 16:03 Dmitriy Pomerantsev File Added: TryStrToBool.patch
2016-12-30 16:45 Michael Van Canneyt Assigned To => Michael Van Canneyt
2016-12-30 16:45 Michael Van Canneyt Status new => assigned
2016-12-30 16:47 Michael Van Canneyt Note Added: 0097174
2016-12-30 16:47 Michael Van Canneyt Status assigned => feedback
2016-12-30 16:47 Michael Van Canneyt Note Edited: 0097174 View Revisions
2016-12-30 19:53 Dmitriy Pomerantsev Note Added: 0097176
2016-12-30 19:53 Dmitriy Pomerantsev Status feedback => assigned
2016-12-30 19:53 Dmitriy Pomerantsev Note Edited: 0097176 View Revisions
2017-01-11 14:07 Dmitriy Pomerantsev Note Added: 0097413
2017-01-12 10:26 Marco van de Voort Note Added: 0097430
2017-01-12 12:04 Dmitriy Pomerantsev Note Added: 0097433
2017-01-12 12:05 Dmitriy Pomerantsev Note Edited: 0097433 View Revisions
2017-01-12 12:05 Dmitriy Pomerantsev Note Edited: 0097433 View Revisions
2017-01-12 12:06 Dmitriy Pomerantsev Note Edited: 0097433 View Revisions
2017-01-12 12:11 Dmitriy Pomerantsev Note Edited: 0097433 View Revisions
2017-01-25 22:24 Michael Van Canneyt Fixed in Revision => 35331
2017-01-25 22:24 Michael Van Canneyt Note Added: 0097708
2017-01-25 22:24 Michael Van Canneyt Status assigned => resolved
2017-01-25 22:24 Michael Van Canneyt Fixed in Version => 3.1.1
2017-01-25 22:24 Michael Van Canneyt Resolution open => fixed
2017-01-25 22:24 Michael Van Canneyt Target Version => 3.2.0