Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cool#11002 kit: fix memory corruption in Document::handleSaveMessage() #11007

Merged
merged 2 commits into from
Jan 24, 2025
Merged

cool#11002 kit: fix memory corruption in Document::handleSaveMessage() #11007

merged 2 commits into from
Jan 24, 2025

Conversation

vmiklos
Copy link
Contributor

@vmiklos vmiklos commented Jan 24, 2025

  • wsd: fix clang-15 build
  • cool#11002 kit: fix memory corruption in Document::handleSaveMessage()

@vmiklos vmiklos changed the title private/vmiklos/master cool#11002 kit: fix memory corruption in Document::handleSaveMessage() Jan 24, 2025
@vmiklos vmiklos mentioned this pull request Jan 24, 2025
Closed
@vmiklos vmiklos requested a review from caolanm January 24, 2025 10:08
@vmiklos
Copy link
Contributor Author

vmiklos commented Jan 24, 2025 •
edited
Loading

@caolanm could you please review this? Thanks.

We had the same pattern in core unit tests in the past: given that "on idle" jobs may send LOK callbacks, it's useful to unregister the callback function before we delete the COOL side. I wasn't entirely happy with the 4x sfx2 band-aid fixes, I think this is finally fixing a root cause.

I promise this is my last review request for today. :-)

caolanm
caolanm reviewed Jan 24, 2025
View reviewed changes
wsd/CacheUtil.hpp Outdated
@@ -23,6 +23,13 @@ struct CacheQuery
std::string _uri;
std::string _stamp;
std::string _dest;

CacheQuery(const std::string& uri, const std::string stamp, const std::string dest)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be better to be all "const std::string&" here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, let me fix. I just happened to have clang-15 here and after pulling, I ran into this where old enough compilers need an explicit ctor for some reason.

caolanm
caolanm approved these changes Jan 24, 2025
View reviewed changes
Copy link
Contributor

@caolanm caolanm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks reasonable to me.

@vmiklos
wsd: fix clang-15 build
d87de5e 
In file included from wsd/DocumentBroker.cpp:14:
In file included from ./wsd/DocumentBroker.hpp:14:
In file included from /usr/bin/../lib64/gcc/x86_64-suse-linux/12/../../../../include/c++/12/atomic:41:
In file included from /usr/bin/../lib64/gcc/x86_64-suse-linux/12/../../../../include/c++/12/bits/atomic_base.h:41:
In file included from /usr/bin/../lib64/gcc/x86_64-suse-linux/12/../../../../include/c++/12/bits/atomic_wait.h:49:
In file included from /usr/bin/../lib64/gcc/x86_64-suse-linux/12/../../../../include/c++/12/bits/std_mutex.h:39:
In file included from /usr/bin/../lib64/gcc/x86_64-suse-linux/12/../../../../include/c++/12/system_error:41:
In file included from /usr/bin/../lib64/gcc/x86_64-suse-linux/12/../../../../include/c++/12/stdexcept:39:
In file included from /usr/bin/../lib64/gcc/x86_64-suse-linux/12/../../../../include/c++/12/string:53:
In file included from /usr/bin/../lib64/gcc/x86_64-suse-linux/12/../../../../include/c++/12/bits/basic_string.h:39:
In file included from /usr/bin/../lib64/gcc/x86_64-suse-linux/12/../../../../include/c++/12/ext/alloc_traits.h:34:
/usr/bin/../lib64/gcc/x86_64-suse-linux/12/../../../../include/c++/12/bits/alloc_traits.h:518:4: error: no matching function for call to 'construct_at'
          std::construct_at(__p, std::forward<_Args>(__args)...);
          ^~~~~~~~~~~~~~~~~
