Created attachment 193026 [details] Crash because of wrong iteration over vector Description: When windowed presenter console is enabled in RTL UI, starting slide show result in an immediate crash. This does not happen in LTR UI. Steps to Reproduce: 1. Open Impress in RTL mode: $ SAL_RTL_ENABLED=1 instdir/program/soffice --impress 2. Activate presenter console in windowed mode in "Slide Show > Slide Show Settings" 3. Start slide show by pressing F5. Actual Results: Immediate Crash Expected Results: Not crashing Reproducible: Always User Profile Reset: No Additional Info: Version: 24.2.0.3 (X86_64) / LibreOffice Community Build ID: da48488a73ddd66ea24cf16bbc4f7b9c08e9bea1 CPU threads: 20; OS: Windows 10.0 Build 22631; UI render: Skia/Raster; VCL: win Locale: en-US (en_US); UI: en-US Calc: CL threaded
Created attachment 193027 [details] bt with debug symbols On pc Debian x86-64 with master sources updated today, I could reproduce this.
Also reproduced with debug build: Version: 24.8.0.0.alpha0+ (X86_64) / LibreOffice Community Build ID: a1a1d8edb9d4a62b747aa7069b3026e2ba75704d CPU threads: 8; OS: Linux 6.5; UI render: default; VCL: gtk3 Locale: en-AU (en_AU.UTF-8); UI: en-US Calc: CL threaded In console: warn:legacy.tools:125993:125993:svx/source/form/fmshimp.cxx:1796: only to be used in alive mode /opt/rh/gcc-toolset-12/root/usr/include/c++/12/debug/safe_iterator.h:954: In function: gnu_debug::_Safe_iterator<gnu_cxx:: normal_iterator<std::shared_ptr<sdext::presenter::PresenterToolBar::ElementContainerPart>*, std:: vector<std::shared_ptr<sdext::presenter::PresenterToolBar::ElementContainerPart>, std::allocator<std::shared_ptr<sdext::presenter::PresenterToolBar::ElementContainerPart> > > >, std:: debug::vector<std::shared_ptr<sdext::presenter::PresenterToolBar::ElementContainerPart> >, std::random_access_iterator_tag>::_Self gnu_debug::operator-(const _Safe_iterator<gnu_cxx:: normal_iterator<std::shared_ptr<sdext::presenter::PresenterToolBar::ElementContainerPart>*, std:: vector<std::shared_ptr<sdext::presenter::PresenterToolBar::ElementContainerPart>, std::allocator<std::shared_ptr<sdext::presenter::PresenterToolBar::ElementContainerPart> > > >, std:: debug::vector<std::shared_ptr<sdext::presenter::PresenterToolBar::ElementContainerPart> >, std::random_access_iterator_tag>::_Self&, _Safe_iterator<gnu_cxx:: normal_iterator<std::shared_ptr<sdext::presenter::PresenterToolBar::ElementContainerPart>*, std:: vector<std::shared_ptr<sdext::presenter::PresenterToolBar::ElementContainerPart>, std::allocator<std::shared_ptr<sdext::presenter::PresenterToolBar::ElementContainerPart> > > >, std:: debug::vector<std::shared_ptr<sdext::presenter::PresenterToolBar::ElementContainerPart> >, std::random_access_iterator_tag>::difference_type) Error: attempt to retreat a dereferenceable (start-of-sequence) iterator 1 steps, which falls outside its valid range. Objects involved in the operation: iterator @ 0x7ffcdbac2f50 { type = gnu_cxx::normal_iterator<std::shared_ptr<sdext::presenter::PresenterToolBar::ElementContainerPart>*, std::vector<std::shared_ptr<sdext::presenter::PresenterToolBar::ElementContainerPart>, std::allocator<std::shared_ptr<sdext::presenter::PresenterToolBar::ElementContainerPart> > > > (mutable iterator); state = dereferenceable (start-of-sequence); references sequence with type 'std::debug::vector<std::shared_ptr<sdext::presenter::PresenterToolBar::ElementContainerPart>, std::allocator<std::shared_ptr<sdext::presenter::PresenterToolBar::ElementContainerPart> > >' @ 0x6368da8 }
Thank you Stéphane for the console logs, I had forgotten to attach them. I tried on a non debug build and didn't reproduce the pb quite weirdly.
(In reply to Julien Nabet from comment #3) > I tried on a non debug build and didn't reproduce the pb quite weirdly. Same, for 24.2.1 and 24.8alpha0+. (In reply to Hossein from comment #0) > Version: 24.2.0.3 (X86_64) / LibreOffice Community Hossein, is this a debug build? Was it really reproduced in 24.2.0?
(In reply to Stéphane Guillou (stragu) from comment #4) > (In reply to Julien Nabet from comment #3) > > I tried on a non debug build and didn't reproduce the pb quite weirdly. > Same, for 24.2.1 and 24.8alpha0+. > > (In reply to Hossein from comment #0) > > Version: 24.2.0.3 (X86_64) / LibreOffice Community > Hossein, is this a debug build? Was it really reproduced in 24.2.0? You are correct. The crash actually happens on a debug build, and the above build info in comment 0 is wrong. The correct one is this: Version: 24.8.0.0.alpha0+ (X86_64) / LibreOffice Community Build ID: f4ba83e82aafc206d17d2fa66a27573ebc5a3624 CPU threads: 20; OS: Windows 10.0 Build 22631; UI render: Skia/Vulkan; VCL: win Locale: en-US (en_US); UI: en-US Calc: CL threaded
There are 2 main parts which rely on RTL/LTR layout in sd/source/console/PresenterToolBar.cxx: 1) in PresenterToolBar::Layout line 747 2) in PresenterToolBar::LayoutPart line 846 I thought that instead of 2 different treatments, perhaps we may just use a reversed vector. "maElementContainer" is defined as "ElementContainer" which is a typedef of ::std::vector<SharedElementContainerPart>. For 1) it does the trick. For 2) we do "for (auto& rxElement : *rpPart)" rpPart is "SharedElementContainerPart" which is defined as "std::shared_ptr<ElementContainerPart>" so *rpPart is "ElementContainerPart". This last one is defined as "class PresenterToolBar::ElementContainerPart : public ::std::vector<rtl::Reference<Element> >" I tried to simplify this by creating a new typedef as: typedef ::std::vector<rtl::Reference<Element> > ElementContainerPart; but it doesn't know "Element" in sd/source/console/PresenterToolBar.hxx. IMHO, there are a lot of things defined in sd/source/console/PresenterToolBar.cxx which should be in sd/source/console/PresenterToolBar.hxx but I got lost with namespaces uses, I didn't succeed in building LO with this refactoring.
I've submitted the patch here: https://gerrit.libreoffice.org/c/core/+/164592
(In reply to Julien Nabet from comment #7) > I've submitted the patch here: > https://gerrit.libreoffice.org/c/core/+/164592 Sorry wrong bug.
(In reply to Julien Nabet from comment #3) > I tried on a non debug build and didn't reproduce the pb quite weirdly. I suspect that's because `--enable-dbgutil` on Linux enables `-D_GLIBCXX_DEBUG`, i.e. extra bounds checks for containers within libstdc++ (the C++ library implementation) which detect the invalid iterator and abort. But in a non-debug build, that is just "ignored" (which may result in other unexpected behavior, of course...). (In reply to Julien Nabet from comment #6) > There are 2 main parts which rely on RTL/LTR layout in > sd/source/console/PresenterToolBar.cxx: > 1) in PresenterToolBar::Layout line 747 > 2) in PresenterToolBar::LayoutPart line 846 > > I thought that instead of 2 different treatments, perhaps we may just use a > reversed vector. > "maElementContainer" is defined as "ElementContainer" which is a typedef of > ::std::vector<SharedElementContainerPart>. > > For 1) it does the trick. > > For 2) we do "for (auto& rxElement : *rpPart)" > rpPart is "SharedElementContainerPart" which is defined as > "std::shared_ptr<ElementContainerPart>" > so *rpPart is "ElementContainerPart". > This last one is defined as "class PresenterToolBar::ElementContainerPart > : public ::std::vector<rtl::Reference<Element> >" The series up to https://gerrit.libreoffice.org/c/core/+/185383 is my take at this which also does some deduplication. More cleanup/simplification may be possible. I mostly left 2) untouched, which has some special "// reverse presentation time with current time" logic that seems to make use of "internal knowledge" of what element is contained at what position in that vector and makes unifying code paths more challenging. (May still be possible, but would need a closer look.) > I tried to simplify this by creating a new typedef as: > typedef ::std::vector<rtl::Reference<Element> > ElementContainerPart; > > but it doesn't know "Element" in sd/source/console/PresenterToolBar.hxx. I had the same thought when coming across this code a few days earlier while looking into something else and independently had the same idea, implemented in commit f63690411df62f373d04d6b78bb93956f5532abf Author: Michael Weghorn <m.weghorn@posteo.de> Date: Thu May 15 09:27:22 2025 +0200 sd console: Don't subclass std::vector
(In reply to Michael Weghorn from comment #9) > (In reply to Julien Nabet from comment #3) > > I tried on a non debug build and didn't reproduce the pb quite weirdly. > > I suspect that's because `--enable-dbgutil` on Linux enables > `-D_GLIBCXX_DEBUG`, i.e. extra bounds checks for containers within libstdc++ > (the C++ library implementation) which detect the invalid iterator and > abort. But in a non-debug build, that is just "ignored" (which may result in > other unexpected behavior, of course...). > > (In reply to Julien Nabet from comment #6) > > There are 2 main parts which rely on RTL/LTR layout in > > sd/source/console/PresenterToolBar.cxx: > > 1) in PresenterToolBar::Layout line 747 > > 2) in PresenterToolBar::LayoutPart line 846 > > > > I thought that instead of 2 different treatments, perhaps we may just use a > > reversed vector. > > "maElementContainer" is defined as "ElementContainer" which is a typedef of > > ::std::vector<SharedElementContainerPart>. > > > > For 1) it does the trick. > > > > For 2) we do "for (auto& rxElement : *rpPart)" > > rpPart is "SharedElementContainerPart" which is defined as > > "std::shared_ptr<ElementContainerPart>" > > so *rpPart is "ElementContainerPart". > > This last one is defined as "class PresenterToolBar::ElementContainerPart > > : public ::std::vector<rtl::Reference<Element> >" > > More cleanup/simplification may > be possible. I mostly left 2) untouched, which has some special "// reverse > presentation time with current time" logic that seems to make use of > "internal knowledge" of what element is contained at what position in that > vector and makes unifying code paths more challenging. (May still be > possible, but would need a closer look.) Upon further analysis, I came up with https://gerrit.libreoffice.org/c/core/+/185407 that drops/simplifies that special logic. From what I can see, behavior remains the same with that change in place, but please let me know in case I missed anything and you see any issues with that series of changes in place.
Michael Weghorn committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/140fb3b98b4f8e1331634213a0825fbe92ccda15 tdf#160094 sd presenter: Don't use invalid iterator for RTL It will be available in 25.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.
Michael Weghorn committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/fc78db91c21468cb5cedb70707e7bfc618edc38f tdf#160094 sd presenter: Use unordered_map for mapping It will be available in 25.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.
Michael Weghorn committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/6f0805563f850b0756d42fd793a63ba91868a74a tdf#160094 sd presenter: Deduplicate RTL and non-RTL code paths It will be available in 25.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.
Michael Weghorn committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/55b13d5faf3ad9e716c196a263cb86c5408b9284 tdf#160094 sd presenter: Simplify RTL-specific handling It will be available in 25.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.
On pc Debian x86-64 with master sources updated today (1724c2fe47d36ac8f8049e428ebfa7284b390a14), I don't reproduce the assertion anymore. Thank you Michael!
Michael Weghorn committed a patch related to this issue. It has been pushed to "libreoffice-25-2": https://git.libreoffice.org/core/commit/8c5a81201e89e406b58f2db325373347ccc8e647 tdf#160094 sd presenter: Don't use invalid iterator for RTL It will be available in 25.2.4. 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.