Bug 111313 - Crashed in Calc Macro (Basic)
Summary: Crashed in Calc Macro (Basic)
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
4.4 all versions
Hardware: x86-64 (AMD64) All
: medium normal
Assignee: Stephan Bergmann
URL:
Whiteboard: target:6.1.0 target:7.3.0
Keywords: bibisected, bisected, regression
Depends on:
Blocks:
 
Reported: 2017-08-03 15:12 UTC by Bytetrampers
Modified: 2022-04-24 11:20 UTC (History)
4 users (show)

See Also:
Crash report or crash signature: ["rtl_uStringbuffer_insert"]


Attachments
Reproducible document (9.44 KB, application/vnd.oasis.opendocument.text)
2018-04-04 16:27 UTC, himajin100000
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Bytetrampers 2017-08-03 15:12:41 UTC
Hello
I use some macros (VBA) from Excel and build it into LibreOffice-Calc into the macro.

The macro contains the following code:
--------------------
'Replace the character with spaces
Mid (S1, P1) = " "  
--------------------

Mid (S1, P1) = " " <---- This generated a Program-Crach. (Reproducible) 
This is taken over from Excel (VBA)
This function works in VBA

With
Mid (S1, P1,1) = " "
The error could be fixed.
Only should not synonymous generate a program crash, but only a syntax error.

Is contained in the following code:
-----------
  'Any characters that can not be contained in the string with Blank
  'Alle Zeichen, die nicht im String enthalten sein dürfen, mit Blank erseten
  For P1 = 1 To Len(S1)
        Select Case Asc(Mid(S1, P1, 1))
            'alle zulässigen behalten
            Case 78, 69, 83, 87, 44, 46, 32
                ' N   E   S   W   ,   .  leer
                'Nichts machen
            Case 48 To 57
                'sind Zahlen
                'Nichts machen
            Case 45
                'ist Minuszeichen
                'Nichts machen
            Case Else
                'Das Zeichen ersetzen mit Leerzeichen
                'Mid(S1, P1) = " "    '<-- generated a Program-Crash  'Erzeugt einen Programmabsturtz
                Mid(S1, P1,1) = " "   '<-- The error could be fixed.  'Fehler behoben
        End Select
    Next
Comment 1 Xisco Faulí 2017-08-03 15:25:21 UTC Comment hidden (obsolete)
Comment 2 QA Administrators 2018-03-02 10:01:36 UTC Comment hidden (obsolete)
Comment 3 QA Administrators 2018-04-04 13:26:59 UTC Comment hidden (obsolete)
Comment 4 himajin100000 2018-04-04 16:27:04 UTC
Created attachment 141089 [details]
Reproducible document

Run Main()

When I run the macro with my own local build, an assertion error happens at
https://opengrok.libreoffice.org/xref/core/sal/rtl/ustrbuf.cxx?r=5a3bb76c#237

I'm not going to write a patch myself this time(I'm tackling on another issue right now)
Comment 5 himajin100000 2018-04-04 16:27:51 UTC
marking this as NEW
Comment 6 Xisco Faulí 2018-04-05 17:59:11 UTC
Regression introduced by:

author	Stephan Bergmann <sbergman@redhat.com>	2014-11-20 08:23:53 +0100
committer	Stephan Bergmann <sbergman@redhat.com>	2014-11-20 08:34:07 +0100
commit 04ae3d0cc9b671729deabf33c2cea1031d72e6ae (patch)
tree 5b3b8f08aa393c8e659947bd709f3752299686f2
parent e7abad5683a28305204977b57357dddefab0e065 (diff)
len cannot be <= 1 here

Bisected with: bibisect-44max

Adding Cc: to Stephan Bergmann
Comment 7 Xisco Faulí 2018-04-05 17:59:44 UTC
@Stephan, it seems this change avoids the crash:

--- a/sal/rtl/ustrbuf.cxx
+++ b/sal/rtl/ustrbuf.cxx
@@ -154,7 +154,7 @@ void SAL_CALL rtl_uStringbuffer_insert( rtl_uString ** This,
             if( len == 1 )
                 /* optimized for 1 character */
                 pBuf[offset] = *str;
-            else
+            else if( len > 1 )
                 memcpy( pBuf + offset, str, len * sizeof(sal_Unicode) );
         }
         (*This)->length = nOldLen + len;
