Bug 158783 - editing alphabetical index crashes Writer
Summary: editing alphabetical index crashes Writer
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
4.0.0.3 release
Hardware: x86-64 (AMD64) All
: medium critical
Assignee: Armin Le Grand (allotropia)
URL:
Whiteboard: target:24.8.0 target:24.2.1 target:24...
Keywords: bibisectRequest, regression
: 161085 (view as bug list)
Depends on:
Blocks: TableofContents-Indexes Crash
  Show dependency treegraph
 
Reported: 2023-12-19 17:29 UTC by 510jrb2301
Modified: 2024-05-15 23:57 UTC (History)
6 users (show)

See Also:
Crash report or crash signature: ["libc.so.6"]


Attachments
books length file on a mathematical subject. Indices follow sections. (139.43 KB, application/vnd.oasis.opendocument.text)
2023-12-19 17:35 UTC, 510jrb2301
Details
smaller sample file with coloured fields (60.30 KB, application/vnd.oasis.opendocument.text)
2024-01-02 20:51 UTC, Stéphane Guillou (stragu)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description 510jrb2301 2023-12-19 17:29:27 UTC
Description:
In writer I want to eliminate certain repeat items in an alphabetical index.  After deleting a few of the items the system crashes and all the previous work is lost.  The response is a Windows 10 panel which asks one to either allow the program to continue or close the file.  If allowed to continue, it will continue for hours without closing. Choosing to close the program loses all previous effort.
Perhaps related.  Some of the index occur in other indices, eg the Table of Contents.  The search function will find these items, they will be reported as alphabetical index items, but the edit reference feature will not allow entrance to them.
The index was created by finding all instances of an item and commanding then to be inserted.  As a consequence the index contains many more instances than necessray.


Steps to Reproduce:
Pleas tell me how to update the file to you
1.In the program go to pg 74
2.Scroll down to "compound variable"  A prompt will indicate is is in the alphabeitcal index.
3.edit entry
4. Delete the entry.  Writer may crash at this point.
5. If not find the next "compound variable entry listed in the alphabeetial index.  Delete that one.  The file may crashg.
6. Continue step 5.  The progrem will crash after a few iterations.


Actual Results:
The file crashes under Windows after one or more iterations.

Expected Results:
The selected items should have been removed from the alphaBetical Index without craching.


Reproducible: Sometimes


User Profile Reset: No

Additional Info:
[Information automatically included from LibreOffice]
Locale: en-US
Module: TextDocument
[Information guessed from browser]
OS: Windows (All)
OS is 64bit: no
Comment 1 510jrb2301 2023-12-19 17:35:17 UTC
Created attachment 191509 [details]
books length file on a mathematical subject.  Indices follow sections.
Comment 2 Stéphane Guillou (stragu) 2024-01-02 20:51:09 UTC
Created attachment 191713 [details]
smaller sample file with coloured fields

Thank you for the report!

Reproduced various issues.

A) In 4.0.0.3, all the way to:

Version: 7.6.4.1 (X86_64) / LibreOffice Community
Build ID: e19e193f88cd6c0525a17fb7a176ed8e6a3e2aa1
CPU threads: 8; OS: Linux 5.15; UI render: default; VCL: gtk3
Locale: en-AU (en_AU.UTF-8); UI: en-US
Calc: threaded

With steps:

1. Open this attachment
2. Double-click on the yellow "functions": "Edit Index Entry" dialog opens (before LO 7.5, you need to place the cursor in front of the index, then use Edit > Reference > Index entry)
3. Click Delete twice
4. Close dialog

Result: crash (or freeze) -> https://crashreport.libreoffice.org/stats/crash_details/1cb92bc4-a72c-45ea-99ae-eb3488292090

In console:

terminate called after throwing an instance of 'std::out_of_range'
  what():  basic_string_view::substr: __pos (which is 50) > __size (which is 35)

Not reproduced in OOo 3.3 -> regression.

However, this is can't be reproduced in 24.2 as the dialog now closes on the first delete, since Armin's ab7c81f55621d7b0d1468c63305163016dd78837 (build commit [1b7cb4eeeef9b131220865ad098d3c8e1bc53cdb] in linux-64-24.2 bibisect repo).

------

B) Since Armin's ab7c81f55621d7b0d1468c63305163016dd78837, and still in a recent trunk build:

Version: 24.8.0.0.alpha0+ (X86_64) / LibreOffice Community
Build ID: e71934471442a8bbba7e661d3ebe5f708627c5d6
CPU threads: 8; OS: Linux 5.15; UI render: default; VCL: gtk3
Locale: en-AU (en_AU.UTF-8); UI: en-US
Calc: threaded

With steps:

1. Open this attachment
2. Double-click on the red "functions": "Edit Index Entry" dialog opens
3. Click Delete (or close the dialog)

Alternatively:

1. Open attachment 191509 [details]
2. Go to page 78
3. Double-click on "functions" in the first sentence: "Edit Index Entry" dialog opens
4. Close the dialog

Result: freeze
Comment 3 Stéphane Guillou (stragu) 2024-01-02 20:55:14 UTC
Armin, can you please have a look at the dialog not being able to move to the next entry automatically (A), and the regression freeze in (B)?

