Fix math macro crash (bug 9418)

In the test case the crash occured in mathml export of the temporary buffer,
because the macro was updated, and because one of the used other macros was
not copied, the macro argument was detached. However, the underlying problem
of the crash was a broken ArgumentProxy::mathMacro_ reference which became
invalid each time the ownng MathMacro was copied. In the bug test case the
copying happened due to resizing a std::vector, but any other copy would have
created the same problem. The crash did not always happen, because sometimes
the old freed memory was not immediately reused, so the invalid reference did
still point to usable data.

The fix is easy: Convert ArgumentProxy::mathMacro_ to a pointer and update it
always after creating a copy of the owner. The pimpl of MathMacro from the
previous commit helps here to distinguish between the data that can be
automatically copied (in MathMacro::Private) and the cleanup that needs to be
done manually (in MathMacro). This way, the manual copy constructor and
assigment operator of MathMacro does not need to be touched if a new member is
added.

diff --git a/src/mathed/MathMacro.cpp b/src/mathed/MathMacro.cpp
index 13d6e0e1103a465627a898d0a3c889e226930c17..1158e4a4e4f074c3b5f3326cf999704539d28a81 100644 (file)
--- a/src/mathed/MathMacro.cpp
+++ b/src/mathed/MathMacro.cpp
@@ -52,44 +52,46 @@ namespace lyx {
 class ArgumentProxy : public InsetMath {
 public:
        ///
-       ArgumentProxy(MathMacro & mathMacro, size_t idx)
+       ArgumentProxy(MathMacro * mathMacro, size_t idx)
                : mathMacro_(mathMacro), idx_(idx) {}
        ///
-       ArgumentProxy(MathMacro & mathMacro, size_t idx, docstring const & def)
+       ArgumentProxy(MathMacro * mathMacro, size_t idx, docstring const & def)
                : mathMacro_(mathMacro), idx_(idx)
        {
                        asArray(def, def_);
        }
        ///
+       void setOwner(MathMacro * mathMacro) { mathMacro_ = mathMacro; }
+       ///
        InsetCode lyxCode() const { return ARGUMENT_PROXY_CODE; }
        ///
        void metrics(MetricsInfo & mi, Dimension & dim) const {
-               mathMacro_.macro()->unlock();
-               mathMacro_.cell(idx_).metrics(mi, dim);
+               mathMacro_->macro()->unlock();
+               mathMacro_->cell(idx_).metrics(mi, dim);
 
-               if (!mathMacro_.editMetrics(mi.base.bv)
-                   && mathMacro_.cell(idx_).empty())
+               if (!mathMacro_->editMetrics(mi.base.bv)
+                   && mathMacro_->cell(idx_).empty())
                        def_.metrics(mi, dim);
 
-               mathMacro_.macro()->lock();
+               mathMacro_->macro()->lock();
        }
        // write(), normalize(), infoize() and infoize2() are not needed since
        // MathMacro uses the definition and not the expanded cells.
        ///
-       void maple(MapleStream & ms) const { ms << mathMacro_.cell(idx_); }
+       void maple(MapleStream & ms) const { ms << mathMacro_->cell(idx_); }
        ///
-       void maxima(MaximaStream & ms) const { ms << mathMacro_.cell(idx_); }
+       void maxima(MaximaStream & ms) const { ms << mathMacro_->cell(idx_); }
        ///
-       void mathematica(MathematicaStream & ms) const { ms << mathMacro_.cell(idx_); }
+       void mathematica(MathematicaStream & ms) const { ms << mathMacro_->cell(idx_); }
        ///
-       void mathmlize(MathStream & ms) const { ms << mathMacro_.cell(idx_); }
+       void mathmlize(MathStream & ms) const { ms << mathMacro_->cell(idx_); }
        ///
-       void htmlize(HtmlStream & ms) const { ms << mathMacro_.cell(idx_); }
+       void htmlize(HtmlStream & ms) const { ms << mathMacro_->cell(idx_); }
        ///
-       void octave(OctaveStream & os) const { os << mathMacro_.cell(idx_); }
+       void octave(OctaveStream & os) const { os << mathMacro_->cell(idx_); }
        ///
        void draw(PainterInfo & pi, int x, int y) const {
-               if (mathMacro_.editMetrics(pi.base.bv)) {
+               if (mathMacro_->editMetrics(pi.base.bv)) {
                        // The only way a ArgumentProxy can appear is in a cell of the
                        // MathMacro. Moreover the cells are only drawn in the DISPLAY_FOLDED
                        // mode and then, if the macro is edited the monochrome
@@ -99,22 +101,22 @@ public:
                        // here (and the assert triggers in pain.leaveMonochromeMode())
                        // it's a bug.
                        pi.pain.leaveMonochromeMode();
-                       mathMacro_.cell(idx_).draw(pi, x, y);
+                       mathMacro_->cell(idx_).draw(pi, x, y);
                        pi.pain.enterMonochromeMode(Color_mathbg, Color_mathmacroblend);
-               } else if (mathMacro_.cell(idx_).empty()) {
-                       mathMacro_.cell(idx_).setXY(*pi.base.bv, x, y);
+               } else if (mathMacro_->cell(idx_).empty()) {
+                       mathMacro_->cell(idx_).setXY(*pi.base.bv, x, y);
                        def_.draw(pi, x, y);
                } else
-                       mathMacro_.cell(idx_).draw(pi, x, y);
+                       mathMacro_->cell(idx_).draw(pi, x, y);
        }
        ///
        size_t idx() const { return idx_; }
        ///
        int kerning(BufferView const * bv) const
        {
-               if (mathMacro_.editMetrics(bv)
-                   || !mathMacro_.cell(idx_).empty())
-                       return mathMacro_.cell(idx_).kerning(bv);
+               if (mathMacro_->editMetrics(bv)
+                   || !mathMacro_->cell(idx_).empty())
+                       return mathMacro_->cell(idx_).kerning(bv);
                else
                        return def_.kerning(bv);
        }
@@ -126,7 +128,7 @@ private:
                return new ArgumentProxy(*this);
        }
        ///
-       MathMacro & mathMacro_;
+       MathMacro * mathMacro_;
        ///
        size_t idx_;
        ///
@@ -134,6 +136,7 @@ private:
 };
 
 
+/// Private implementation of MathMacro
 class MathMacro::Private {
 public:
        Private(Buffer * buf, docstring const & name)
@@ -144,6 +147,10 @@ public:
                  appetite_(9)
        {
        }
+       /// Update the pointers to our owner of all expanded macros.
+       /// This needs to be called every time a copy of the owner is created
+       /// (bug 9418).
+       void updateChildren(MathMacro * owner);
        /// name of macro
        docstring name_;
        /// current display mode
@@ -177,6 +184,16 @@ public:
 };
 
 
+void MathMacro::Private::updateChildren(MathMacro * owner)
+{
+       for (size_t i = 0; i < expanded_.size(); ++i) {
+               ArgumentProxy * p = dynamic_cast<ArgumentProxy *>(expanded_[i].nucleus());
+               if (p)
+                       p->setOwner(owner);
+       }
+}
+
+
 MathMacro::MathMacro(Buffer * buf, docstring const & name)
        : InsetMathNest(buf, 0), d(new Private(buf, name))
 {}
@@ -185,6 +202,7 @@ MathMacro::MathMacro(Buffer * buf, docstring const & name)
 MathMacro::MathMacro(MathMacro const & that)
        : InsetMathNest(that), d(new Private(*that.d))
 {
+       d->updateChildren(this);
 }
 
 
@@ -194,6 +212,7 @@ MathMacro & MathMacro::operator=(MathMacro const & that)
                return *this;
        InsetMathNest::operator=(that);
        *d = *that.d;
+       d->updateChildren(this);
        return *this;
 }
 
