Bug 79135 - Crash in std::_Rb_tree
Summary: Crash in std::_Rb_tree
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
4.3.0.0.beta1
Hardware: Other All
: medium critical
Assignee: Markus Mohrhard
URL:
Whiteboard: target:4.4.0 target:4.3.0.1
Keywords:
Depends on:
Blocks:
 
Reported: 2014-05-23 15:45 UTC by nicolas.gregoire
Modified: 2014-06-07 03:13 UTC (History)
5 users (show)

See Also:
Crash report or crash signature:


Attachments
Repro file (36.03 KB, application/vnd.openxmlformats-officedocument.spreadsheetml.sheet)
2014-05-23 15:45 UTC, nicolas.gregoire
Details
Original file (36.19 KB, application/vnd.openxmlformats-officedocument.spreadsheetml.sheet)
2014-05-23 15:45 UTC, nicolas.gregoire
Details
bt with symbols (17.81 KB, text/plain)
2014-05-24 15:55 UTC, Julien Nabet
Details

Note You need to log in before you can comment on or make changes to this bug.
Description nicolas.gregoire 2014-05-23 15:45:34 UTC
Created attachment 99655 [details]
Repro file

When opening a mutated DOCX file, an ASan build of LO 4.4.0.0 alpha0 will crash:

Program received signal SIGSEGV, Segmentation fault.
0x00007fffa7b41f4a in std::_Rb_tree<rtl::OUString, std::pair<rtl::OUString const, void*>, std::_Select1st<std::pair<rtl::OUString const, void*> >, std::less<rtl::OUString>, std::allocator<std::pair<rtl::OUString const, void*> > >::_M_begin (this=<optimized out>) at /usr/lib64/gcc/x86_64-suse-linux/4.7/../../../../include/c++/4.7/bits/stl_tree.h:505

rax            0x10     16
rbx            0x7ffffffe8700   140737488258816
rcx            0x10     16
rdx            0x2      2
rsi            0x10007fff50ac   17594333483180
rdi            0x7ffffffe8580   140737488258432
rbp            0x7ffffffe85d0   0x7ffffffe85d0
rsp            0x7ffffffe8520   0x7ffffffe8520

   0x00007fffa7b41f45 <std::_Rb_tree<rtl::OUString, std::pair<rtl::OUString const, void*>, std::_Select1st<std::pair<rtl::OUString const, void*> >, std::less<rtl::OUString>, std::allocator<std::pair<rtl::OUString const, void*> > >::_M_begin() const+309>:  mov    0x8(%rsp)
,%rax
=> 0x00007fffa7b41f4a <std::_Rb_tree<rtl::OUString, std::pair<rtl::OUString const, void*>, std::_Select1st<std::pair<rtl::OUString const, void*> >, std::less<rtl::OUString>, std::allocator<std::pair<rtl::OUString const, void*> > >::_M_begin() const+314>:  mov    (%rax),%r
cx
   0x00007fffa7b41f4d <std::_Rb_tree<rtl::OUString, std::pair<rtl::OUString const, void*>, std::_Select1st<std::pair<rtl::OUString const, void*> >, std::less<rtl::OUString>, std::allocator<std::pair<rtl::OUString const, void*> > >::_M_begin() const+317>:  mov    0x20(%rsp
),%rdx

Original OO file: Matrix_%3092712.xlsx
Mutated OO file (repro file): crash-30775.docx

Modified XML file: word/document.xml
Numerous modifications were done on tag "definedName" (cf modifs.xml in crash-30775.docx for details)
Comment 1 nicolas.gregoire 2014-05-23 15:45:50 UTC
Created attachment 99657 [details]
Original file
Comment 2 Julien Nabet 2014-05-24 15:55:02 UTC
Created attachment 99718 [details]
bt with symbols

On pc Debian x86-64 with master sources updated yesterday, I could reproduce this.
Comment 3 Julien Nabet 2014-05-24 16:01:36 UTC
Kohei/Markus/Eike: docx file but bt showed this:
#5  0x00002aaadcf083dc in ScRangeName::findByUpperName (this=0x0, rName="_XLNM.PRINT_AREA")
    at /home/julien/compile-libreoffice/libreoffice/sc/source/core/tool/rangenam.cxx:707
#6  0x00002aaaded3b6a2 in oox::xls::(anonymous namespace)::findUnusedName (pRangeName=0x0, rSuggestedName="_xlnm.Print_Area")
    at /home/julien/compile-libreoffice/libreoffice/sc/source/filter/oox/workbookhelper.cxx:402
#7  0x00002aaaded3b845 in oox::xls::WorkbookGlobals::createLocalNamedRangeObject (this=0x89d7650, orName="_xlnm.Print_Area", rTokens=empty uno::Sequence, nIndex=1, 
    nNameFlags=0, nTab=31337) at /home/julien/compile-libreoffice/libreoffice/sc/source/filter/oox/workbookhelper.cxx:437

