View Issue Details

IDProjectCategoryView StatusLast Update
0038999FPCDatabasepublic2021-06-21 09:26
Reporterwp Assigned To 
PrioritynormalSeverityminorReproducibilityalways
Status newResolutionopen 
Summary0038999: TDbf has issues when writing large float values
DescriptionWhen writing large numbers to a float field of a dbf table two kinds of errors have been observed by me:
- The order of magnitude is not written correctly, e.g. when writing the value 1E20 the value 1E19 may appear in the db.
- When writing very large values an access violation can occur
Steps To ReproduceRun the attached demo

- It attempts to write the value 1E20 to a float field set up with Size=20 and Precision=5. Reading the field after posting, however, yields the value 1E19 instead of 1E20.

 - Replace the value to be written by 1E23. Now an access violation happens.
Additional InformationThe issue occurs in the procedure FloatToDbfStr of unit dbf_dbffile:

procedure FloatToDbfStr(const Val: Extended; const Size, Precision: Integer; const Dest: PChar);
var
  Buffer: array [0..24] of Char;
  resLen: Integer;
  iPos: PChar;
begin
  // convert to temporary buffer
  resLen := FloatToText(@Buffer[0], Val, {$ifndef FPC_VERSION}fvExtended,{$endif} ffFixed, Size, Precision);
  // prevent overflow in destination buffer
  if resLen > Size then
    resLen := Size;
  ...

It uses the function FloatToText to convert the value to be written to a string. It does it in fixed format - as aconsequence the string can become very long for very large numbers. FloatToText, however, requires a preallocated buffer for the result string, and this buffer is overflowing when the string is too long.

My solution: Use FloatToStrF which simply returns a string and thus automatically takes care of buffer allocation.

In order to avoid another overflow in the record buffer into which the result string will be inserted at the end of the FloatToDbfStr procedure the procedure checks whether the string length (resLen) is greater than the space reserved in the buffer (Size). When the string is too long it is simply chopped to the correct size. This is ok when the lost digts are decimals, but it is absolutely wrong when the list digits are "in front of the decimal point" as is can happen with large numbers.

My solution here: Rather than truncating the result string apply FloatToStrF a second time, but now with exponential format so that numbers of any size fit into the string length allowed (of course within the limits of the data type).

These modifications are used in the attached patch. After application of the patch, the issues reported are gone.
TagsNo tags attached.
Fixed in Revision
FPCOldBugId
FPCTarget
Attached Files

Activities

wp

2021-06-13 22:59

reporter  

Project1.pas (887 bytes)   
program Project1;

uses
  SysUtils, db, dbf;
var
  tbl: TDbf;

begin
  FormatSettings.DecimalSeparator := '.';

  tbl := TDbf.Create(nil);
  try
    tbl.FilePath := ExtractFilePath(ParamStr(0));
    tbl.TableName := 'test.dbf';
    with tbl.FieldDefs.AddFieldDef do
    begin
      Name := 'Data';
      DataType := ftFloat;
      Size := 20;
      Precision := 5;
    end;
    if FileExists(tbl.FilePath + tbl.TableName) then
      DeleteFile(tbl.FilePath + tbl.TableName);
    tbl.CreateTable;
    tbl.Open;
    tbl.Append;
    try
//      tbl.Fields[0].AsFloat := 1.0E20;  // --> is written as 1.0E19
      tbl.Fields[0].AsFloat := 1.0E23;    // --> access violation
      tbl.Post;
      WriteLn(tbl.Fields[0].AsString);
    except
      on E:Exception do
        WriteLn(E.Message);
    end;
  finally
    tbl.Free;
  end;

  ReadLn;
end.

Project1.pas (887 bytes)   
dbf_dbffile.pas.patch (1,548 bytes)   
Index: packages/fcl-db/src/dbase/dbf_dbffile.pas
===================================================================
--- packages/fcl-db/src/dbase/dbf_dbffile.pas	(revision 49498)
+++ packages/fcl-db/src/dbase/dbf_dbffile.pas	(working copy)
@@ -286,21 +286,22 @@
 
 procedure FloatToDbfStr(const Val: Extended; const Size, Precision: Integer; const Dest: PChar);
 var
