Bug 143707 - Macro optional parameter is incorrectly initialized to the default integer value with Option VBASupport 1
Summary: Macro optional parameter is incorrectly initialized to the default integer va...
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: BASIC (show other bugs)
Version:
(earliest affected)
7.1.5.2 release
Hardware: All All
: medium normal
Assignee: Andreas Heinisch
URL:
Whiteboard: target:7.3.0
Keywords:
Depends on:
Blocks:
 
Reported: 2021-08-03 16:24 UTC by Vladimir Sokolinskiy
Modified: 2021-08-10 10:24 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:
Regression By:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Vladimir Sokolinskiy 2021-08-03 16:24:11 UTC
Description:
Macro optional parameter is incorrectly initialized to the default integer value with Option VBASupport 1

Steps to Reproduce:
Run Macro Test.

Option Explicit 
Option VbaSupport 1

Sub Mysub(Optional ByVal arg1 As Variant=1) 
  Msgbox arg1
End Sub

Sub Test
  Mysub 
End Sub  


Actual Results:
Shows 1%

Expected Results:
MUST show 1


Reproducible: Always


User Profile Reset: No



Additional Info:
Comment 1 Mike Kaganski 2021-08-03 16:30:38 UTC
Repro: Version: 7.2.0.2 (x64) / LibreOffice Community
Build ID: 614be4f5c67816389257027dc5e56c801a547089
CPU threads: 12; OS: Windows 10.0 Build 19043; UI render: Skia/Raster; VCL: win
Locale: ru-RU (ru_RU); UI: en-US
Calc: threaded
Comment 2 Mike Kaganski 2021-08-03 17:09:56 UTC
Looks like "%" is the type character, read at SbiRuntime::StepPARAM using 'pImg->GetString( nDefaultId )'. It is assigned to a variable of type Variant, which happily accepts string as is, without conversion to integer.
Comment 3 Andreas Heinisch 2021-08-03 17:58:35 UTC
The special characters will be added in SbiStringPool::Add.

For instance,

Option Explicit 
Option VbaSupport 1

Sub Mysub(Optional ByVal arg1 As Variant=100000) 
  Msgbox arg1
End Sub

Sub Test
  Mysub 
End Sub  

produces

100000&
Comment 4 Mike Kaganski 2021-08-03 18:20:54 UTC
(In reply to Andreas Heinisch from comment #3)

Thinking about it, I see how the type character is unsafe in case of the default arguments. Yet, the type is really needed...

Looking at SbiImage::GetString, I see that a string in the pool can't have a 0 character inside (the strings are 0-terminated). Yet, we can write such a character in the middle; and we can learn the size of the string by taking offset of the next string, so we may know that there are more characters there.

Can we use embedded 0 to separate number string from its type char maybe?
Comment 5 Andreas Heinisch 2021-08-03 18:28:00 UTC
(In reply to Mike Kaganski  from comment #4)

What about something like:
short SbiStringPool::Add( double n, SbxDataType t, bool bAddSuffixType )
and
snprintf(buf, sizeof(buf), "%d%s", static_cast<short>(n), bAddSuffixType ? "%" : "");
Comment 6 Commit Notification 2021-08-10 10:23:45 UTC
Andreas Heinisch committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/3bcfb1aac1f43f16c579486264103ebd4f3f829b

tdf#143707 - Change strategy to support suffix type characters

It will be available in 7.3.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.