From 1b5875043dafd5b900eb14676bdce0eee235b2ae Mon Sep 17 00:00:00 2001 From: Georg Baum Date: Thu, 2 Apr 2015 21:35:05 +0200 Subject: [PATCH] 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. --- src/mathed/MathMacro.cpp | 70 +++++++++++++++++++++++++--------------- 1 file changed, 44 insertions(+), 26 deletions(-) diff --git a/src/mathed/MathMacro.cpp b/src/mathed/MathMacro.cpp index 13d6e0e110..1158e4a4e4 100644 --- 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(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 -- 2.39.5