Then we could have a look at older crash (which might be bibisectable).
Comment 4 Armin Le Grand (allotropia) 2024-01-11 13:26:40 UTC
Taking a look...
Comment 5 Armin Le Grand (allotropia) 2024-01-11 13:33:46 UTC
Tried (A) and indeed Dlg closes on 1st 'Delete' and no crash. This smells like SfxPoolItem lifetime, so it is viable that my last changes might already have eliminated this. The mentioned ab7c81f55621d7b0d1468c63305163016dd78837 is only one of these.
Thus: *IS* there a problem with (A) still or not? I do not know that functionalities, for me it looks good (?)
Comment 6 Armin Le Grand (allotropia) 2024-01-11 13:52:20 UTC
I get a crash in SwScanner::SwScanner due to an illegal index access to the string m_aPreDashReplacementText which contains
   "\tf(r(x1,a)) a generalized function;" -> length (36) AFAICC
The loop there
        for (sal_Int32 i = m_nStartPos; i < m_nEndPos; ++i)
with m_nStartPos==34 and m_nEndPos==42. Thus the access
            sal_Unicode cChar = aBuf[i];
with I==36 FAILS. What is clear.

Question is why m_nStartPos/nEndPos values like these are handed in and if that was different before. It clearly is an error.

I could just ensure no accesses after string length, but that might not solve the problem, I just do not know that functionality.

I also get (VERY often) the warning
  info:svl.items:567770:567770:svl/source/items/poolitem.cxx:633: ITEM: PtrCompare != ContentCompare (!)
as SAL_INFO from areSfxPoolItemPtrsEqual. It means that two SfxPoolItems get compared whereby the *content* compare is equal, but the pointers differ. This is a hint that evtl. content comapre and ItemRe-use should be used at some place. That warning is from the mentioned change and may be relevant.
Comment 7 Armin Le Grand (allotropia) 2024-01-11 14:34:37 UTC
The SAL_INFO comes from areSfxPoolItemPtrsEqual in SwDoc::DeleteTOXMark. Here the content compare of two SwTOXMark is equal, but SwTOXMark::operator== just compares the *type* and ignores all the content, so this is a false positive. Is that SwTOXMark::operator== sufficient...? SwTOXMark *is* derived from SfxPoolItem, so part of the ItemPool/PoolItem concept, this seems vague at least...?

Also it seems not to happen every time, hunting where that string and the wrong indices might come from...
Comment 8 Armin Le Grand (allotropia) 2024-01-11 14:46:34 UTC
Not every time, but if it comes from
    SwDoc::CountWords
last line. pTNd contains the text, and the indices come from
    const sal_Int32 nSttCnt = pStt->GetContentIndex();
    const sal_Int32 nEndCnt = pEnd->GetContentIndex();
and seem to be wrong already, at least do not fit the text.
Added Michael Stahl to cc, maybe he knows why this can be wrong (?)

I can do a 'fix' by making sure that the mentioned loop in comment 6 does avoid invalid indices, but would that be correct?
Comment 9 Armin Le Grand (allotropia) 2024-01-11 14:56:40 UTC
I replaced
        for (sal_Int32 i = m_nStartPos; i < m_nEndPos; ++i)
with
        for (sal_Int32 i = std::min(aBuf.getLength(), m_nStartPos); i < std::min(aBuf.getLength(), m_nEndPos); ++i)
and that solves the direct problem. But I get another problem on the same string: Also from SwDoc::CountWords, same string, but the pTNd->CountWords call in line 859,
            pTNd->CountWords( rStat, 0, nEndCnt );
thus crashes in SwBreakIt::getGraphemeCount due to nEnd==42 being out of bounds for that string.
Thus somehow my Item changes have led to more possibly wrong indices in texts...?
Comment 10 Armin Le Grand (allotropia) 2024-01-11 15:09:37 UTC
@mst: In SwTextNode::CountWords *somehow* after mentioned change nExpandEnd can be bigger that aExpandText.getLength(), so called methods
    SwScanner aScanner(...)
and
    getGraphemeCount(...)
that use aExpandText and nExpandEnd trigger out of bound accesses and can lead to crash. nExpandBegin may also be wrong.
I have no idea how my item changes (ab7c81f55621d7b0d1468c63305163016dd78837) may lead to this, would it be OK to change
    const sal_Int32 nExpandBegin = aConversionMap.ConvertToViewPosition( nStt );
    const sal_Int32 nExpandEnd   = aConversionMap.ConvertToViewPosition( nEnd );
to
    const sal_Int32 nExpandBegin = std::min(aExpandText.getLength(), aConversionMap.ConvertToViewPosition( nStt ));
    const sal_Int32 nExpandEnd   = std::min(aExpandText.getLength(), aConversionMap.ConvertToViewPosition( nEnd ));