Comment 8 Stephan Bergmann 2018-04-06 09:56:22 UTC
(In reply to Xisco Faulí from comment #6)
> Regression introduced by:
> 
> author	Stephan Bergmann <sbergman@redhat.com>	2014-11-20 08:23:53 +0100
> committer	Stephan Bergmann <sbergman@redhat.com>	2014-11-20 08:34:07 +0100
> commit 04ae3d0cc9b671729deabf33c2cea1031d72e6ae (patch)
> tree 5b3b8f08aa393c8e659947bd709f3752299686f2
> parent e7abad5683a28305204977b57357dddefab0e065 (diff)
> len cannot be <= 1 here

That commit is merely adding the assert that fires here because SbRtl_Mid (basic/source/runtime/methods.cxx) is calling rtl::OUStringBuffer::remove with bad arguments (*this being "abc", start=0, len=-1).
Comment 9 Commit Notification 2018-04-06 12:56:59 UTC
Stephan Bergmann committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=cc20344010e94eda22fee662aab966d395a0796a

tdf#111313: Honor bWriteNoLenParam in !bCompatibility, too

It will be available in 6.1.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 10 Xisco Faulí 2018-04-10 07:19:02 UTC
Verified in

Version: 6.1.0.0.alpha0+
Build ID: 0a4c1fb68c3619e61099a7c548f550b0d3fd7a53
CPU threads: 4; OS: Linux 4.13; UI render: default; VCL: gtk3; 
Locale: ca-ES (ca_ES.UTF-8); Calc: group

Stephan, Thanks for fixing this.
Comment 11 Xisco Faulí 2018-04-10 07:21:09 UTC
Should it be backported to 6-0?
Comment 12 Stephan Bergmann 2018-04-10 08:02:33 UTC
(In reply to Xisco Faulí from comment #11)
> Should it be backported to 6-0?

I wasn't sure.  For one, I'm not entirely sure what the indented difference of the (non-)compatibility modes in e8deba22e887a972f60ff05551e93c334ac1e7b6 "INTEGRATION: CWS ab26" was, and whether my change is actually faithful to that.  For another, the issue has been present for so long (even inherited from OOo in its original form, merely made more severe by tighter argument checking of the rtl string functions in later versions of LO) that I don't see a need to rush in a (potentially brittle) fix.

But I wouldn't oppose a backport either, if others see a need.
Comment 13 Commit Notification 2021-07-15 08:02:51 UTC
Xisco Fauli committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/0ce3a9dcc071094121252a286aa88529ee09a4eb

tdf#111313: basic_macros: Add unittest

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.
Comment 14 Julien Nabet 2022-04-24 11:20:19 UTC
I'm giving a try at tdf#126913 with a straightforward patch:
diff --git a/basic/source/runtime/methods.cxx b/basic/source/runtime/methods.cxx
index 17b6092a3278..04d1645f4ccb 100644
--- a/basic/source/runtime/methods.cxx
+++ b/basic/source/runtime/methods.cxx
@@ -1092,6 +1092,8 @@ void SbRtl_LTrim(StarBASIC *, SbxArray & rPar, bool)
 
 void SbRtl_Mid(StarBASIC *, SbxArray & rPar, bool bWrite)
 {
+    // Beware, format of Mid is: Mid(stringvar, start, [ length ] ) = string
+    // if "string" is provided, nArgCount is always = 4 even if "length" isn't provided
     int nArgCount = rPar.Count() - 1;
     if ( nArgCount < 2 )
     {
@@ -1147,7 +1149,7 @@ void SbRtl_Mid(StarBASIC *, SbxArray & rPar, bool bWrite)
                 sal_Int32 nReplaceLen;
                 if( bWriteNoLenParam )
                 {
-                    nReplaceLen = nArgLen - nStartPos;
+                    nReplaceLen = std::min(nReplaceStrLen, aArgStr.getLength() - nStartPos);
                 }
                 else
                 {


Xisco: are you sure about test_tdf111313.bas
I mean when doing:
    s = "abc"
    Mid(s,1) = "d"

shouldn't it return:
"dbc" ?

Since the replace string is 1 character long, why removing "bc" ?
Reading 
https://docs.microsoft.com/en-us/office/vba/language/reference/user-interface-help/mid-statement
Part 	Description
stringvar 	Required. Name of string variable to modify.
start 	Required; Variant (Long). Character position in stringvar where the replacement of text begins.
length 	Optional; Variant (Long). Number of characters to replace. If omitted, all of string is used.
string 	Required. String expression that replaces part of stringvar.