tdf#143707 - Change strategy to support suffix type characters

In order to support the correct data type of numeric constants,
booleans, and default values for strings, the strategy to transport the
data type character to the runtime has been changed.

The type character will be added after the literal and the string
termination symbol in order to keep compatibility. This allows to
retrieve the correct type in StepLOADNC and in StepPARAM.

Any legacy written images are still possible to process, since if there
is a suffix type character where the data type character was directly
written after the numeric constant, it will be processed in StepLOADNC.

Without this fix, an optional parameter of type Variant, would still
include the suffixe type character, and will not be converted to the
correct type in StepPARAM.

Change-Id: I86c8192c6dc28457053fa7b23a073420e45407b2
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/119945
Tested-by: Jenkins
Reviewed-by: Andreas Heinisch <andreas.heinisch@yahoo.de>

6 files changed, 101 insertions, 17 deletions

diff --git a/basic/qa/vba_tests/optional_paramters.vb b/basic/qa/vba_tests/optional_paramters.vb
index 9001ab952de9..d47854d2fb69 100644
--- a/basic/qa/vba_tests/optional_paramters.vb
+++ b/basic/qa/vba_tests/optional_paramters.vb
@@ -22,6 +22,12 @@ End Function
 Sub verify_testOptionalsVba()
     On Error GoTo errorHandler
 
+    ' tdf#143707 - check correct initialization of default value for optionals
+    ' Without the fix in place, this test would have failed with
+    ' - Expected: 123
+    ' - Actual  : 123%
+    TestUtil.AssertEqual(TestOptVariantInit(), 123, "TestOptVariantInit()")
+
     ' optionals with variant datatypes
     TestUtil.AssertEqual(TestOptVariant(), 123, "TestOptVariant()")
     TestUtil.AssertEqual(TestOptVariant(123), 246, "TestOptVariant(123)")
@@ -113,6 +119,10 @@ errorHandler:
     TestUtil.ReportErrorHandler("verify_testOptionalsVba", Err, Error$, Erl)
 End Sub
 
+Function TestOptVariantInit(Optional A As Variant = 123)
+    TestOptVariantInit = A
+End Function
+
 Function TestOptVariant(Optional A, Optional B As Variant = 123)
     TestOptVariant = OptNumberSum(IsMissing(A), A, IsMissing(B), B)
 End Function
diff --git a/basic/qa/vba_tests/typename.vb b/basic/qa/vba_tests/typename.vb
index 4442cba55e98..98bfd58feda4 100644
--- a/basic/qa/vba_tests/typename.vb
+++ b/basic/qa/vba_tests/typename.vb
@@ -53,7 +53,17 @@ Sub verify_testTypeName()
     TestUtil.AssertEqual(TypeName(TestCurrSign),   "Currency", "TypeName(TestCurrSign)")
     TestUtil.AssertEqual(TypeName(TestStrSign),    "String",   "TypeName(TestStrSign)")
 
+    ' tdf#143707 - check correct initialization of default value for optionals
+    ' Without the fix in place, this test would have failed with
+    ' - Expected: Integer
+    ' - Actual  : String
+    TestUtil.AssertEqual(TestOptVariantInit(), "Integer", "TestOptVariantInit()")
+
     Exit Sub
 errorHandler:
     TestUtil.ReportErrorHandler("verify_testTypeName", Err, Error$, Erl)
 End Sub
+
+Function TestOptVariantInit(Optional A As Variant = 123)
+    TestOptVariantInit = TypeName(A)
+End Function
diff --git a/basic/source/classes/image.cxx b/basic/source/classes/image.cxx
index 69e30bb11954..73d65e98eab9 100644
--- a/basic/source/classes/image.cxx
+++ b/basic/source/classes/image.cxx
@@ -20,6 +20,7 @@
 #include <tools/stream.hxx>
 #include <tools/tenccvt.hxx>
 #include <osl/thread.h>
+#include <o3tl/safeint.hxx>
 #include <sal/log.hxx>
 #include <basic/sbx.hxx>
 #include <sb.hxx>
@@ -647,18 +648,18 @@ void SbiImage::AddEnum(SbxObject* pObject) // Register enum type
 }
 
 // Note: IDs start with 1