what would make the calls safe, but without knowing why this happens. I have probably not enough SW knowledge to identify the real problem (?)
Comment 11 Michael Stahl (allotropia) 2024-01-11 17:19:59 UTC
(In reply to Armin Le Grand from comment #7)
> The SAL_INFO comes from areSfxPoolItemPtrsEqual in SwDoc::DeleteTOXMark.
> Here the content compare of two SwTOXMark is equal, but
> SwTOXMark::operator== just compares the *type* and ignores all the content,
> so this is a false positive. Is that SwTOXMark::operator== sufficient...?
> SwTOXMark *is* derived from SfxPoolItem, so part of the ItemPool/PoolItem
> concept, this seems vague at least...?

possibly the SwTOXMark was not POOLABLE and thus the operator== is unused in practice?
Comment 12 Armin Le Grand (allotropia) 2024-01-11 18:29:45 UTC
Discussed with mst (thanks a lot) and indeed SwTOXMark as Item was never pooled, so op== was not really ever used. I discussed to implement it (there is quite some content, currently only type is used), but we came to the point that it's only safe to say instances are equal when same instance -> fallback to ptr compare. Also corrected places where it is known that it's SwTOXMark* from SfxPoolItem::areSame (which would use op==) to areSfxPoolItemPtrsEqual (which is just ptr compare).
Also SwTOXMark::operator== uses ptr compare now. All in all that would be enough, but is more expressive/clear that way. Also there might be compares of item* where it is not clear that SwTOXMark is used.
Making some more checks...
Comment 13 Michael Stahl (allotropia) 2024-01-11 18:36:39 UTC
various crashes happen because SwIndexMarkPane::InitControls() calls GotoTOXMark and then checks if it went to a different mark and if so calls GotoTOXMark again to move back.

problem is that now all TOXMarks compare equal so it doesn't move back and the cursor point remains on a node that's different from the node that contains the SwTOXMark and then SwCursorShell::SelectTextAttr() creates a very invalid cursor position.
Comment 14 Commit Notification 2024-01-11 19:34:13 UTC
Michael Stahl committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/36f232e27616f267577851c38a24c2a5c3caad90

tdf#158783 sw: add some asserts for calling SelectTextAttr() ...

It will be available in 24.8.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 15 Armin Le Grand (allotropia) 2024-01-12 10:27:49 UTC
Fix at https://gerrit.libreoffice.org/c/core/+/161963
Comment 16 Armin Le Grand (allotropia) 2024-01-12 11:13:04 UTC
Still have a problem when doing (B) from comment 2, but using right arrow -> at some time (turnaround to 1st candidate?) I stll get a hang/crash, so not yet done.
Comment 17 Commit Notification 2024-01-12 14:14:04 UTC
Michael Stahl committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/3fefff7ce29a234694343105c50c7bc3259a9cb8

tdf#158783 sw: SwCursorShell::GotoTOXMark() must actually move cursor

It will be available in 24.8.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 18 Commit Notification 2024-01-12 14:23:08 UTC
Armin Le Grand (allotropia) committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/d22a86089edfcadbef5231525a2947b954f4784e

tdf#158783 Correct compares of SwTOXMark Items

It will be available in 24.8.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 19 Armin Le Grand (allotropia) 2024-01-15 17:59:08 UTC
Should be good from what I see.
@Stéphane: Thanks for that well written report!
Comment 20 Stéphane Guillou (stragu) 2024-01-19 09:56:59 UTC
(In reply to Armin Le Grand from comment #19)
> Should be good from what I see.
> @Stéphane: Thanks for that well written report!

Thank you Armin and Michael for looking into it!

Fix verified for all comment 2 steps in:

Version: 24.8.0.0.alpha0+ (X86_64) / LibreOffice Community
Build ID: 8b393bba91111bd4f8988457f3a78b0306462bf2
CPU threads: 8; OS: Linux 5.15; UI render: default; VCL: gtk3
Locale: en-AU (en_AU.UTF-8); UI: en-US
Calc: threaded

No freeze, no crash, and dialog moves to the next item automatically.
Comment 21 Commit Notification 2024-01-20 16:39:34 UTC
Michael Stahl committed a patch related to this issue.
It has been pushed to "libreoffice-24-2":

https://git.libreoffice.org/core/commit/908abf0b0e36fd1c6e8d3df48e9c35ce911c9bda

tdf#158783 sw: SwCursorShell::GotoTOXMark() must actually move cursor

It will be available in 24.2.1.

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 22 Commit Notification 2024-03-19 21:22:08 UTC
Armin Le Grand (allotropia) committed a patch related to this issue.
It has been pushed to "libreoffice-24-2":

https://git.libreoffice.org/core/commit/a48ee46a085abfa04779ece38c08dddb5bf017ea

tdf#158783 Correct compares of SwTOXMark Items

It will be available in 24.2.3.

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 23 Commit Notification 2024-03-22 18:37:57 UTC
Armin Le Grand (allotropia) committed a patch related to this issue.
It has been pushed to "libreoffice-24-2-2":

https://git.libreoffice.org/core/commit/c5b878a7c55f7e6f8d3a66fa9728c8fca5d0e13b

tdf#158783 Correct compares of SwTOXMark Items

It will be available in 24.2.2.

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 24 Stéphane Guillou (stragu) 2024-05-15 23:57:40 UTC
*** Bug 161085 has been marked as a duplicate of this bug. ***