Bug 114815 - Crash in: take_gil on paste from clipboard in Windows
Summary: Crash in: take_gil on paste from clipboard in Windows
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
5.2.0.4 release
Hardware: All Windows (All)
: medium critical
Assignee: Michael Stahl (allotropia)
URL:
Whiteboard: target:6.1.0 target:6.0.1 target:5.4.6
Keywords:
: 115268 115500 115633 116614 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-01-02 23:07 UTC by Ivan Kovalev
Modified: 2018-04-15 00:29 UTC (History)
8 users (show)

See Also:
Crash report or crash signature: ["take_gil","LngSvcMgr::GetAvailableGrammarSvcs_Impl()"]


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ivan Kovalev 2018-01-02 23:07:51 UTC
This bug was filed from the crash reporting server and is br-5fe61fa1-e973-405b-b084-47b36d6c9798.
=========================================

Reason  for  the  crash: paste text  from  clipboard. Text  was copied from Kindle  reader  for  PC.
Comment 1 Timur 2018-01-03 16:07:15 UTC Comment hidden (me-too)
Comment 2 Timur 2018-01-03 16:08:05 UTC
There are 21824 reports for this and latest is LO 6.0.0.0: https://crashreport.libreoffice.org/stats/signature/take_gil
So this bug can be confirmed.
But it would be really helpful to write exact minimal steps to reproduce. 
Do you repro consistently or intermittently?
Comment 3 Timur 2018-01-03 16:09:30 UTC
So far we see reports from 5.2.0 but with steps it should be tested earlier.
Comment 4 Ivan Kovalev 2018-01-03 16:52:57 UTC
I used Ctr-Insert to copy and Shift-Insert to paste. The size  of  the  data  was just  one  text  page. The issue  was intermittent AND, = I  have a feeling = it  was related to  how fast  I  was doing  "paste" after  opening Writer. All  crashes were  happening if  I  paste the same  very  moment Writer was loaded and opened the  blank document. If the Writer was open before, I had no  problem  paste to  it  the  same data.
Comment 5 Xisco Faulí 2018-01-03 17:44:43 UTC
(In reply to Ivan Kovalev from comment #4)
> I used Ctr-Insert to copy and Shift-Insert to paste. The size  of  the  data
> was just  one  text  page. The issue  was intermittent AND, = I  have a
> feeling = it  was related to  how fast  I  was doing  "paste" after  opening
> Writer. All  crashes were  happening if  I  paste the same  very  moment
> Writer was loaded and opened the  blank document. If the Writer was open
> before, I had no  problem  paste to  it  the  same data.

Do you reproduce the same behaviour if the paste is done coming from another program?
BTW, this is the Kindle reader https://www.amazon.es/kindle-dbs/fd/kcp ?
Comment 6 Ivan Kovalev 2018-01-03 19:35:40 UTC
I  reported my user experience, I  did  not  do  any  special  testing. Yes, source  of  the  data  was Kindle  reader you  pointed to  (  I  loaded  from  English  language  page  ). No, I  do not  think source  of  the  data  matters. As I  pointed before, it  feels  as the time  lag  between Writer  being loaded  and "paste" is  attempted that  matters.
Comment 7 Mike Kaganski 2018-01-16 09:21:15 UTC
The problem looks to be locking the mutex (executing MUTEX_LOCK(gil_mutex); in workdir/unpackedtarball/python3/python/ceval_gil.h:214). This looks consistent with other cases, where we have ntdll.dll 0x4a259-related crashes (see https://www.google.ru/search?q=ntdll.dll+0x4a259, which gives details of other crashes, most of them related to osl_acquireMutex).

Most possibly the gil_mutex hasn't been initialized yet?
Comment 8 Mike Kaganski 2018-01-29 06:45:42 UTC
In CreateInstance( const Reference< XComponentContext > & ) (pyuno/source/loader/pyuno_loader.cxx), there is a check that initializes Python **and threads** when !Py_IsInitialized().

Can't it be that sometimes Python may be initialized somehow already, but miss threads initialization? so that in this case, PyEval_InitThreads() isn't called, global interpreter lock isn't created, and following attempt to use it crashes?

The idea is to use another check to initialize threads:

    if( ! Py_IsInitialized() )
    {
        ...
        // initialize python
        Py_Initialize();
    }

    if( ! PyEval_ThreadsInitialized() )
    {
        PyEval_InitThreads();

        PyThreadState *tstate = PyThreadState_Get();
        PyEval_ReleaseThread( tstate );
        // This tstate is never used again, so delete it here.
        // This prevents an assertion in PyThreadState_Swap on the
        // PyThreadAttach below.
        PyThreadState_Delete(tstate);
    }

...
Comment 9 Mike Kaganski 2018-01-29 08:11:14 UTC
https://gerrit.libreoffice.org/48808
Comment 10 Xisco Faulí 2018-01-29 08:53:08 UTC
Hi Mike,
Could the root cause be the same as in bug 115268?
Comment 11 Timur 2018-01-29 09:51:58 UTC
I guess we may wait now to see if there will be further crashes in 6.1 or have this backported in 6.0 and see that sooner. 
Because it's not likely to get steps.
Comment 12 Mike Kaganski 2018-01-29 10:19:36 UTC
(In reply to Xisco Faulí from comment #10)
> Could the root cause be the same as in bug 115268?

Yes, looking at the minidump, that's the same - but i'm curious if that's related to some Python extension installed, or why the Python gets called at all?
Comment 13 Michael Stahl (allotropia) 2018-02-01 12:13:14 UTC
i have a potential explanation:

https://crashreport.libreoffice.org/stats/crash_details/5fe61fa1-e973-405b-b084-47b36d6c9798#allthreads

look at thread 17, it's here:

                uno::Reference< linguistic2::XProofreader > xGC( GetGrammarChecker( aCurLocale ), uno::UNO_QUERY );
                if (xGC.is())
                {
                     aGuard.clear();
HERE--->             uno::Sequence<beans::PropertyValue> const aProps(
                         lcl_makeProperties(xFlatPara));
                     aRes = xGC->doProofreading( aCurDocId, aCurTxt,
                             aCurLocale, nStartPos, nSuggestedEnd, aProps );


so it has *just* returned from GetGammarChecker - and we have a lightproof grammar checker implemented in python.

my theory is that both thread 1 and thread 17 have called pyuno_loader::CreateInstance at the same time, and that function
has no internal locking, so one thread sees
Py_IsInitialized() returning true despite the other thread not having
called PyEval_InitThreads().

so better stick some static mutex into pyuno_loader::CreateInstance().
Comment 14 Commit Notification 2018-02-01 19:14:27 UTC
Michael Stahl committed a patch related to this issue.
It has been pushed to "master":

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

tdf#114815 pyuno: avoid 2 threads initing python in parallel

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 15 Stephan Bergmann 2018-02-06 13:16:11 UTC
(In reply to Michael Stahl from comment #13)
> my theory is that both thread 1 and thread 17 have called
> pyuno_loader::CreateInstance at the same time, and that function
> has no internal locking, so one thread sees
> Py_IsInitialized() returning true despite the other thread not having
> called PyEval_InitThreads().

cppuhelper::ServiceManager::loadImplementation is indeed designed so that it can be invoked multiple times in parallel, and will go on to call createInstanceWithContext (and anything below that) in parallel, and only at the end of the function will throw away the data produced by any invocation that did not win the race.
Comment 16 Commit Notification 2018-02-06 13:17:26 UTC
Michael Stahl committed a patch related to this issue.
It has been pushed to "libreoffice-6-0":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=ed8a8c9a6ed377aaed3d62e17653a65e93eb3cc6&h=libreoffice-6-0

tdf#114815 pyuno: avoid 2 threads initing python in parallel

It will be available in 6.0.1.

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 17 Commit Notification 2018-02-06 13:48:55 UTC
Michael Stahl committed a patch related to this issue.
It has been pushed to "libreoffice-5-4":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=362b0c521c1c58dc8ea5e87ecbb482d5bdc073f4&h=libreoffice-5-4

tdf#114815 pyuno: avoid 2 threads initing python in parallel

It will be available in 5.4.6.

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 18 Stephan Bergmann 2018-02-08 07:20:00 UTC
*** Bug 115500 has been marked as a duplicate of this bug. ***
Comment 19 V Stuart Foote 2018-02-08 14:21:02 UTC
(In reply to Stephan Bergmann from comment #18)
> *** Bug 115500 has been marked as a duplicate of this bug. ***

With 2018-02-08 6.0 crashfix build [1] unable to reproduce.

=-ref-=
[1] https://dev-builds.libreoffice.org/daily/libreoffice-6-0/Win-x86_64@62-TDF/crashfix/

Version: 6.0.1.0.0+ (x64)
Build ID: 1a915a3f2906b005770de982c78bfccb21913273
CPU threads: 4; OS: Windows 10.0; UI render: GL; 
Locale: en-US (en_US); Calc: CL
Comment 20 V Stuart Foote 2018-02-08 14:28:37 UTC
*** Bug 115268 has been marked as a duplicate of this bug. ***
Comment 21 Commit Notification 2018-02-08 22:02:44 UTC
Michael Stahl committed a patch related to this issue.
It has been pushed to "libreoffice-5-4-5":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=42e2c4af310deb7660b07c6307459c9d2a42bb8b&h=libreoffice-5-4-5

tdf#114815 pyuno: avoid 2 threads initing python in parallel

It will be available in 5.4.5.

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 22 Michael Stahl (allotropia) 2018-02-09 15:02:29 UTC
reportedly it's fixed now.

because of that prematurely disclosed CVE, 5.4.5 is already released without the fix for this, wait until 5.4.6.

but somehow the problem caused a crash much less frequently in versions before 6.0 anyway.
Comment 23 V Stuart Foote 2018-02-11 18:54:32 UTC
*** Bug 115633 has been marked as a duplicate of this bug. ***
Comment 24 Aron Budea 2018-03-25 05:55:41 UTC
*** Bug 116614 has been marked as a duplicate of this bug. ***
Comment 25 Aron Budea 2018-03-25 05:57:33 UTC
Adding the other likely crash signature: LngSvcMgr::GetAvailableGrammarSvcs_Impl()