Bug 147731 - Crash in SwFrameFormat::~SwFrameFormat()
Summary: Crash in SwFrameFormat::~SwFrameFormat()
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
7.2.0.0.alpha0+
Hardware: All All
: medium critical
Assignee: Attila Bakos (NISZ)
URL:
Whiteboard: target:24.8.0 target:24.2.2 target:7.6.6
Keywords: bibisected, bisected, haveBacktrace, regression
: 150441 (view as bug list)
Depends on:
Blocks: Cut-Copy Crash
  Show dependency treegraph
 
Reported: 2022-03-02 10:39 UTC by Xisco Faulí
Modified: 2024-02-27 13:44 UTC (History)
9 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. ***
Comment 16 Stéphane Guillou (stragu) 2023-10-22 20:19:42 UTC
Still crashing with comment 0 or comment 9 steps in:

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

Attila, are you still working on it or should it be unassigned?
Comment 17 Michael Stahl (allotropia) 2024-02-23 16:43:57 UTC
the problem was fixed by quikee.
the commit introduced a memory leak though, which i have a fix for in gerrit.

commit 963de9feb37105560fde14b44d992e47f341bb5b
Author:     Tomaž Vajngerl <tomaz.vajngerl@collabora.co.uk>
AuthorDate: Thu Nov 30 16:42:26 2023 +0900
Commit:     Tomaž Vajngerl <quikee@gmail.com>
CommitDate: Fri Dec 1 05:07:42 2023 +0100

    sw: fix issue with copying stashed frame format
    
    When the PageDesc is copied from one document to another, we
    don't make sure the stashed FrameFormat(s) are also properly
    copied to the new document (which can happen at copy/paste). This
    can cause a crash if the stashed FrameFormats are accessed or
    destructed after the original document is destroyed.
    
    This fixes the issue so that when we detect the PageDesc belong
    to different documents, the stashed FrameFormats are copied just
    like the non-stashed FrameFormats (used for headers and footers).
    
    Change-Id: I948068dba4d39bb47c3725dfa8491c53c5833c7e
    Reviewed-on: https://gerrit.libreoffice.org/c/core/+/160065
    Tested-by: Jenkins
    Reviewed-by: Tomaž Vajngerl <quikee@gmail.com>
Comment 18 Commit Notification 2024-02-24 08:45:01 UTC
Michael Stahl committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/5c4ae1b19c51dcd62dad8e1d3e8beb87a0311352

tdf#147731 sw: fix memory leak in SwDoc::CopyPageDesc()

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 Stéphane Guillou (stragu) 2024-02-27 07:07:18 UTC
No crash with:

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

Thank you!
Comment 20 Commit Notification 2024-02-27 08:49:09 UTC
Michael Stahl committed a patch related to this issue.
It has been pushed to "libreoffice-24-2":

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

tdf#147731 sw: fix memory leak in SwDoc::CopyPageDesc()

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 21 Commit Notification 2024-02-27 13:44:10 UTC
Michael Stahl committed a patch related to this issue.
It has been pushed to "libreoffice-7-6":

https://git.libreoffice.org/core/commit/737f8ce51d47e030b4cd6d955ef24cfb71c185c2

tdf#147731 sw: fix memory leak in SwDoc::CopyPageDesc()

It will be available in 7.6.6.

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.