Bug 147731 - Crash in SwFrameFormat::~SwFrameFormat()
Summary: Crash in SwFrameFormat::~SwFrameFormat()
Status: ASSIGNED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
7.2.0.0.alpha0+
Hardware: All All
: medium normal
Assignee: Attila Bakos (NISZ)
URL:
Whiteboard:
Keywords: bibisected, bisected, haveBacktrace, regression
: 150441 (view as bug list)
Depends on:
Blocks: Crash
  Show dependency treegraph
 
Reported: 2022-03-02 10:39 UTC by Xisco Faulí
Modified: 2022-12-15 13:49 UTC (History)
7 users (show)

See Also:
Crash report or crash signature: ["SwFrameFormat::~SwFrameFormat()"]


Attachments
bt with debug symbols (9.78 KB, text/plain)
2022-03-02 19:50 UTC, Julien Nabet
Details
valgrind trace (118.86 KB, text/x-log)
2022-03-02 21:41 UTC, Julien Nabet
Details
Minimised version (37.39 KB, application/vnd.openxmlformats-officedocument.wordprocessingml.document)
2022-03-08 11:52 UTC, Attila Bakos (NISZ)
Details
The reason of the crash (268.74 KB, image/jpeg)
2022-03-08 13:24 UTC, Attila Bakos (NISZ)
Details
The original docx without password (2.13 MB, application/vnd.openxmlformats-officedocument.wordprocessingml.document)
2022-06-09 13:32 UTC, Attila Bakos (NISZ)
Details
ASAN log (17.48 KB, text/plain)
2022-08-11 10:18 UTC, Michael Stahl (allotropia)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Xisco Faulí 2022-03-02 10:39:01 UTC
Steps to reproduce:
1. Open attachment 99325 [details] from bug 78902
2. Select all
3. Copy
4. Close the document
5. Open it again
6. Select all
7. Copy

-> Crash

Reproduced in

Version: 7.4.0.0.alpha0+ / LibreOffice Community
Build ID: c97a3592c78ce276a353f95ce68c70a8a39174a0
CPU threads: 8; OS: Linux 5.10; UI render: default; VCL: gtk3
Locale: es-ES (es_ES.UTF-8); UI: en-US
Calc: threaded
Comment 1 Xisco Faulí 2022-03-02 10:40:57 UTC
Regression introduced by:

https://cgit.freedesktop.org/libreoffice/core/commit/?id=b802ab694a8a7357d4657f3e11b571144fa7c7bf

author	Attila Bakos (NISZ) <bakos.attilakaroly@nisz.hu>	2021-03-12 14:33:08 +0100
committer	László Németh <nemeth@numbertext.org>	2021-03-31 14:44:27 +0200
commit b802ab694a8a7357d4657f3e11b571144fa7c7bf (patch)
tree 1c8aba77e9b6aed048d8b49f55ff006c5d099d5f
parent 3c49a3be592eb515bba100b4a17617136fabbcba (diff)
tdf#141158 DOCX: import discarded headers/footers

Bisected with: bibisect-linux64-7.2

Adding Cc: to Attila Bakos
Comment 2 Julien Nabet 2022-03-02 19:50:57 UTC
Created attachment 178622 [details]
bt with debug symbols

On pc Debian x86-64 with master sources updated today, I could reproduce this.
Comment 3 Julien Nabet 2022-03-02 21:41:05 UTC
Created attachment 178625 [details]
valgrind trace

I also noticed that when reopening the file and did "Select All", there was a kind of picture of the content of the file which partly displays at left, a kind of "ghost" or shadow of the document itself.
Comment 4 Attila Bakos (NISZ) 2022-03-07 15:49:17 UTC Comment hidden (obsolete)
Comment 5 Attila Bakos (NISZ) 2022-03-08 11:52:18 UTC
Created attachment 178720 [details]
Minimised version
Comment 6 Attila Bakos (NISZ) 2022-03-08 13:24:58 UTC Comment hidden (obsolete)
Comment 7 Attila Bakos (NISZ) 2022-03-08 15:11:44 UTC Comment hidden (obsolete)
Comment 8 Attila Bakos (NISZ) 2022-06-09 13:32:50 UTC
Created attachment 180653 [details]
The original docx without password

Hi there!

Forget about my last comments.
I have tried to minimize the bugdoc with word, but it complained because that was protected with password. After removing the password from the xml i cannot repro the crash with:

Version: 7.4.0.0.alpha1+ (x64) / LibreOffice Community
Build ID: 19632bf0bc0561138f3dee4b2d79a5b3ade0de52
CPU threads: 8; OS: Windows 10.0 Build 19043; UI render: Skia/Raster; VCL: win
Locale: hu-HU (hu_HU); UI: en-US
Calc: CL

I think this issue depends on password protection. How to test it without password? Any idea? Thx.
Comment 9 Michael Stahl (allotropia) 2022-08-11 10:16:39 UTC
i found the same UAF yesterday.

1. open https://bugs.documentfoundation.org/attachment.cgi?id=180723
2. select all
3. copy
4. close
5. do something that clears the clipboard, e.g. open new document and copy some text

typically it crashes in step 5, but the problem already happens at step 4.