-OUString SbiImage::GetString( short nId ) const
+OUString SbiImage::GetString( short nId, SbxDataType *eType ) const
 {
     if( nId && nId <= short(mvStringOffsets.size()) )
     {
         sal_uInt32 nOff = mvStringOffsets[ nId - 1 ];
         sal_Unicode* pStr = pStrings.get() + nOff;
 
+        sal_uInt32 nNextOff = (nId < short(mvStringOffsets.size())) ? mvStringOffsets[ nId ] : nStringOff;
+        sal_uInt32 nLen = nNextOff - nOff - 1;
         // #i42467: Special treatment for vbNullChar
-        if( *pStr == 0 )
+        if (*pStr == 0)
         {
-            sal_uInt32 nNextOff = (nId < short(mvStringOffsets.size())) ? mvStringOffsets[ nId ] : nStringOff;
-            sal_uInt32 nLen = nNextOff - nOff - 1;
             if( nLen == 1 )
             {
                 return OUString( u'\0');
@@ -666,7 +667,29 @@ OUString SbiImage::GetString( short nId ) const
         }
         else
         {
-            return OUString(pStr);
+            // tdf#143707 - check if the data type character was added after the string termination
+            // symbol. It was added in basic/source/comp/symtbl.cxx.
+            OUString aOUStr(pStr);
+            if (eType != nullptr)
+            {
+                *eType = SbxSTRING;
+                if (o3tl::make_unsigned(aOUStr.getLength()) < nLen)
+                {
+                    const sal_Unicode pTypeChar = *(pStrings.get() + nOff + aOUStr.getLength() + 1);
+                    switch (pTypeChar)
+                    {
+                        // See GetSuffixType in basic/source/comp/scanner.cxx for type characters
+                        case '%': *eType = SbxINTEGER; break;
+                        case '&': *eType = SbxLONG; break;
+                        case '!': *eType = SbxSINGLE; break;
+                        case '#': *eType = SbxDOUBLE; break;
+                        case '@': *eType = SbxCURRENCY; break;
+                        // tdf#142460 - properly handle boolean values in string pool
+                        case 'b': *eType = SbxBOOL; break;
+                    }
+                }
+            }
+            return aOUStr;
         }
     }
     return OUString();
diff --git a/basic/source/comp/symtbl.cxx b/basic/source/comp/symtbl.cxx
index ed245a364874..6fb7f081d853 100644
--- a/basic/source/comp/symtbl.cxx
+++ b/basic/source/comp/symtbl.cxx
@@ -61,24 +61,49 @@ short SbiStringPool::Add( const OUString& rVal )
     return static_cast<short>(++n);
 }
 
-short SbiStringPool::Add( double n, SbxDataType t )
+short SbiStringPool::Add(double n, SbxDataType t)
 {
-    char buf[40]{};
+    size_t size = 0;
+    const size_t aBufLength = 40;
+    char buf[aBufLength]{};
+
+    // tdf#143707 - add the type character after the null termination of the string in order to
+    // keep compatibility. After the type character has been added, the buffer contains the value
+    // of the double n, the string termination symbol, and the type character.
     switch( t )
     {
         // tdf#142460 - properly handle boolean values in string pool
-        case SbxBOOL: snprintf( buf, sizeof(buf), "%db", static_cast<short>(n) ); break;
+        case SbxBOOL:
+            size = snprintf(buf, sizeof(buf), "%d", static_cast<short>(n)) + 1;
+            buf[size++] = 'b';
+            break;
         // tdf#131296 - store numeric value including its type character
         // See GetSuffixType in basic/source/comp/scanner.cxx for type characters
-        case SbxINTEGER: snprintf( buf, sizeof(buf), "%d%%", static_cast<short>(n) ); break;
+        case SbxINTEGER:
+            size = snprintf(buf, sizeof(buf), "%d", static_cast<short>(n)) + 1;
+            buf[size++] = '%';
+            break;
         case SbxLONG:
-            snprintf( buf, sizeof(buf), "%" SAL_PRIdINT32 "&", static_cast<sal_Int32>(n) ); break;
-        case SbxSINGLE:  snprintf( buf, sizeof(buf), "%.6g!", static_cast<float>(n) ); break;
-        case SbxDOUBLE:  snprintf( buf, sizeof(buf), "%.16g", n ); break; // default processing in SbiRuntime::StepLOADNC - no type character
-        case SbxCURRENCY: snprintf(buf, sizeof(buf), "%.16g@", n); break;
+            size = snprintf(buf, sizeof(buf), "%" SAL_PRIdINT32, static_cast<sal_Int32>(n)) + 1;
+            buf[size++] = '&';
+            break;
+        case SbxSINGLE:
+            size = snprintf(buf, sizeof(buf), "%.6g", static_cast<float>(n)) + 1;
+            buf[size++] = '!';
+            break;
+        case SbxDOUBLE:
+            size = snprintf(buf, sizeof(buf), "%.16g", n) + 1;
+            buf[size++] = '#';
+            break;
+        case SbxCURRENCY:
+            size = snprintf(buf, sizeof(buf), "%.16g", n) + 1;
+            buf[size++] = '@';
+            break;
         default: assert(false); break; // should not happen
     }
-    return Add( OUString::createFromAscii( buf ) );
+
+    // tdf#143707 - add the content of the buffer to the string pool inclding its calculated length
+    return Add(OUString::createFromAscii(std::string_view(buf, size)));
 }
 
 SbiSymPool::SbiSymPool( SbiStringPool& r, SbiSymScope s, SbiParser* pP ) :
diff --git a/basic/source/inc/image.hxx b/basic/source/inc/image.hxx
index f0ea655d6899..b33009c1e029 100644
--- a/basic/source/inc/image.hxx
+++ b/basic/source/inc/image.hxx
@@ -84,7 +84,7 @@ public:
     const sal_uInt8* GetCode() const { return aCode.data(); }
     sal_uInt32 GetCodeSize() const { return aCode.size(); }
     sal_uInt16  GetBase() const     { return nDimBase;  }
-    OUString    GetString( short nId ) const;
+    OUString    GetString( short nId, SbxDataType *eType = nullptr ) const;
     const SbxObject* FindType (const OUString& aTypeName) const;
 
     const SbxArrayRef& GetEnums() const { return rEnums; }
diff --git a/basic/source/runtime/runtime.cxx b/basic/source/runtime/runtime.cxx
index 43e8eea9a0e4..7a246247e903 100644
--- a/basic/source/runtime/runtime.cxx
+++ b/basic/source/runtime/runtime.cxx
@@ -2817,8 +2817,10 @@ void SbiRuntime::StepERROR()
 
 void SbiRuntime::StepLOADNC( sal_uInt32 nOp1 )
 {
+    // tdf#143707 - check if the data type character was added after the string termination symbol
+    SbxDataType eTypeStr;
     // #57844 use localized function
-    OUString aStr = pImg->GetString( static_cast<short>( nOp1 ) );
+    OUString aStr = pImg->GetString(static_cast<short>(nOp1), &eTypeStr);
     // also allow , !!!
     sal_Int32 iComma = aStr.indexOf(',');
     if( iComma >= 0 )
@@ -2833,6 +2835,8 @@ void SbiRuntime::StepLOADNC( sal_uInt32 nOp1 )
     SbxDataType eType = SbxDOUBLE;
     if ( nParseEnd < aStr.getLength() )
     {
+        // tdf#143707 - Check if there was a data type character after the numeric constant,
+        // added by older versions of the fix of the default values for strings.
         switch ( aStr[nParseEnd] )
         {
             // See GetSuffixType in basic/source/comp/scanner.cxx for type characters
@@ -2844,6 +2848,12 @@ void SbiRuntime::StepLOADNC( sal_uInt32 nOp1 )
             case 'b': eType = SbxBOOL; break;
         }
     }
+    // tdf#143707 - if the data type character is different from the default value, it was added
+    // in basic/source/comp/symtbl.cxx. Hence, change the type of the numeric constant to be loaded.
+    else if (eTypeStr != SbxSTRING)
+    {
+        eType = eTypeStr;
+    }
     SbxVariable* p = new SbxVariable( eType );
     p->PutDouble( n );
     // tdf#133913 - create variable with Variant/Type in order to prevent type conversion errors
@@ -4179,9 +4189,15 @@ void SbiRuntime::StepPARAM( sal_uInt32 nOp1, sal_uInt32 nOp2 )
                     sal_uInt16 nDefaultId = static_cast<sal_uInt16>(pParam->nUserData & 0x0ffff);
                     if( nDefaultId > 0 )
                     {
-                        OUString aDefaultStr = pImg->GetString( nDefaultId );
+                        // tdf#143707 - check if the data type character was added after the string
+                        // termination  symbol, and convert the variable if it was present. The
+                        // data type character was It was added in basic/source/comp/symtbl.cxx.
+                        SbxDataType eTypeStr;
+                        OUString aDefaultStr = pImg->GetString( nDefaultId, &eTypeStr );
                         pVar = new SbxVariable(pParam-> eType);
                         pVar->PutString( aDefaultStr );
+                        if (eTypeStr != SbxSTRING)
+                            pVar->Convert(eTypeStr);
                         refParams->Put(pVar, nIdx);
                     }
                     else if ( SbiRuntime::isVBAEnabled() && eType != SbxVARIANT )