/usr/bin/../lib64/gcc/x86_64-suse-linux/12/../../../../include/c++/12/bits/vector.tcc:117:21: note: in instantiation of function template specialization 'std::allocator_traits<std::allocator<CacheQuery>>::construct<CacheQuery, const std::basic_string<char> &, const std::basic_string<char> &, std::basic_string<char> &>' requested here
            _Alloc_traits::construct(this->_M_impl, this->_M_impl._M_finish,
                           ^
wsd/DocumentBroker.cpp:1576:21: note: in instantiation of function template specialization 'std::vector<CacheQuery>::emplace_back<const std::basic_string<char> &, const std::basic_string<char> &, std::basic_string<char> &>' requested here
            queries.emplace_back(uri, stamp, fileName);
                    ^
/usr/bin/../lib64/gcc/x86_64-suse-linux/12/../../../../include/c++/12/bits/stl_construct.h:94:5: note: candidate template ignored: substitution failure [with _Tp = CacheQuery, _Args = <const std::basic_string<char> &, const std::basic_string<char> &, std::basic_string<char> &>]: no matching constructor for initialization of 'CacheQuery'
    construct_at(_Tp* __location, _Args&&... __args)
    ^
1 error generated.

So add an explicit ctor to help the compiler.

Signed-off-by: Miklos Vajna <[email protected]>
Change-Id: I89b33935201eb2892aa628124fb024b9c9743dd3
@vmiklos
cool#11002 kit: fix memory corruption in Document::handleSaveMessage()
0c442cc 
Open a Draw document, do some edit action like inserting a new page,
press save -> crash.

No idea why this started to be a problem just now, but seems the trouble
is:

1) Document::handleSaveMessage() in the background process deletes
   _loKitDocument, which includes the view callback:

    #0 0x55e4a8bb3dfb in operator delete(void*, unsigned long) _asan_rtl_:3
...
    #18 0x7f02e7a8ef0c in std::__debug::map<unsigned long, std::shared_ptr<desktop::CallbackFlushHandler>, std::less<unsigned long>, std::allocator<std::pair<unsigned long const, std::shared_ptr<desktop::CallbackFlushHandler> > > >::~map() /usr/bin/../lib64/gcc/x86_64-suse-linux/12/../../../../include/c++/12/debug/map.h:136:22
    #19 0x7f02e79c8fe9 in desktop::LibLODocument_Impl::~LibLODocument_Impl() desktop/source/lib/init.cxx:1590:1
    #20 0x7f02e7984d41 in doc_destroy(_LibreOfficeKitDocument*) desktop/source/lib/init.cxx:2633:5
    #21 0x55e4a9028d67 in lok::Document::~Document() include/LibreOfficeKit/LibreOfficeKit.hxx:39:9

2) The document is deleted, and during shutdown some view callback is
   invoked in the core -> crash:

    #0 0x7f02d06f6d02 in SfxViewShell::libreOfficeKitViewCallback(int, rtl::OString const&) const sfx2/source/view/viewsh.cxx:3360:47
    #1 0x7f02ceb22a4d in InterceptLOKStateChangeEvent(unsigned short, SfxViewFrame*, com::sun::star::frame::FeatureStateEvent const&, SfxPoolItem const*) sfx2/source/control/unoctitm.cxx:1435:21
...
    #11 0x7f02abb2ff8c in Timer::Invoke() vcl/source/app/timer.cxx:75:21
    #12 0x7f02ab99407a in Scheduler::CallbackTaskScheduling() vcl/source/app/scheduler.cxx:480:20

Fix the problem similar to unloading a single view works in
Document::onUnload(): first unregister the view callbacks and only
delete the document, which avoids the crash on shutdown inside the
background save process.

Signed-off-by: Miklos Vajna <[email protected]>
Change-Id: I39f6e7c08ef9466a7e9e51a1a543ed57fa640753
@vmiklos vmiklos force-pushed the private/vmiklos/master branch from b05fb19 to 0c442cc Compare January 24, 2025 10:17
@vmiklos vmiklos merged commit e20c183 into master Jan 24, 2025
14 checks passed
@vmiklos vmiklos deleted the private/vmiklos/master branch January 24, 2025 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caolanm caolanm caolanm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Collabora Online
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vmiklos @caolanm