the main SwDoc is destroyed, but the clipboard SwDoc has pointers into the main SwDoc's item pool, which is of course not supposed to happen.

the problem happens in this code:

│     1545      // Copy the stashed formats as well between the page descriptors...
│     1546      for (bool bFirst : { true, false })                                                                   
│     1547          for (bool bLeft : { true, false }) 
│     1548              for (bool bHeader : { true, false }) 
│     1549              { 
│     1550                  if (!bLeft && !bFirst) 
│     1551                      continue; 
│     1552                  if (auto pStashedFormat = rSrcDesc.GetStashedFrameFormat(bHeader, bLeft, bFirst)) 
│  >  1553                      rDstDesc.StashFrameFormat(*pStashedFormat, bHeader, bLeft, bFirst);
│     1554              }

this works if rDstDesc and rSrcDesc are in the same SwDoc, but if different SwDoc all the items inside the SwFrameFormats must be cloned into the destination doc's item pool - but this just calls SwFrameFormat copy ctor so they're in the source SwDoc.

see how SwDoc::CopyPageDesc calls special functions CopyHeader/CopyFooter.
Comment 10 Michael Stahl (allotropia) 2022-08-11 10:18:28 UTC
Created attachment 181725 [details]
ASAN log
Comment 11 Michael Stahl (allotropia) 2022-08-11 10:26:16 UTC
note: UAF on close is because SwFormatHeader item owns SfxItemSet (via its GetRegisteredIn() SwFrameFormat)

there aren't supposed to be any SwFormatHeader items alive at the time when the SfxItemPool is destroyed - all must be destroyed earlier when all SwPageDescs are destroyed in ~SwDoc.

they're not because the other SwDoc increased their refcounts.
Comment 12 Attila Bakos (NISZ) 2022-08-15 07:41:58 UTC
Hello Michael,

sorry of my late reply. I did not write a lot here since my last comment. Thank you for the advice, yes i also had the same idea according to the backtrace, and i have started to reimplement this in https://gerrit.libreoffice.org/c/core/+/136959 which is currently waits for the finish about a month, because of i do not had time to do that. In addition, there were also problems, but that were a month before so unfortunately i not remember well what was that, maybe crash not remember well, so be careful if you interested in that change, not merge yet, and the local version is different from that in the gerrit (with different problem :)). That had been interrupted, but when i will have time, i want to finish it. However, if i remember well i did not think of that what you wrote, this issue is occurs when there are different swdocs, but as i remember that is true, so thanks for showing me that. Hopefully i can finish it soon.
Comment 13 Attila Bakos (NISZ) 2022-08-15 07:44:47 UTC
(In reply to Michael Stahl (allotropia) from comment #9)
> i found the same UAF yesterday.
> 
> 1. open https://bugs.documentfoundation.org/attachment.cgi?id=180723
> 2. select all
> 3. copy
> 4. close
> 5. do something that clears the clipboard, e.g. open new document and copy
> some text
> 
> typically it crashes in step 5, but the problem already happens at step 4.
> 
> the main SwDoc is destroyed, but the clipboard SwDoc has pointers into the
> main SwDoc's item pool, which is of course not supposed to happen.
> 
> the problem happens in this code:
> 
> │     1545      // Copy the stashed formats as well between the page
> descriptors...
> │     1546      for (bool bFirst : { true, false })                         
> 
> │     1547          for (bool bLeft : { true, false }) 
> │     1548              for (bool bHeader : { true, false }) 
> │     1549              { 
> │     1550                  if (!bLeft && !bFirst) 
> │     1551                      continue; 
> │     1552                  if (auto pStashedFormat =
> rSrcDesc.GetStashedFrameFormat(bHeader, bLeft, bFirst)) 
> │  >  1553                      rDstDesc.StashFrameFormat(*pStashedFormat,
> bHeader, bLeft, bFirst);
> │     1554              }
> 
> this works if rDstDesc and rSrcDesc are in the same SwDoc, but if different
> SwDoc all the items inside the SwFrameFormats must be cloned into the
> destination doc's item pool - but this just calls SwFrameFormat copy ctor so
> they're in the source SwDoc.
> 
> see how SwDoc::CopyPageDesc calls special functions CopyHeader/CopyFooter.

Agree, this is not enough for copying, there must be a deep copy. :)
Comment 14 Attila Bakos (NISZ) 2022-08-15 07:47:53 UTC
(In reply to Michael Stahl (allotropia) from comment #11)
> note: UAF on close is because SwFormatHeader item owns SfxItemSet (via its
> GetRegisteredIn() SwFrameFormat)
> 
> there aren't supposed to be any SwFormatHeader items alive at the time when
> the SfxItemPool is destroyed - all must be destroyed earlier when all
> SwPageDescs are destroyed in ~SwDoc.
> 
> they're not because the other SwDoc increased their refcounts.

Yep, i have seen in the assert the ref count problem, but i did not know the other doc keeps that alive, so thank you for pointing that me. (I had not idea who still owns the item :))
Comment 15 Michael Stahl (allotropia) 2022-08-16 15:02:16 UTC
*** Bug 150441 has been marked as a duplicate of this bug. ***