Bug 109331 - crash on invalid basic macro
Summary: crash on invalid basic macro
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: BASIC (show other bugs)
Version:
(earliest affected)
6.0.0.0.alpha0+
Hardware: All All
: high major
Assignee: Caolán McNamara
URL:
Whiteboard: target:6.0.0
Keywords: bibisected, bisected, haveBacktrace, regression
Depends on:
Blocks:
 
Reported: 2017-07-25 06:55 UTC by Miklos Vajna
Modified: 2017-07-26 06:57 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:


Attachments
bt with debug symbols (4.43 KB, text/plain)
2017-07-25 12:58 UTC, Julien Nabet
Details
valgrind trace (34.51 KB, application/x-bzip)
2017-07-25 18:34 UTC, Julien Nabet
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Miklos Vajna 2017-07-25 06:55:23 UTC
On recent master any invalid basic macro like this:

Sub Main

asdf

End Sub

results in a crash when it's executed (instead of showing an error message).

I'll try to bisect or at least bibisect this.
Comment 1 Miklos Vajna 2017-07-25 07:26:15 UTC
Bibisect points out the e0bafa78e3ad0df397d78cd65ad19bd5b07dc5f2..62f4151683cc6bde8db61820aa581ad9073b0841 range, this includes 00657aef09d854c74fb426a935a3e8b1fc390bb0 (migrate to boost::gettext, 2017-06-11), that may be the problematic commit.
Comment 2 Julien Nabet 2017-07-25 12:58:14 UTC
Created attachment 134836 [details]
bt with debug symbols

On Macos 10.12 with master sources updated yesterday, I could reproduce this.
I attached bt with symbols.
Comment 3 Julien Nabet 2017-07-25 12:59:25 UTC
Let's increase the importance since:
- it's a crash
- it's a regression
- it happens on several platforms
Comment 4 Xisco Faulí 2017-07-25 17:11:28 UTC
Regression introduced by

author	Caolán McNamara <caolanm@redhat.com>	2017-06-11 19:56:30 (GMT)
committer	Caolán McNamara <caolanm@redhat.com>	2017-07-21 07:20:50 (GMT)
commit 00657aef09d854c74fb426a935a3e8b1fc390bb0 (patch)
tree fd1a9bb264fe15dcc129498e62060ecd256b1ee7
parent fa987cbb813cfd729fe490f2f1258b7c8d7fb174 (diff)
migrate to boost::gettext

Adding Cc: to Caolán McNamara
Comment 5 Julien Nabet 2017-07-25 18:34:41 UTC
Created attachment 134848 [details]
valgrind trace
Comment 6 Julien Nabet 2017-07-25 19:13:24 UTC
1) Valgrind shows this (I added content of the code line):
==22847== Invalid read of size 4
==22847==    at 0xFC552BA: ErrorInfo::GetErrorCode() const (errinf.hxx:147)
ErrCode                 GetErrorCode() const { return nUserId; }

==22847==   by 0xFC53E58: ErrorHandler::HandleError(ErrCode, DialogMask) (errinf.cxx:179)
if (pInfo->GetErrorCode() != ERRCODE_ABORT)

...
==22847==  Address 0x55c05a08 is 8 bytes inside a block of size 32 free'd
==22847==    at 0x4C2D2DB: operator delete(void*) (vg_replace_malloc.c:576)
==22847==    by 0xFC61178: StringErrorInfo::~StringErrorInfo() (errinf.hxx:171)
class SAL_WARN_UNUSED VCL_DLLPUBLIC StringErrorInfo : public DynamicErrorInfo

==22847==    by 0xFC5390F: ErrorHandler::GetErrorString(ErrCode, rtl::OUString&) (errinf.cxx:107)
delete pInfo;

==22847==    by 0xFC53C89: ErrorHandler::HandleError(ErrCode, DialogMask) (errinf.cxx:151)
if (ErrorHandler::GetErrorString(nErrCodeId, aErr))

So pInfo is invalid because we deleted it.

2) pInfo is retrieved with this line:
ErrorInfo *pInfo = ErrorInfo::GetErrorInfo(nErrCodeId);
(see https://opengrok.libreoffice.org/xref/core/vcl/source/window/errinf.cxx#99
+
https://opengrok.libreoffice.org/xref/core/vcl/source/window/errinf.cxx#118)

Here's the content of GetErrorInfo (in this same file):
272  ErrorInfo *ErrorInfo::GetErrorInfo(ErrCode nId)
273  {
274      if(nId.IsDynamic())
275          return ImplDynamicErrorInfo::GetDynamicErrorInfo(nId);
276      else
277          return new ErrorInfo(nId);
278  }

After having added some traces, it shows we enter in dynamic part.
Comment 7 Julien Nabet 2017-07-25 19:17:26 UTC
Let's give it a try with https://gerrit.libreoffice.org/#/c/40428/
At least, no crash with it.
Miklos/Caolán: I put you in cc of this patch as you may have seen.
Comment 8 Julien Nabet 2017-07-25 19:25:27 UTC
Argh, even if ImplDynamicErrorInfo::GetDynamicErrorInfo(ErrCode nId) doesn't call "new" in our cases, it may sometimes call "new".
261  ErrorInfo* ImplDynamicErrorInfo::GetDynamicErrorInfo(ErrCode nId)
262  {
263      sal_uInt32 nIdx = nId.GetDynamic() - 1;
264      DynamicErrorInfo* pDynErrInfo = TheErrorRegistry::get().ppDynErrInfo[nIdx];
265  
266      if(pDynErrInfo && ErrCode(*pDynErrInfo)==nId)
267          return pDynErrInfo;
268      else
269          return new ErrorInfo(nId.StripDynamic());
270  }
Comment 9 Julien Nabet 2017-07-25 19:49:06 UTC
I don't know how to distinguish pure dynamic case of the other case, so can't assign myself and abandon the patch.
Comment 10 Caolán McNamara 2017-07-25 20:26:10 UTC
I see it, https://gerrit.libreoffice.org/#/c/40433/
Comment 11 Commit Notification 2017-07-25 21:04:54 UTC
Caolán McNamara committed a patch related to this issue.
It has been pushed to "master":

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

Resolves: tdf#109331 stupid looking cast was relied upon to do useful thing

It will be available in 6.0.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 12 Miklos Vajna 2017-07-26 06:53:01 UTC
Thanks for the quick fix! :-)