-  Buffer: array [0..24] of Char;
+  Buffer: String;
   resLen: Integer;
   iPos: PChar;
 begin
   // convert to temporary buffer
-  resLen := FloatToText(@Buffer[0], Val, {$ifndef FPC_VERSION}fvExtended,{$endif} ffFixed, Size, Precision);
-  // prevent overflow in destination buffer
+  Buffer := FloatToStrF(Val, ffFixed, Size, Precision);
+  reslen := Length(Buffer);
   if resLen > Size then
-    resLen := Size;
-  // null-terminate buffer
-  Buffer[resLen] := #0;
+  begin
+    Buffer := FloatToStrF(Val, ffExponent, Size-7, 4);
+    resLen := Length(Buffer);
+  end;
   // we only have to convert if decimal separator different
   if DecimalSeparator <> sDBF_DEC_SEP then
   begin
-    iPos := StrScan(@Buffer[0], DecimalSeparator);
+    iPos := StrScan(@Buffer[1], DecimalSeparator);
     if iPos <> nil then
       iPos^ := sDBF_DEC_SEP;
   end;
@@ -307,7 +308,7 @@
   // fill destination with spaces
   FillChar(Dest^, Size, ' ');
   // now copy right-aligned to destination
-  Move(Buffer[0], Dest[Size-resLen], resLen);
+  Move(Buffer[1], Dest[Size-resLen], resLen);
 end;
 
 function GetIntFromStrLength(Src: Pointer; Size: Integer; Default: Integer): Integer;
dbf_dbffile.pas.patch (1,548 bytes)   

Michal Gawrycki

2021-06-16 20:05

reporter   ~0131341

DBF format does not provide exponential form for numeric fields.

wp

2021-06-17 09:40

reporter   ~0131351