Indeed pNames returned by GetRangeName may be null
(see http://opengrok.libreoffice.org/xref/core/sc/source/filter/oox/workbookhelper.cxx#433)
Adding an "if (pNames)" like this:
+        if (pNames)
+        {
+            // find an unused name
+            orName = findUnusedName( pNames, orName );
+            // create the named range
+            pScRangeData = lcl_addNewByNameAndTokens( rDoc, pNames, orName, rTokens, nIndex, nNameFlags );
+        }

stops the crash.

Now 
1) createNamedRangeObject method some lines above has the same problem
See http://opengrok.libreoffice.org/xref/core/sc/source/filter/oox/workbookhelper.cxx#410
2) Should we add a console debug message, display a popup warning indicating there's something wrong on the file?

Any idea?
Comment 4 Markus Mohrhard 2014-05-24 16:21:44 UTC
(In reply to comment #3)
> Kohei/Markus/Eike: docx file but bt showed this:
> #5  0x00002aaadcf083dc in ScRangeName::findByUpperName (this=0x0,
> rName="_XLNM.PRINT_AREA")
>     at
> /home/julien/compile-libreoffice/libreoffice/sc/source/core/tool/rangenam.
> cxx:707
> #6  0x00002aaaded3b6a2 in oox::xls::(anonymous namespace)::findUnusedName
> (pRangeName=0x0, rSuggestedName="_xlnm.Print_Area")
>     at
> /home/julien/compile-libreoffice/libreoffice/sc/source/filter/oox/
> workbookhelper.cxx:402
> #7  0x00002aaaded3b845 in
> oox::xls::WorkbookGlobals::createLocalNamedRangeObject (this=0x89d7650,
> orName="_xlnm.Print_Area", rTokens=empty uno::Sequence, nIndex=1, 
>     nNameFlags=0, nTab=31337) at
> /home/julien/compile-libreoffice/libreoffice/sc/source/filter/oox/
> workbookhelper.cxx:437
> 
> Indeed pNames returned by GetRangeName may be null
> (see
> http://opengrok.libreoffice.org/xref/core/sc/source/filter/oox/
> workbookhelper.cxx#433)
> Adding an "if (pNames)" like this:
> +        if (pNames)
> +        {
> +            // find an unused name
> +            orName = findUnusedName( pNames, orName );
> +            // create the named range
> +            pScRangeData = lcl_addNewByNameAndTokens( rDoc, pNames, orName,
> rTokens, nIndex, nNameFlags );
> +        }
> 
> stops the crash.

That is just wrong. ScDocument::GetRangeName can not return a null pointer.

> 
> Now 
> 1) createNamedRangeObject method some lines above has the same problem
> See
> http://opengrok.libreoffice.org/xref/core/sc/source/filter/oox/
> workbookhelper.cxx#410
> 2) Should we add a console debug message, display a popup warning indicating
> there's something wrong on the file?
> 
> Any idea?

The issue is that the document has been imported into calc. So the fix is to find out why the filter detection code messed up and not try to add wrong checks into correct code. I'd very much prefer if we only add nul pointer checks if there is a reason and in this case the bug is much earlier.
Comment 5 Julien Nabet 2014-05-24 16:42:13 UTC
Markus: thank you for your feedback.

Trying to unwind this, I noticed this
#21 0x00002aaadebc43e4 in oox::xls::ExcelFilter::filter (this=0x89420c0, rDescriptor=uno::Sequence of length 13 = {...})
    at /home/julien/compile-libreoffice/libreoffice/sc/source/filter/oox/excelfilter.cxx:175
#22 0x00002aaaada926e1 in SfxObjectShell::ImportFrom (this=0x8895ed0, rMedium=..., xInsertPosition=empty uno::Reference)
    at /home/julien/compile-libreoffice/libreoffice/sfx2/source/doc/objstor.cxx:2269

2269         bool bRtn = xLoader->filter( aArgs );
xLoader comes from :
   2208         xLoader = uno::Reference< document::XFilter >
   2209             ( xFilterFact->createInstanceWithArguments( aFilterName, uno::Sequence < uno::Any >() ), uno::UNO_QUERY );

which leads here:
http://opengrok.libreoffice.org/xref/core/filter/source/config/cache/filterfactory.cxx#79

Is it the good way?
Comment 6 Maxim Monastirsky 2014-05-25 15:00:50 UTC
(In reply to comment #4)
> The issue is that the document has been imported into calc. So the fix is to
> find out why the filter detection code messed up
In fact this file looks like xlsx file with a wrong extension, so the type detection is right here.
Comment 7 Markus Mohrhard 2014-06-07 02:02:58 UTC
Ok, added a check that prevents us from importing the whole document. It is invalid and we should bail out early.
Comment 8 Commit Notification 2014-06-07 02:54:42 UTC
Markus Mohrhard committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=86f8772131a0588f3bfa4f09cc03746a2afe8cc8

don't try to import invalid document, fdo#79135



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 9 Commit Notification 2014-06-07 03:13:02 UTC
Markus Mohrhard committed a patch related to this issue.
It has been pushed to "libreoffice-4-3":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=1ccbf70397adf07f1fe55f61c05ce1a741305cfc&h=libreoffice-4-3

don't try to import invalid document, fdo#79135


It will be available in LibreOffice 4.3.

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.