Bug Hunting Session
Bug 117444 - Crash in: BigPtrArray::Index2Block(unsigned __int64)
Summary: Crash in: BigPtrArray::Index2Block(unsigned __int64)
Status: RESOLVED WORKSFORME
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: x86-64 (AMD64) Windows (All)
: high major
Assignee: Not Assigned
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: Track-Changes
  Show dependency treegraph
 
Reported: 2018-05-05 18:43 UTC by Dan Carr
Modified: 2019-06-03 17:01 UTC (History)
6 users (show)

See Also:
Crash report or crash signature: ["BigPtrArray::Index2Block(unsigned __int64)"]


Attachments
Primary Writer file in compare (35.15 KB, application/vnd.oasis.opendocument.text)
2018-05-09 00:27 UTC, Dan Carr
Details
secondary file in compare (34.56 KB, application/vnd.oasis.opendocument.text)
2018-05-09 00:28 UTC, Dan Carr
Details
Example file (35.20 KB, application/vnd.oasis.opendocument.text)
2019-06-02 18:03 UTC, Telesto
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dan Carr 2018-05-05 18:43:22 UTC
Viewing 
This bug was filed from the crash reporting server and is br-0d487f9e-5075-4371-b776-652da4665208.

This may be a duplicate of 98202 - Comparing two documents.
I was comparing two versions of a document. 
In this instance of the crash, LO writer brought up the list of differences. I realized the two documents had a long list of differences because of formatting changes that had occurred and using compare would not be a reasonable technique for me to use. I closed the window showing the differences. The pop up window closed and was followed by LO Writer crashing. 
=========================================
Comment 1 fiftyigfuci_f_mi 2018-05-07 02:07:05 UTC
It looks like the binary search algorithm in Index2Block has a bug:
If the value of "m_nBlock" is 1, then "n" is 0 and "cur" becomes 1,
thus "m_ppInf[ cur ]" is semantically out of the range.

    // binary search: always successful
    sal_uInt16 lower = 0, upper = m_nBlock - 1;
    sal_uInt16 cur = 0;
    for(;;)
    {
        sal_uInt16 n = lower + ( upper - lower ) / 2;
        cur = ( n == cur ) ? n+1 : n;
        p = m_ppInf[ cur ];
Comment 2 Xisco Faulí 2018-05-07 09:21:14 UTC
@Dan, Thanks for reporting the issue.
Could you please attach the two documents you're comparing?

@fiftyigfuci, thanks for the analysis.
Would you mind submitting a patch to gerrit for review? -> https://wiki.documentfoundation.org/Development/gerrit
Comment 3 Xisco Faulí 2018-05-08 10:13:02 UTC
Setting to NEEDINFO until the document has been provided...
Comment 4 Dan Carr 2018-05-09 00:27:15 UTC
Created attachment 141990 [details]
Primary Writer file in compare
Comment 5 Dan Carr 2018-05-09 00:28:44 UTC
Created attachment 141991 [details]
secondary file in compare
Comment 6 Xisco Faulí 2018-05-09 08:07:52 UTC
Thanks for attaching the document.
So you open the first document, Go to Edit - Track Changes - Compare Documents and it crashes all the time ?
Comment 7 Xisco Faulí 2018-05-09 08:11:03 UTC
(In reply to fiftyigfuci_f_mi from comment #1)
> It looks like the binary search algorithm in Index2Block has a bug:
> If the value of "m_nBlock" is 1, then "n" is 0 and "cur" becomes 1,
> thus "m_ppInf[ cur ]" is semantically out of the range.
> 
>     // binary search: always successful
>     sal_uInt16 lower = 0, upper = m_nBlock - 1;
>     sal_uInt16 cur = 0;
>     for(;;)
>     {
>         sal_uInt16 n = lower + ( upper - lower ) / 2;
>         cur = ( n == cur ) ? n+1 : n;
>         p = m_ppInf[ cur ];

@Noel, @Stephan, I thought you might be interested in this comment...
Comment 8 Noel Grandin 2018-05-09 15:09:46 UTC
Good spotting!

If m_nBlock is 1, then m_nCur must be 0, and we should use the very first block of logic in that function, and exit early.

Unless some higher level code is asking for a position that does not exist, in which case we would fall through to the binary search, and trigger an access violation by dereferencing a null pointer.

So the real bug is somewhere higher up the call stack.
Comment 9 Xisco Faulí 2018-05-31 11:54:15 UTC
Putting this to NEW and increasing severity...
Comment 10 Dan Carr 2018-06-01 20:38:41 UTC
Late response to Comment 6 -
Does it happen every time?

It happened twice in succession and I have not tried repeating the activity until today (Jun 1, 2018)  and it did not occur. 
In the interim period I have updated Windows 7 with a major update and now it does not occur.
Comment 11 Telesto 2018-07-16 17:09:07 UTC
No repro with
Version: 6.2.0.0.alpha0+
Build ID: e7d3976cb80f7e7401be071f905a764dd6cb4d6e
CPU threads: 4; OS: Windows 6.3; UI render: default; 
TinderBox: Win-x86@42, Branch:master, Time: 2018-06-29_04:46:32
Locale: nl-NL (nl_NL); Calc: CL
Comment 12 Xisco Faulí 2018-09-06 13:55:23 UTC
(In reply to Noel Grandin from comment #8)
> Good spotting!
> 
> If m_nBlock is 1, then m_nCur must be 0, and we should use the very first
> block of logic in that function, and exit early.
> 
> Unless some higher level code is asking for a position that does not exist,
> in which case we would fall through to the binary search, and trigger an
> access violation by dereferencing a null pointer.
> 
> So the real bug is somewhere higher up the call stack.

Hi Noel,
This crash is among the top 10 in 6.1.0.3.
Do you think you could take a look at the problem described in your comment above at some point ?
Comment 13 Telesto 2018-09-06 17:06:38 UTC
I'm not able to reproduce this
Version: 6.2.0.0.alpha0+
Build ID: 76bf3939b0583212a56c317c85aea110f8ac6fee
CPU threads: 4; OS: Mac OS X 10.12.6; UI render: default; 
TinderBox: MacOSX-x86_64@49-TDF, Branch:master, Time: 2018-07-27_06:01:47
Locale: nl-NL (nl_NL.UTF-8); Calc: group threaded
Comment 14 Telesto 2019-06-02 18:03:32 UTC
Created attachment 151856 [details]
Example file

I'm able to produce a crash in swlo!SwRedlineData::SetSeqNo (not sure if this is the same crash reported here ..)

Anyway
1. Open the attached file 
2. Download attachment 141990 [details]
3. Edit -> Track changes -> Compare documents & select attachment 141990 [details]
4. Accept all in the dialog & close
5. Repeat step 3/4
6. Press CTRL+Z -> Crash
Comment 15 Telesto 2019-06-03 12:41:47 UTC
@Dieter/Xisco
Are you able to confirm comment 14?
Comment 16 Dan Carr 2019-06-03 15:34:20 UTC
I have updated my version from 5.4.5 to 6.0.5 and cannot recreate the problem.
Comment 17 Dan Carr 2019-06-03 15:38:06 UTC
(In reply to Dan Carr from comment #16)
> I have updated my version from 5.4.5 to 6.0.5 and cannot recreate the
> problem.

That is when using the original process of opening the primary document, selecting compare document, and then closing the comparison list pop up window.  This did not cause LO Writer to creash.
Comment 18 Telesto 2019-06-03 16:10:06 UTC
(In reply to Dan Carr from comment #17)
> (In reply to Dan Carr from comment #16)
> > I have updated my version from 5.4.5 to 6.0.5 and cannot recreate the
> > problem.
> 
> That is when using the original process of opening the primary document,
> selecting compare document, and then closing the comparison list pop up
> window.  This did not cause LO Writer to creash.

@Dan Carr
Thanks for testing.. Moving my crash to a new report (bug 125660). Marking this one RESOLVED WORKSFORME
Comment 19 Xisco Faulí 2019-06-03 17:01:22 UTC
I guess this issue got fixed by the same commits fixing bug 109376