Bug 109331

Bibisect points out the e0bafa78e3ad0df397d78cd65ad19bd5b07dc5f2..62f4151683cc6bde8db61820aa581ad9073b0841 range, this includes 00657aef09d854c74fb426a935a3e8b1fc390bb0 (migrate to boost::gettext, 2017-06-11), that may be the problematic commit.

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.

Let's increase the importance since:
- it's a crash
- it's a regression
- it happens on several platforms

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

Created attachment 134848 [details]
valgrind trace

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.

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.

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  }

I don't know how to distinguish pure dynamic case of the other case, so can't assign myself and abandon the patch.

I see it, https://gerrit.libreoffice.org/#/c/40433/

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.

Thanks for the quick fix! :-)