Conversion of text with '\0' to DECFLOAT without errors.

[dmitry-lipetsk]

SQL> show version;
ISQL Version: WI-V4.0.3.2937 Firebird 4.0
Server version:
Firebird/Windows/AMD/Intel/x64 (access method), version "WI-V4.0.3.2937 Firebird 4.0"
Firebird/Windows/AMD/Intel/x64 (remote server), version "WI-V4.0.3.2937 Firebird 4.0/tcp (HOME4)/P17:C"
Firebird/Windows/AMD/Intel/x64 (remote interface), version "WI-V4.0.3.2937 Firebird 4.0/tcp (HOME4)/P17:C"
on disk structure version 13.0
SQL> select cast('123'||x'00'||'321' as integer) from rdb$database;

        CAST
============
Statement failed, SQLSTATE = 22018
conversion error from string "123"
SQL> select cast('123'||x'00'||'321' as DECFLOAT(34)) from rdb$database;

                                      CAST
==========================================
                                       123

[AlexPeshkoff]

BTW diags in case of integer is also rather far from ideal

[dmitry-lipetsk]

Alex, I rewrote decFloatFromString to work with a range of characters, and I seem to have found a few pointer issues.

If you haven't started dealing with this issue yet, you can wait while I finish and provide more information.

[AlexPeshkoff]

Have not. It will be great.
BTW, if there are issues with decFloatFromString, that probably should be reported to IBM.

[dmitry-lipetsk]

About a problem with pointers. I'll try to explian a problem as simple as possible :)

Sample query:

select cast('-0000000000000000000000000000000000000000000000000000000000000000000000000000000.E+7000' as DECFLOAT(34))
from rdb$database

decFinalize function receives 'num' with this data:

As you can see, lsd<msd and both members are pointing to uninitialized memory.

msd is pointing on MOST significant digit
lsd is pointing LEAST significant digit

decFinalize expectes that num contains at least one digit - it means that msd<=lsd

Look at this code of decFinalize:

firebird/extern/decNumber/decCommon.c

Lines 261 to 269 in 5ed9d76

	if (!NUMISSPECIAL(num)) {
	Int drop; // digits to be dropped
	// skip leading insignificant zeros to calculate an exact length
	// [this is quite expensive]
	if (*umsd==0) {
	for (; umsd+3<ulsd && UBTOUI(umsd)==0;) umsd+=4;
	for (; *umsd==0 && umsd<ulsd;) umsd++;
	length=ulsd-umsd+1; // recalculate
	}

It reads this bad data in line 265 without any preliminary checks (NUMISSPECIAL does not check these pointers).

This situation is occurring by mistake in decFloatFromString

firebird/extern/decNumber/decCommon.c

Lines 906 to 917 in 5ed9d76

	// [This is a rare and unusual case; arbitrary-length input]
	// strip leading zeros [but leave final 0 if all 0's]
	if (*cfirst=='.') cfirst++; // step past dot at start
	if (*cfirst=='0') { // [cfirst always -> digit]
	for (; cfirst<clast; cfirst++) {
	if (*cfirst!='0') { // non-zero found
	if (*cfirst=='.') continue; // [ignore]
	break; // done
	}
	digits--; // 0 stripped
	} // cfirst
	} // at least one leading 0

This cycle must keep at least one (the last) zero symbol but it is ignoring all them. The reason for it - the last '.'

I rewrote this code:

        // [This is a rare and unusual case; arbitrary-length input]
        // strip leading zeros [but leave final 0 if all 0's]
        {
          char* cfirst2=NULL;

          for (; cfirst<=clast; cfirst++) {
              if (*cfirst == '.') continue;  // [ignore]
              cfirst2=cfirst;                // let's save the position of this digit
              if (*cfirst!='0') break;       // done
              assert(digits>0);
              digits--;                      // 0 stripped
            } // cfirst

          assert(cfirst2 != NULL);
          assert(cfirst2 <= clast);

          if(clast<cfirst) { // all the symbols are ZEROs
            assert(digits==0);
            digits=1;
            } else {
                assert(digits>0);
              }
          cfirst=cfirst2;
        } // local - at least one leading 0

Now decFinalize receives the correct presentation of 'num':

I ran my others tests - it seems all is ok, too.

[dmitry-lipetsk]

The second problem with pointers lives in decFinalize function and I am not sure that I am right.

Look at this code:

firebird/extern/decNumber/decCommon.c

Lines 455 to 469 in 5ed9d76

	if (shift>0) { // fold-down needed
	// fold down needed; must copy to buffer in order to pad
	// with zeros safely; fortunately this is not the worst case
	// path because cannot have had a round
	uByte buffer[ROUNDUP(DECPMAX+3, 4)]; // [+3 allows uInt padding]
	uByte *s=umsd; // source
	uByte *t=buffer; // safe target
	uByte *tlsd=buffer+(ulsd-umsd)+shift; // target LSD
	// printf("folddown shift=%ld\n", (LI)shift);
	for (; s<=ulsd; s+=4, t+=4) UBFROMUI(t, UBTOUI(s));
	for (t=tlsd-shift+1; t<=tlsd; t+=4) UBFROMUI(t, 0); // pad 0s
	num->exponent-=shift;
	umsd=buffer;
	ulsd=tlsd;
	}

Lines with problem:

firebird/extern/decNumber/decCommon.c

Lines 467 to 468 in 5ed9d76

	umsd=buffer;
	ulsd=tlsd;

buffer it is a local memory and can be freed/reused after this block.

I think this 'buffer' must be declared at the top level of decFinalize.

[AlexPeshkoff]

Confirm both problems - with pointers and illegally located buffer. Can you prepare PR?

[dmitry-lipetsk]

Yes, no problems. For FB4 and HEAD.

[dmitry-lipetsk]

Can I add asserts (native asserts, not fb_asserts)?

[AlexPeshkoff]

If asserts are already used in original decfloat library code - yes, certainly. If not - I think it's better to avoid them in order to follow code style of the project. Provided you are not going to rework all code - hope not :)

[dmitry-lipetsk]

Alex, applyed PR does not fix this problem :)

For fix of this issue it is need

1. Detect '\0' before the call of fromString function
   or
2. Create a new fromString function that will work with range of symbols

[AlexPeshkoff]

On 5/30/23 14:39, Dmitry Kovalenko wrote: Detect '\0' before the call of fromString function

That's what I was going to do initially... Will reopen an issue.

[AlexPeshkoff]

Also raise conversion error instead indeterminant decfloat error and output symbols < 32 in \xDD format. Single \ replaced with \.

[dmitry-lipetsk]

Also raise conversion error instead indeterminant decfloat error and output symbols < 32 in \xDD format. Single \ replaced with .

I think decFloatFromString_fb can be upgraded to return an additional info about a bad position and this information can be used for creating a detail message.

At the next stage, of course, not right now.

[AlexPeshkoff]

That makes sense provided all conversion errors are accompanied with error column info.