@@ -300,8 +319,7 @@ void MathMacro::metrics(MetricsInfo & mi, Dimension & dim) const
        d->editing_[mi.base.bv] = editMode(mi.base.bv);
 
        // calculate new metrics according to display mode
-       if (d->displayMode_ == DISPLAY_INIT ||
-           d->displayMode_ == DISPLAY_INTERACTIVE_INIT) {
+       if (d->displayMode_ == DISPLAY_INIT || d->displayMode_ == DISPLAY_INTERACTIVE_INIT) {
                mathed_string_dim(mi.base.font, from_ascii("\\") + name(), dim);
        } else if (d->displayMode_ == DISPLAY_UNFOLDED) {
                cell(0).metrics(mi, dim);
@@ -461,9 +479,9 @@ void MathMacro::updateRepresentation(Cursor * cur, MacroContext const & mc,
        for (size_t i = 0; i < nargs(); ++i) {
                ArgumentProxy * proxy;
                if (i < defaults.size())
-                       proxy = new ArgumentProxy(*this, i, defaults[i]);
+                       proxy = new ArgumentProxy(this, i, defaults[i]);
                else
-                       proxy = new ArgumentProxy(*this, i);
+                       proxy = new ArgumentProxy(this, i);
                values[i].insert(0, MathAtom(proxy));
        }
        // expanding macro with the values