When I use Delphi 7 and the BDE (that's as close to "real" dbf files as I can get) and create a dbf file with a ftFloat field (Size = 0, Precision = 5) and the value of a record is - say - 1.2345E23 then the file is written without any issue.

When I look into this file with an editor I see that the field is written in exponential format as " .12345000000000E+24", similar to TDbf after application of my patch (and to the sourceforge version by the way) (Note that Delphi uses this format only when the Delphi-only property TableType is ttFoxPro; when TableType is ttDBase the binary O field type ("double") is written rather than the textual "N" field ("Numeric")).

I opened these dbf files (tfFloat field with value 1.2345E23 in text format, all table levels) created by Delphi and by the patched FPC in various programs (LibreOffice Base, Microsoft Access, DBFPlus) and they could be read flawlessly and correctly (except for the Laz/TableLevel7 which is considered to be defective by LibreOffice Base).

Michal Gawrycki

2021-06-17 16:26

reporter   ~0131359

I did some tests. DBase / Clipper / Harbor reads such a file with value 1.00000 (definitely wrong). Advantage Database Server can correctly read this file, but cannot save such value - an attempt to assign a value outside the range ends with an exception. Harbor behaves similarly - it ends with an error when trying to write such a value. It is worth checking how FoxPro behaves.
Another problem with the use of exponential notation will be indexes, which are defined as a string and sorted by comparing strings.
I think we should act like ADS and Harbour/Clipper in this case. In this example (field size 20,5), the range is from '-9999999999999.99999' to '99999999999999.99999'.

As for the length of the numeric field, DBase allows a maximum of 20 characters, ADS to 32 and in Clipper it is probably unlimited.

In this case, the best data type would be double. I took a quick look at the TDBF code and it seems that it supports such a field type. Unfortunately, it cannot be created from FieldDefs.

wp

2021-06-17 17:45

reporter   ~0131360

Last edited: 2021-06-17 17:47

View 2 revisions

In view of this, it is probably better to not touch the conversion.

But I still think that the buffer overflow must be addressed. What about simply increasing the buffer size to something which gives more safety margin:

procedure FloatToDbfStr(const Val: Extended; const Size, Precision: Integer; const Dest: PChar);
var
  Buffer: array [0..99] of Char; // maybe overkill:
...

This leaves us with the silent truncation of the string which should raise an exception.

if resLen > Size then
  raise EDbfError.Create('Field size overflow');

What do you think?

Michal Gawrycki

2021-06-17 19:28

reporter   ~0131364

I think it's a good idea. A truncated string is also a wrong value so we shouldn't let that happen.
When I have more time, I will take a closer look at the tdbf code. For numeric fields, TBCDField or TFMTBCDField class would be more appropriate than TFloatField.

wp

2021-06-18 22:00

reporter   ~0131392

Last edited: 2021-06-19 10:42

View 2 revisions

Added a new patch:
- Kept the original float-to-text conversion, but increased the buffer size to avoid buffer overrun.
- Raises an exception when the value string is truncated *left* of the decimal separator. Truncation of float values *right* of the decimal separator, conversely, is unavoidable because of the precision parameter.
dbf_dbffile.pas-v2.patch (1,735 bytes)   
Index: packages/fcl-db/src/dbase/dbf_dbffile.pas
===================================================================
--- packages/fcl-db/src/dbase/dbf_dbffile.pas	(revision 49509)
+++ packages/fcl-db/src/dbase/dbf_dbffile.pas	(working copy)
@@ -286,15 +286,21 @@
 
 procedure FloatToDbfStr(const Val: Extended; const Size, Precision: Integer; const Dest: PChar);
 var
-  Buffer: array [0..24] of Char;
+  Buffer: array [0..255] of Char;
   resLen: Integer;
   iPos: PChar;
+  truncated: Boolean = false;
+  inpval: Extended;
+  bufval: Extended;
 begin
   // convert to temporary buffer
   resLen := FloatToText(@Buffer[0], Val, {$ifndef FPC_VERSION}fvExtended,{$endif} ffFixed, Size, Precision);
   // prevent overflow in destination buffer
   if resLen > Size then
+  begin
     resLen := Size;
+    truncated := true;
+  end;
   // null-terminate buffer
   Buffer[resLen] := #0;
   // we only have to convert if decimal separator different
@@ -308,6 +314,21 @@
   FillChar(Dest^, Size, ' ');
   // now copy right-aligned to destination
   Move(Buffer[0], Dest[Size-resLen], resLen);
+
+  // When the field size is too small the value is truncated in the destination
+  // buffer. This is considered harmful since truncation removed digits left
+  // of the decimal point.
+  if truncated then
+  begin
+    // Determine integer part of value in buffer
+    iPos := StrScan(@Buffer[0], sDBF_DEC_SEP);
+    if iPos <> nil then iPos^ := #0;
+    TextTofloat(PChar(@Buffer[0]), bufVal);
+    // integer part of inpur value
+    inpval := int(Val);
+    if bufval <> inpval then
+      raise EDbfError.Create('Field overflow');
+  end;
 end;
 
 function GetIntFromStrLength(Src: Pointer; Size: Integer; Default: Integer): Integer;
dbf_dbffile.pas-v2.patch (1,735 bytes)   

Michal Gawrycki

2021-06-21 09:26

reporter   ~0131408

I don't think we should manipulate precision. We should only accept values that fit the range of field. The solution you proposed earlier is enough - increasing the buffer size and raising exception when value is not within the size.

Issue History

Date Modified Username Field Change
2021-06-13 22:59 wp New Issue
2021-06-13 22:59 wp File Added: Project1.pas
2021-06-13 22:59 wp File Added: dbf_dbffile.pas.patch
2021-06-16 20:05 Michal Gawrycki Note Added: 0131341
2021-06-17 09:40 wp Note Added: 0131351
2021-06-17 16:26 Michal Gawrycki Note Added: 0131359
2021-06-17 17:45 wp Note Added: 0131360
2021-06-17 17:47 wp Note Edited: 0131360 View Revisions
2021-06-17 19:28 Michal Gawrycki Note Added: 0131364
2021-06-18 22:00 wp Note Added: 0131392
2021-06-18 22:00 wp File Added: dbf_dbffile.pas-v2.patch
2021-06-19 10:42 wp Note Edited: 0131392 View Revisions
2021-06-21 09:26 Michal Gawrycki Note Added: 0131408