Download it now!
Bug 89783 - sal_uint16 counters for containers exceed during mailmerge
Summary: sal_uint16 counters for containers exceed during mailmerge
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
Inherited From OOo
Hardware: Other All
: medium normal
Assignee: Vasily Melenchuk (CIB)
URL:
Whiteboard: lhm-limux target:4.5.0 target:5.0.0 t...
Keywords:
Depends on:
Blocks:
 
Reported: 2015-03-02 17:10 UTC by Christoph Lutz
Modified: 2017-03-10 08:51 UTC (History)
6 users (show)

See Also:
Crash report or crash signature:


Attachments
main document containing 32 frames (10.37 KB, application/vnd.oasis.opendocument.text)
2015-03-02 17:10 UTC, Christoph Lutz
Details
main document containing 32 headers / different page styles (10.94 KB, application/vnd.oasis.opendocument.text)
2015-03-02 17:11 UTC, Christoph Lutz
Details
main document containing 32 lists (13.24 KB, application/vnd.oasis.opendocument.text)
2015-03-02 17:11 UTC, Christoph Lutz
Details
main document containing 32 sections (9.92 KB, application/vnd.oasis.opendocument.text)
2015-03-02 17:12 UTC, Christoph Lutz
Details
main document containing 32 tables (11.37 KB, application/vnd.oasis.opendocument.text)
2015-03-02 17:12 UTC, Christoph Lutz
Details
a datasource with 2051 datasets (22.82 KB, application/vnd.oasis.opendocument.spreadsheet)
2015-03-02 17:13 UTC, Christoph Lutz
Details
patch: not yet a complete solution, but a first clue into my approach (17.47 KB, patch)
2015-03-02 20:05 UTC, Christoph Lutz
Details
List of all 3259 places with for loop with sal_uInt16 counter (321.79 KB, text/plain)
2015-03-04 18:07 UTC, Christoph Lutz
Details
main document containing 32 draw lines (9.64 KB, application/vnd.oasis.opendocument.text)
2015-03-04 18:41 UTC, Christoph Lutz
Details
ODT document having >64k of sections (267.26 KB, application/vnd.oasis.opendocument.text)
2015-04-03 12:02 UTC, Vasily Melenchuk (CIB)
Details
ODT document having >64k of text frames (412.14 KB, application/vnd.oasis.opendocument.text)
2015-04-03 12:10 UTC, Vasily Melenchuk (CIB)
Details
ODT document having >64k of tables (4.50 MB, application/vnd.oasis.opendocument.text)
2015-04-03 12:11 UTC, Vasily Melenchuk (CIB)
Details
ODT document with > 64k lists (156.06 KB, application/vnd.oasis.opendocument.text)
2015-04-29 07:07 UTC, Katarina Behrens (Inactive)
Details
ODT document with > 64k headers (938.62 KB, application/vnd.oasis.opendocument.text)
2015-04-29 07:08 UTC, Katarina Behrens (Inactive)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Christoph Lutz 2015-03-02 17:10:08 UTC
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
Comment 1 Christoph Lutz 2015-03-02 17:11:29 UTC
Created attachment 113829 [details]
main document containing 32 headers / different page styles
Comment 2 Christoph Lutz 2015-03-02 17:11:56 UTC
Created attachment 113830 [details]
main document containing 32 lists
Comment 3 Christoph Lutz 2015-03-02 17:12:20 UTC
Created attachment 113831 [details]
main document containing 32 sections
Comment 4 Christoph Lutz 2015-03-02 17:12:43 UTC
Created attachment 113832 [details]
main document containing 32 tables
Comment 5 Christoph Lutz 2015-03-02 17:13:14 UTC
Created attachment 113833 [details]
a datasource with 2051 datasets
Comment 6 Christoph Lutz 2015-03-02 20:05:33 UTC
Created attachment 113837 [details]
patch: not yet a complete solution, but a first clue into my approach
Comment 7 Christoph Lutz 2015-03-03 08:06:53 UTC
one first patch ist here: https://gerrit.libreoffice.org/#/c/14725/
Comment 8 Christoph Lutz 2015-03-04 14:47:24 UTC
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).
Comment 9 Christoph Lutz 2015-03-04 17:15:20 UTC
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...
Comment 10 Christoph Lutz 2015-03-04 18:07:31 UTC
Created attachment 113880 [details]
List of all 3259 places with for loop with sal_uInt16 counter
Comment 11 Christoph Lutz 2015-03-04 18:09:04 UTC
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"
Comment 12 Christoph Lutz 2015-03-04 18:41:57 UTC
Created attachment 113888 [details]
main document containing 32 draw lines
Comment 13 Christoph Lutz 2015-03-06 08:59:41 UTC
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
Comment 14 Commit Notification 2015-03-18 13:10:03 UTC
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.
Comment 15 Commit Notification 2015-03-24 10:06:02 UTC
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.
Comment 16 Thorsten Behrens (CIB) 2015-04-02 15:00:39 UTC
I'd say this problem is pretty confirmed.
Comment 17 Vasily Melenchuk (CIB) 2015-04-03 12:02:39 UTC
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.
Comment 19 Vasily Melenchuk (CIB) 2015-04-03 12:10:10 UTC
Created attachment 114581 [details]
ODT document having >64k of text frames
Comment 20 Vasily Melenchuk (CIB) 2015-04-03 12:11:01 UTC
Created attachment 114582 [details]
ODT document having >64k of tables
Comment 21 Commit Notification 2015-04-28 15:58:57 UTC
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.
Comment 22 Katarina Behrens (Inactive) 2015-04-29 07:07:31 UTC
Created attachment 115172 [details]
ODT document with > 64k  lists
Comment 23 Katarina Behrens (Inactive) 2015-04-29 07:08:18 UTC
Created attachment 115173 [details]
ODT document with > 64k  headers
Comment 24 Commit Notification 2015-04-29 08:19:45 UTC
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.
Comment 25 Commit Notification 2015-04-29 08:19:48 UTC
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.
Comment 26 Commit Notification 2015-05-03 21:37:24 UTC
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.
Comment 27 Commit Notification 2015-05-03 21:37:28 UTC
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.
Comment 28 Commit Notification 2015-05-18 11:55:43 UTC
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.
Comment 29 Commit Notification 2015-05-28 13:45:45 UTC
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 30 Björn Michaelsen 2015-07-06 16:07:24 UTC
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.
Comment 31 Xisco Faulí 2016-09-15 10:13:10 UTC
Hi Vasily,
Can this bug be converted into an easyhack?
Regards
Comment 32 Katarina Behrens (Inactive) 2016-09-15 11:12:41 UTC
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
Comment 33 Xisco Faulí 2016-09-15 11:30:35 UTC
@Bubli: Thanks for the feedback.
Thus, closing this bug as mentioned in comment 32.