From 66d7e7cb60756e316fdb993db5e928a81ab984a2 Mon Sep 17 00:00:00 2001 From: Richard Kimberly Heck Date: Sat, 25 Apr 2020 22:17:51 -0400 Subject: [PATCH] Try to fix bug #6505. Keep track of nested includes and just refuse to re-enter a file we're already in the process of handling. There's a question whether we should do this in updateBuffer and validate, or whether we should do it separately. For now, this seems to work. --- src/Buffer.cpp | 50 ++++++++++- src/insets/InsetInclude.cpp | 165 +++++++++++++++++++----------------- src/insets/InsetInclude.h | 4 + 3 files changed, 137 insertions(+), 82 deletions(-) diff --git a/src/Buffer.cpp b/src/Buffer.cpp index ebb302f3ec..8b5fae1802 100644 --- a/src/Buffer.cpp +++ b/src/Buffer.cpp @@ -1095,6 +1095,10 @@ bool Buffer::readDocument(Lexer & lex) d->old_position = params().origin; else d->old_position = filePath(); + + if (!parent()) + clearIncludeList(); + bool const res = text().read(lex, errorList, d->inset); d->old_position.clear(); @@ -2400,6 +2404,9 @@ void Buffer::validate(LaTeXFeatures & features) const if (!features.runparams().is_child) params().validate(features); + if (!parent()) + clearIncludeList(); + for (Paragraph const & p : paragraphs()) p.validate(features); @@ -2612,6 +2619,9 @@ void Buffer::reloadBibInfoCache(bool const force) const void Buffer::collectBibKeys(FileNameList & checkedFiles) const { + if (!parent()) + clearIncludeList(); + for (InsetIterator it = inset_iterator_begin(inset()); it; ++it) { it->collectBibKeys(it, checkedFiles); if (it->lyxCode() == BIBITEM_CODE) { @@ -3474,8 +3484,37 @@ bool Buffer::isReadonly() const void Buffer::setParent(Buffer const * buffer) { - // Avoids recursive include. - d->setParent(buffer == this ? nullptr : buffer); + // We need to do some work here to avoid recursive parent structures. + // This is the easy case. + if (buffer == this) { + LYXERR0("Ignoring attempt to set self as parent in\n" << fileName()); + return; + } + // Now we check parents going upward, to make sure that IF we set the + // parent as requested, we would not generate a recursive include. + set sb; + Buffer const * b = buffer; + bool found_recursion = false; + while (b) { + if (sb.find(b) != sb.end()) { + found_recursion = true; + break; + } + sb.insert(b); + b = b->parent(); + } + + if (found_recursion) { + LYXERR0("Ignoring attempt to set parent of\n" << + fileName() << + "\nto " << + buffer->fileName() << + "\nbecause that would create a recursive inclusion."); + return; + } + + // We should be safe now. + d->setParent(buffer); updateMacros(); } @@ -3496,8 +3535,6 @@ ListOfBuffers Buffer::allRelatives() const Buffer const * Buffer::masterBuffer() const { - // FIXME Should be make sure we are not in some kind - // of recursive include? A -> B -> A will crash this. Buffer const * const pbuf = d->parent(); if (!pbuf) return this; @@ -4950,6 +4987,8 @@ void Buffer::updateBuffer(UpdateScope scope, UpdateType utype) const // do the real work ParIterator parit = cbuf.par_iterator_begin(); + if (scope == UpdateMaster) + clearIncludeList(); updateBuffer(parit, utype); // If this document has siblings, then update the TocBackend later. The @@ -4992,6 +5031,7 @@ void Buffer::updateBuffer(UpdateScope scope, UpdateType utype) const } d->cite_labels_valid_ = true; /// FIXME: Perf + clearIncludeList(); cbuf.tocBackend().update(true, utype); if (scope == UpdateMaster) cbuf.structureChanged(); @@ -5222,6 +5262,7 @@ void Buffer::Impl::setLabel(ParIterator & it, UpdateType utype) const void Buffer::updateBuffer(ParIterator & parit, UpdateType utype, bool const deleted) const { + pushIncludedBuffer(this); // LASSERT: Is it safe to continue here, or should we just return? LASSERT(parit.pit() == 0, /**/); @@ -5277,6 +5318,7 @@ void Buffer::updateBuffer(ParIterator & parit, UpdateType utype, bool const dele // set change indicator for the inset (or the cell that the iterator // points to, if applicable). parit.text()->inset().isChanged(changed); + popIncludedBuffer(); } diff --git a/src/insets/InsetInclude.cpp b/src/insets/InsetInclude.cpp index b0d2be4f03..be823a77d5 100644 --- a/src/insets/InsetInclude.cpp +++ b/src/insets/InsetInclude.cpp @@ -189,7 +189,8 @@ char_type replaceCommaInBraces(docstring & params) InsetInclude::InsetInclude(Buffer * buf, InsetCommandParams const & p) : InsetCommand(buf, p), include_label(uniqueID()), preview_(make_unique(this)), failedtoload_(false), - set_label_(false), label_(nullptr), child_buffer_(nullptr), file_exist_(false) + set_label_(false), label_(nullptr), child_buffer_(nullptr), file_exist_(false), + recursion_error_(false) { preview_->connect([=](){ fileChanged(); }); @@ -205,7 +206,7 @@ InsetInclude::InsetInclude(InsetInclude const & other) : InsetCommand(other), include_label(other.include_label), preview_(make_unique(this)), failedtoload_(false), set_label_(false), label_(nullptr), child_buffer_(nullptr), - file_exist_(other.file_exist_) + file_exist_(other.file_exist_),recursion_error_(other.recursion_error_) { preview_->connect([=](){ fileChanged(); }); @@ -379,6 +380,7 @@ void InsetInclude::setParams(InsetCommandParams const & p) // reset in order to allow loading new file failedtoload_ = false; + recursion_error_ = false; InsetCommand::setParams(p); set_label_ = false; @@ -446,12 +448,7 @@ docstring InsetInclude::screenLabel() const Buffer * InsetInclude::getChildBuffer() const { - Buffer * childBuffer = loadIfNeeded(); - - // FIXME RECURSIVE INCLUDE - // This isn't sufficient, as the inclusion could be downstream. - // But it'll have to do for now. - return (childBuffer == &buffer()) ? nullptr : childBuffer; + return loadIfNeeded(); } @@ -482,6 +479,9 @@ Buffer * InsetInclude::loadIfNeeded() const return nullptr; Buffer * child = theBufferList().getBuffer(included_file); + if (checkForRecursiveInclude(child)) + return child; + if (!child) { // the readonly flag can/will be wrong, not anymore I think. if (!included_file.exists()) { @@ -494,6 +494,7 @@ Buffer * InsetInclude::loadIfNeeded() const // Buffer creation is not possible. return nullptr; + buffer().pushIncludedBuffer(child); // Set parent before loading, such that macros can be tracked child->setParent(&buffer()); @@ -502,9 +503,11 @@ Buffer * InsetInclude::loadIfNeeded() const child->setParent(nullptr); //close the buffer we just opened theBufferList().release(child); + buffer().popIncludedBuffer(); return nullptr; } + buffer().popIncludedBuffer(); if (!child->errorList("Parse").empty()) { // FIXME: Do something. } @@ -527,6 +530,26 @@ Buffer * InsetInclude::loadIfNeeded() const } +bool InsetInclude::checkForRecursiveInclude( + Buffer const * cbuf, bool silent) const +{ + if (recursion_error_) + return true; + + if (!buffer().isBufferIncluded(cbuf)) + return false; + + if (!silent) { + docstring const msg = _("The file\n%1$s\n has attempted to include itself.\n" + "The document set will not work properly until this is fixed!"); + frontend::Alert::warning(_("Recursive Include"), + bformat(msg, from_utf8(cbuf->fileName().absFileName()))); + } + recursion_error_ = true; + return true; +} + + void InsetInclude::latex(otexstream & os, OutputParams const & runparams) const { string incfile = ltrim(to_utf8(params()["filename"])); @@ -551,20 +574,6 @@ void InsetInclude::latex(otexstream & os, OutputParams const & runparams) const } FileName const included_file = includedFileName(buffer(), params()); - - // Check we're not trying to include ourselves. - // FIXME RECURSIVE INCLUDE - // This isn't sufficient, as the inclusion could be downstream. - // But it'll have to do for now. - if (isInputOrInclude(params()) && - buffer().absFileName() == included_file.absFileName()) - { - Alert::error(_("Recursive input"), - bformat(_("Attempted to include file %1$s in itself! " - "Ignoring inclusion."), from_utf8(incfile))); - return; - } - Buffer const * const masterBuffer = buffer().masterBuffer(); // if incfile is relative, make it relative to the master @@ -803,6 +812,9 @@ void InsetInclude::latex(otexstream & os, OutputParams const & runparams) const return; } + if (recursion_error_) + return; + if (!runparams.silent) { if (tmp->params().baseClass() != masterBuffer->params().baseClass()) { // FIXME UNICODE @@ -966,19 +978,13 @@ docstring InsetInclude::xhtml(XHTMLStream & xs, OutputParams const & rp) const // In the other cases, we will generate the HTML and include it. - // Check we're not trying to include ourselves. - // FIXME RECURSIVE INCLUDE - if (buffer().absFileName() == included_file.absFileName()) { - Alert::error(_("Recursive input"), - bformat(_("Attempted to include file %1$s in itself! " - "Ignoring inclusion."), ltrim(params()["filename"]))); - return docstring(); - } - Buffer const * const ibuf = loadIfNeeded(); if (!ibuf) return docstring(); + if (recursion_error_) + return docstring(); + // are we generating only some paragraphs, or all of them? bool const all_pars = !rp.dryrun || (rp.par_begin == 0 && @@ -995,6 +1001,7 @@ docstring InsetInclude::xhtml(XHTMLStream & xs, OutputParams const & rp) const << from_utf8(included_file.absFileName()) << XHTMLStream::ESCAPE_NONE << " -->"; + return docstring(); } @@ -1029,6 +1036,10 @@ int InsetInclude::plaintext(odocstringstream & os, os << str; return str.size(); } + + if (recursion_error_) + return 0; + writePlaintextFile(*ibuf, os, op); return 0; } @@ -1043,18 +1054,6 @@ int InsetInclude::docbook(odocstream & os, OutputParams const & runparams) const return 0; string const included_file = includedFileName(buffer(), params()).absFileName(); - - // Check we're not trying to include ourselves. - // FIXME RECURSIVE INCLUDE - // This isn't sufficient, as the inclusion could be downstream. - // But it'll have to do for now. - if (buffer().absFileName() == included_file) { - Alert::error(_("Recursive input"), - bformat(_("Attempted to include file %1$s in itself! " - "Ignoring inclusion."), from_utf8(incfile))); - return 0; - } - string exppath = incfile; if (!runparams.export_folder.empty()) { exppath = makeAbsPath(exppath, runparams.export_folder).realPath(); @@ -1067,6 +1066,9 @@ int InsetInclude::docbook(odocstream & os, OutputParams const & runparams) const Buffer * tmp = loadIfNeeded(); if (tmp) { + if (recursion_error_) + return 0; + string const mangled = writefile.mangledFileName(); writefile = makeAbsPath(mangled, buffer().masterBuffer()->temppath()); @@ -1136,40 +1138,41 @@ void InsetInclude::validate(LaTeXFeatures & features) const // Load the file in the include if it needs // to be loaded: Buffer * const tmp = loadIfNeeded(); - if (tmp) { - // the file is loaded - // make sure the buffer isn't us - // FIXME RECURSIVE INCLUDES - // This is not sufficient, as recursive includes could be - // more than a file away. But it will do for now. - if (tmp && tmp != &buffer()) { - // We must temporarily change features.buffer, - // otherwise it would always be the master buffer, - // and nested includes would not work. - features.setBuffer(*tmp); - // Maybe this is already a child - bool const is_child = - features.runparams().is_child; - features.runparams().is_child = true; - tmp->validate(features); - features.runparams().is_child = is_child; - features.setBuffer(buffer()); - } - } + if (!tmp) + return; + + // the file is loaded + if (checkForRecursiveInclude(tmp)) + return; + buffer().pushIncludedBuffer(tmp); + + // We must temporarily change features.buffer, + // otherwise it would always be the master buffer, + // and nested includes would not work. + features.setBuffer(*tmp); + // Maybe this is already a child + bool const is_child = + features.runparams().is_child; + features.runparams().is_child = true; + tmp->validate(features); + features.runparams().is_child = is_child; + features.setBuffer(buffer()); + + buffer().popIncludedBuffer(); } void InsetInclude::collectBibKeys(InsetIterator const & /*di*/, FileNameList & checkedFiles) const { - Buffer * child = loadIfNeeded(); - if (!child) + Buffer * ibuf = loadIfNeeded(); + if (!ibuf) return; - // FIXME RECURSIVE INCLUDE - // This isn't sufficient, as the inclusion could be downstream. - // But it'll have to do for now. - if (child == &buffer()) + + if (checkForRecursiveInclude(ibuf)) return; - child->collectBibKeys(checkedFiles); + buffer().pushIncludedBuffer(ibuf); + ibuf->collectBibKeys(checkedFiles); + buffer().popIncludedBuffer(); } @@ -1189,7 +1192,7 @@ void InsetInclude::metrics(MetricsInfo & mi, Dimension & dim) const } else { if (!set_label_) { set_label_ = true; - button_.update(screenLabel(), true, false, !file_exist_); + button_.update(screenLabel(), true, false, !file_exist_ || recursion_error_); } button_.metrics(mi, dim); } @@ -1338,15 +1341,19 @@ void InsetInclude::addToToc(DocIterator const & cpit, bool output_active, if (!childbuffer) return; + if (checkForRecursiveInclude(childbuffer)) + return; + buffer().pushIncludedBuffer(childbuffer); // Update the child's tocBackend. The outliner uses the master's, but // the navigation menu uses the child's. childbuffer->tocBackend().update(output_active, utype); // Include Tocs from children childbuffer->inset().addToToc(DocIterator(), output_active, utype, backend); - //Copy missing outliner names (though the user has been warned against - //having different document class and module selection between master - //and child). + buffer().popIncludedBuffer(); + // Copy missing outliner names (though the user has been warned against + // having different document class and module selection between master + // and child). for (auto const & name : childbuffer->params().documentClass().outlinerNames()) backend.addName(name.first, translateIfPossible(name.second)); @@ -1379,14 +1386,16 @@ void InsetInclude::updateCommand() void InsetInclude::updateBuffer(ParIterator const & it, UpdateType utype, bool const deleted) { file_exist_ = includedFileExist(); - - button_.update(screenLabel(), true, false, !file_exist_); - Buffer const * const childbuffer = getChildBuffer(); if (childbuffer) { - childbuffer->updateBuffer(Buffer::UpdateChildOnly, utype); + if (!checkForRecursiveInclude(childbuffer)) + childbuffer->updateBuffer(Buffer::UpdateChildOnly, utype); + button_.update(screenLabel(), true, false, recursion_error_); return; } + + button_.update(screenLabel(), true, false, !file_exist_); + if (!isListings(params())) return; diff --git a/src/insets/InsetInclude.h b/src/insets/InsetInclude.h index 6ba8b63982..6636583e6c 100644 --- a/src/insets/InsetInclude.h +++ b/src/insets/InsetInclude.h @@ -139,6 +139,9 @@ private: bool isChildIncluded() const; /// check whether the included file exist bool includedFileExist() const; + /// \return True if there is a recursive include + /// Also issues appropriate warning, etc + bool checkForRecursiveInclude(Buffer const * cbuf, bool silent = false) const; /// \name Private functions inherited from Inset class //@{ @@ -173,6 +176,7 @@ private: InsetLabel * label_; mutable Buffer * child_buffer_; mutable bool file_exist_; + mutable bool recursion_error_; }; -- 2.39.5