Created attachment 113828 [details] main document containing 32 frames Lot of containers for elements in writer are internally managed by SwVectorModifyBase class which has an old method 102 sal_uInt16 GetPos(Value const& p) const 103 { 104 const_iterator const it = std::find(begin(), end(), p); 105 return it == end() ? USHRT_MAX : it - begin(); 106 } see http://opengrok.libreoffice.org/xref/core/sw/inc/docary.hxx#102 This means: In all concrete element container implementations/managers using this methods, only 65536 elements can be managed in a safe way. Adding more elements than 65536 may cause LibreOffice to do undefined things (in the conrete scenario it freezes). This limit can easily exceed in particular during mail merge. I attached some example documents that can be used as main documents for mail merge and a datasource with >2048 datasets. They contain just 32 elements of one type and can't be processed by mail merge with more than 2048 datasets (32*2048 = 65336 --> 16bit limit) In my tests the following elements were causing troubles (and maybe others, too): - main document with 32 sections - main document with 32 text frames - main document with 32 lists - main document with 32 different header definitions (=page styles) - main document with 32 tables The 16bit limit needs to be eliminated. The container behind SwVectorModifyBase is already a std::vector (which has no 16 bit limitation), but the old Method GetPos is referred from many places of the code (>700 hits for "git grep GetPos") How to reproduce the issue: 1) open one of the attached 32xxx.odt files 2) use tools->mail merge wizard 3) select 2051datasets.ods as datasource (in step 3 of the wizard) 4) run step 7 of the wizard (personalize document) --> in most cases LO freezes after 2048 datasets
Created attachment 113829 [details] main document containing 32 headers / different page styles
Created attachment 113830 [details] main document containing 32 lists
Created attachment 113831 [details] main document containing 32 sections
Created attachment 113832 [details] main document containing 32 tables
Created attachment 113833 [details] a datasource with 2051 datasets
Created attachment 113837 [details] patch: not yet a complete solution, but a first clue into my approach
one first patch ist here: https://gerrit.libreoffice.org/#/c/14725/
Pasted a comment from the LiMux internal issue tracker written by Lubos Lunak (Collabora): ... Specifically the problems I ran into were internal counters, as said above. Objects such as frames have a document-wide container for all of them, and if e.g. sal_uInt16 is used in a loop iterating over all of them, it will become an infinite loop if there are more objects than maximum value the variable can hold. I fixed few of such problems in http://cgit.freedesktop.org/libreoffice/core/commit/?id=43f98495b5d0756fc37eef99efba45e388101a1a , but there are presumably more. The best way of finding all such problems should be using MM with a test document that would make the resulting merged document contain more than 65535 items of each object type, seeing where MM breaks, and then fixing those places in the code (which may be a non-trivial amount of work though, as the ancient sal_IntXY types are in widespread use in LO code).
Lubos comments hinted me to the solution for the freeze in case of "main document containing 32 frames". This simple patch seems to solve the issue: diff --cc sw/source/core/layout/frmtool.cxx index 7145ef7,7145ef7..93b5f2e --- a/sw/source/core/layout/frmtool.cxx +++ b/sw/source/core/layout/frmtool.cxx @@@ -1005,7 -1005,7 +1005,7 @@@ void AppendObjs( const SwFrmFmts *pTbl (void) pTbl; #if OSL_DEBUG_LEVEL > 0 std::list<SwFrmFmt*> checkFmts; -- for ( sal_uInt16 i = 0; i < pTbl->size(); ++i ) ++ for ( size_t i = 0; i < pTbl->size(); ++i ) { SwFrmFmt *pFmt = (*pTbl)[i]; const SwFmtAnchor &rAnch = pFmt->GetAnchor(); It is of cause still open to check further cases...
Created attachment 113880 [details] List of all 3259 places with for loop with sal_uInt16 counter
This causes me to grep for more situations in which sal_uInt16 is used within a for statement. There are many. I used the following statement: find . -type f -exec perl -ne '++$x; exit unless($ARGV =~ /\.(hxx|cxx|cpp|h|c)/); print "$ARGV:$x: $_" if (/for\s*\(\s*sal_uInt16/)' {} \; >sal_uInt16_loops.txt see attachment "List of all 3259 places with for loop with sal_uInt16 counter"
Created attachment 113888 [details] main document containing 32 draw lines
In this comment I try to specify little bit more concrete what needs to be done to solve this issue completely: The current state regarding the above test files is: * 32 frames --> LO freezes after 2048 datasets (tested with master) * 32 sections --> freezes after 2048 datasets (tested with master) * 32 draw lines --> freezes after 2048 datasets (tested with master) * 32 (different) Headers --> segfault somewhen >2000 datasets processed (tested with LO 4.2 on Win) * 32 lists --> behaves like O(n2) but seems to run successfully (tested with LO 4.2 on Win). It is not yet tested what happens if there are actions done on the created document * 32 tables --> mailmerge runs successfully. Changing some of the tables in the result document seems to work, but saving the result seems to freeze/hang LO The expected behaviour is: 1) mail merge on the above document can be executed successfully (= no more freeze). This should be ensured for all possible ways to start mail merge in LibreOffice: mail merge printing via "File->Print", mail merge via "Tools->Mail Merge Wizard", mail merge via UNO-Service. 2) Test and fix roundtripping and further processing actions on the generated (big) document: 2a) Change some of the elements in the generated document - at least test with some elements on the first pages, some elements from the middle of the document and some elements on the last pages. Possible change actions are: - add some (new) text - change some formatting attributes 2b) Remove some elements from the document - again from the beginning, middle and end 2c) Add some new elements at the begin, middle and end 2d) In particular ensure that copy & paste works - I got hints that this could be a problematic cornercase: test doing CTRL+a, CTRL+c and CTRL+v at the end of the generated document. 2e) Save and close the result document 2f) reopening the document works fine and changes like in 2a) can be done successfully In order to make testing more efficient, It could be useful to write the corresponding unit tests very early. This could for example be done like in http://cgit.freedesktop.org/libreoffice/core/tree/sw/qa/extras/mailmerge/mailmerge.cxx
Vasily Melenchuk committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=edbf82cbfc8d886db40b1b6e3d7b3ef84fddf604 tdf#89783: sal_uInt16 replacement by size_t It will be available in 4.5.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Vasily Melenchuk committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=8418de72f592daae87a385f105519d637dfa4841 tdf#89783: sal_uInt16 replacement by size_t It will be available in 4.5.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
I'd say this problem is pretty confirmed.
Created attachment 114580 [details] ODT document having >64k of sections Attached document with 64k sections. It hangs up upon loading and saving in LibreOffice. Work on it is in progress.
Fixes for tables (http://cgit.freedesktop.org/libreoffice/core/commit/?id=edbf82cbfc8d886db40b1b6e3d7b3ef84fddf604) and text frames (http://cgit.freedesktop.org/libreoffice/core/commit/?id=8418de72f592daae87a385f105519d637dfa4841) are ready.
Created attachment 114581 [details] ODT document having >64k of text frames
Created attachment 114582 [details] ODT document having >64k of tables
Christoph Lutz committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=4de83e2c322509c0fb1b989f7e4898728fc4a408 tdf#89783: MM fixes potential endless loops with dbgutil build It will be available in 5.0.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Created attachment 115172 [details] ODT document with > 64k lists
Created attachment 115173 [details] ODT document with > 64k headers
Katarina Behrens committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=1fe1fb546889d824b7331ca0f4731fdf58aea829 tdf#89783: fix another potential endless loop It will be available in 5.0.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Katarina Behrens committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=f01c61a70dca72e4e66f0e6f9df5c2ad52bb778f tdf#89783: avoid endless loop with >65k style names It will be available in 5.0.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Vasily Melenchuk committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=bc9d02b0ca6244b46c9e2c59b7cc3618eb0f0148 tdf#89783: sal_uInt16 replacement by size_t: sections It will be available in 5.0.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Katarina Behrens committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=98436c4b53639d86f261ac630c46d32e3c7b8e28 tdf#89783: Adjust to new GetPos retval (size_t vs. sal_uInt16) It will be available in 5.0.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Katarina Behrens committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=3ccacb47faa625563f8bf0af8d4c62c1eea6ce7c tdf#89783: Allow more than 64k PageDescs It will be available in 5.0.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Katarina Behrens committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=f12f5525427d7d743a267ab2e8bf5315d389e9cf Unit test related to tdf#89783: more than 64k PageDescs It will be available in 5.1.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Comment on attachment 113837 [details] patch: not yet a complete solution, but a first clue into my approach Marking the patch as obsolete, as there is newer work. Please use gerrit (or gerrit drafts: https://wiki.documentfoundation.org/Development/gerrit/SubmitPatch#Submitting_patches_as_drafts ) for discussion of source code, if only because that helps knowing what state the change is build against.
Hi Vasily, Can this bug be converted into an easyhack? Regards
Hullo Xisco this bug can be closed. The most burning issues in Writer have been solved last year and for the remaining cases in the rest of the codebase, clang plugin has been written (loopvartoosmall), activated and all the reported incidents have been fixed by noelgrandin and me. Nothing left to do, since the plugin is active and guards that people don't add new code containing loop int overflow issues
@Bubli: Thanks for the feedback. Thus, closing this bug as mentioned in comment 32.