Bug Hunting Session
Bug 99556 - CRASH: NULL pointer dereference in MathML node.cxx
Summary: CRASH: NULL pointer dereference in MathML node.cxx
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: framework (show other bugs)
Version:
(earliest affected)
5.2.0.0.alpha1
Hardware: All All
: medium normal
Assignee: Caolán McNamara
URL:
Whiteboard: target:5.2.0 target:5.1.4
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-28 16:50 UTC by Sergey Zelenyuk
Modified: 2016-10-25 19:01 UTC (History)
2 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Sergey Zelenyuk 2016-04-28 16:50:18 UTC
Abstract: MathML msqrt element will be handled improperly when it has no children.

Problem: there is no code to check whether a node has a subnodes or not in SmRootNode::CreateTextFromNode function:

// ...
if (!pExtra && GetSubNode(2)->GetNumSubNodes() > 1)
    rText += "{ ";

GetSubNode(2)->CreateTextFromNode(rText);

if (!pExtra && GetSubNode(2)->GetNumSubNodes() > 1)
    rText += "} ";
// ...

In the case of <msqrt/> (see below) a return value of GetSubNode will be NULL so it cannot be dereferenced.

Solution: store a return values of GetSubNode and compare them with NULL before dereferencing.

How to reproduce: copy the code below into example.xml and open it in LibreOffice.
<math xmlns="http://www.w3.org/1998/Math/MathML">
<msqrt/>
</math>
Comment 1 Regina Henschel 2016-04-28 17:42:21 UTC
Import via clipboard shows the crash as well.

Making the method SmRootNode::CreateTextFromNode save is one thing, and should be done.

But I think the error happens earlier. If a msqrt element is empty, it should be handled as 
<msqrt>
  <mrow>
  </mrow>
</msqrt>
So it would have a child.

https://www.w3.org/TR/MathML2/chapter3.html#presm.reqarg
Comment 2 Sergey Zelenyuk 2016-04-28 19:29:55 UTC
The cause of problem is in mathmlimport.cxx.

// ...
void SmXMLSqrtContext_Impl::EndElement()
{
    /*
    <msqrt> accepts any number of arguments; if this number is not 1, its
    contents are treated as a single "inferred <mrow>" containing its
    arguments
    */
    if (GetSmImport().GetNodeStack().size() - nElementCount > 1)
        SmXMLRowContext_Impl::EndElement();
// ...

If <msqrt> has more than one children they will be handled inside a <mrow>, but it should also be done if there is no child.

- GetSmImport().GetNodeStack().size() - nElementCount > 1
+ GetSmImport().GetNodeStack().size() - nElementCount != 1

Not only msqrt affect to this problem but also mphantom, menclose, mstyle, mpadded and several attributes such as fontweight. Unlike msqrt, they will not crash the program now because they have no code deals with children in their EndElement functions, but they can do it in future.

mphantom and font attributes have been fixed in https://bugs.documentfoundation.org/show_bug.cgi?id=98088. The fix adds a code which checks number of children but the cause of problem remains.

menclose, mstyle and mpadded aren't cause a crash because they have no code in mathmlimport.cxx which works with children elements but the problem at the place anyway.

Summarize. How to fix the cause:
1) msqrt. See above.
2) mphantom, menclose, mstyle, mpadded and font attributes. Same changes as with msqrt should be done but in other functions:
SmXMLPhantomContext_Impl::EndElement
SmXMLEncloseContext_Impl::EndElement
SmXMLStyleContext_Impl::EndElement
SmXMLPaddedContext_Impl::EndElement
Comment 3 Regina Henschel 2016-04-28 20:14:00 UTC
You have already analyzed the problem so deep; what do you think about contributing to LibreOffice and fixing the bug yourself?
Comment 4 Sergey Zelenyuk 2016-04-28 23:22:42 UTC
Sorry, I'm a bad programmer and I try to do the best I can. My position is that a programmer is responsible for a code that he write, even it is only a bug fix. If the one cannot maintain the code after he had wrote it, the one should not do it at all.
Comment 5 Commit Notification 2016-05-13 12:24:17 UTC
Caolán McNamara committed a patch related to this issue.
It has been pushed to "master":

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

Resolves: tdf#99556 if the num of arguments is not 1 infer a raw

It will be available in 5.2.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 6 Caolán McNamara 2016-05-13 12:30:41 UTC
"If the one cannot maintain the code after he had wrote it, the one should not do it at all"

I think I might have written this about 15 years or so ago.
Comment 7 Commit Notification 2016-05-17 07:21:37 UTC
Caolán McNamara committed a patch related to this issue.
It has been pushed to "libreoffice-5-1":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=d4e284ab44f4644a60eea50052f7940098541145&h=libreoffice-5-1

Resolves: tdf#99556 if the num of arguments is not 1 infer a row

It will be available in 5.1.4.

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.