From ba5fb6916446d8bdf79cba25f08d759bc5595aec Mon Sep 17 00:00:00 2001 From: Jay Berkenbilt Date: Tue, 27 Aug 2019 22:10:11 -0400 Subject: [PATCH] Make popping pipeline stack safer Use destructors to pop the pipeline stack, and ensure that code that pops the stack is actually popping the intended thing. --- include/qpdf/Pipeline.hh | 2 + include/qpdf/QPDFWriter.hh | 50 ++++++--- libqpdf/Pipeline.cc | 6 + libqpdf/QPDFWriter.cc | 223 +++++++++++++++++++++---------------- 4 files changed, 171 insertions(+), 110 deletions(-) diff --git a/include/qpdf/Pipeline.hh b/include/qpdf/Pipeline.hh index 36a25df0..e8f62b4e 100644 --- a/include/qpdf/Pipeline.hh +++ b/include/qpdf/Pipeline.hh @@ -70,6 +70,8 @@ class QPDF_DLL_CLASS Pipeline virtual void write(unsigned char* data, size_t len) = 0; QPDF_DLL virtual void finish() = 0; + QPDF_DLL + std::string getIdentifier() const; protected: Pipeline* getNext(bool allow_null = false); diff --git a/include/qpdf/QPDFWriter.hh b/include/qpdf/QPDFWriter.hh index 0fd114db..c3818ae4 100644 --- a/include/qpdf/QPDFWriter.hh +++ b/include/qpdf/QPDFWriter.hh @@ -473,6 +473,34 @@ class QPDFWriter enum trailer_e { t_normal, t_lin_first, t_lin_second }; + // An reference to a PipelinePopper instance is passed into + // activatePipelineStack. When the PipelinePopper goes out of + // scope, the pipeline stack is popped. PipelinePopper's + // destructor calls finish on the current pipeline and pops the + // pipeline stack until the top of stack is a previous active top + // of stack, and restores the pipeline to that point. It deletes + // any pipelines that it pops. If the bp argument is non-null and + // any of the stack items are of type Pl_Buffer, the buffer is + // retrieved. + class PipelinePopper + { + friend class QPDFWriter; + public: + PipelinePopper(QPDFWriter* qw, + PointerHolder* bp = 0) : + qw(qw), + bp(bp) + { + } + ~PipelinePopper(); + + private: + QPDFWriter* qw; + PointerHolder* bp; + std::string stack_id; + }; + friend class PipelinePopper; + unsigned int bytesNeeded(long long n); void writeBinary(unsigned long long val, unsigned int bytes); void writeString(std::string const& str); @@ -560,24 +588,17 @@ class QPDFWriter int calculateXrefStreamPadding(qpdf_offset_t xref_bytes); // When filtering subsections, push additional pipelines to the - // stack. When ready to switch, activate the pipeline stack. - // Pipelines passed to pushPipeline are deleted when - // clearPipelineStack is called. + // stack. When ready to switch, activate the pipeline stack. When + // the passed in PipelinePopper goes out of scope, the stack is + // popped. Pipeline* pushPipeline(Pipeline*); - void activatePipelineStack(); + void activatePipelineStack(PipelinePopper&); void initializePipelineStack(Pipeline *); - // Calls finish on the current pipeline and pops the pipeline - // stack until the top of stack is a previous active top of stack, - // and restores the pipeline to that point. Deletes any pipelines - // that it pops. If the bp argument is non-null and any of the - // stack items are of type Pl_Buffer, the buffer is retrieved. - void popPipelineStack(PointerHolder* bp = 0); - void adjustAESStreamLength(size_t& length); - void pushEncryptionFilter(); - void pushDiscardFilter(); - void pushMD5Pipeline(); + void pushEncryptionFilter(PipelinePopper&); + void pushDiscardFilter(PipelinePopper&); + void pushMD5Pipeline(PipelinePopper&); void computeDeterministicIDData(); void discardGeneration(std::map const& in, @@ -654,6 +675,7 @@ class QPDFWriter std::map object_to_object_stream; std::map > object_stream_to_objects; std::list pipeline_stack; + unsigned long long next_stack_id; bool deterministic_id; Pl_MD5* md5_pipeline; std::string deterministic_id_data; diff --git a/libqpdf/Pipeline.cc b/libqpdf/Pipeline.cc index bcd48e46..bd4fb087 100644 --- a/libqpdf/Pipeline.cc +++ b/libqpdf/Pipeline.cc @@ -31,3 +31,9 @@ Pipeline::getNext(bool allow_null) } return this->m->next; } + +std::string +Pipeline::getIdentifier() const +{ + return this->identifier; +} diff --git a/libqpdf/QPDFWriter.cc b/libqpdf/QPDFWriter.cc index 30bc1fcb..895f98ce 100644 --- a/libqpdf/QPDFWriter.cc +++ b/libqpdf/QPDFWriter.cc @@ -63,6 +63,7 @@ QPDFWriter::Members::Members(QPDF& pdf) : cur_stream_length(0), added_newline(false), max_ostream_index(0), + next_stack_id(0), deterministic_id(false), md5_pipeline(0), did_write_setup(false), @@ -1049,36 +1050,51 @@ QPDFWriter::pushPipeline(Pipeline* p) void QPDFWriter::initializePipelineStack(Pipeline *p) { - this->m->pipeline = new Pl_Count("qpdf count", p); + this->m->pipeline = new Pl_Count("pipeline stack base", p); this->m->to_delete.push_back(this->m->pipeline); this->m->pipeline_stack.push_back(this->m->pipeline); } void -QPDFWriter::activatePipelineStack() +QPDFWriter::activatePipelineStack(PipelinePopper& pp) { - Pl_Count* c = new Pl_Count("count", this->m->pipeline_stack.back()); + std::string stack_id( + "stack " + QUtil::uint_to_string(this->m->next_stack_id)); + Pl_Count* c = new Pl_Count(stack_id.c_str(), + this->m->pipeline_stack.back()); + ++this->m->next_stack_id; this->m->pipeline_stack.push_back(c); this->m->pipeline = c; + pp.stack_id = stack_id; } -void -QPDFWriter::popPipelineStack(PointerHolder* bp) +QPDFWriter::PipelinePopper::~PipelinePopper() { - assert(this->m->pipeline_stack.size() >= 2); - this->m->pipeline->finish(); - assert(dynamic_cast(this->m->pipeline_stack.back()) == - this->m->pipeline); - delete this->m->pipeline_stack.back(); - this->m->pipeline_stack.pop_back(); - while (dynamic_cast(this->m->pipeline_stack.back()) == 0) + if (stack_id.empty()) { - Pipeline* p = this->m->pipeline_stack.back(); - if (dynamic_cast(p) == this->m->md5_pipeline) + return; + } + assert(qw->m->pipeline_stack.size() >= 2); + qw->m->pipeline->finish(); + assert(dynamic_cast(qw->m->pipeline_stack.back()) == + qw->m->pipeline); + // It might be possible for this assertion to fail if + // writeLinearized exits by exception when deterministic ID, but I + // don't think so. As of this writing, this is the only case in + // which two dynamically allocated PipelinePopper objects ever + // exist at the same time, so the assertion will fail if they get + // popped out of order from automatic destruction. + assert(qw->m->pipeline->getIdentifier() == stack_id); + delete qw->m->pipeline_stack.back(); + qw->m->pipeline_stack.pop_back(); + while (dynamic_cast(qw->m->pipeline_stack.back()) == 0) + { + Pipeline* p = qw->m->pipeline_stack.back(); + if (dynamic_cast(p) == qw->m->md5_pipeline) { - this->m->md5_pipeline = 0; + qw->m->md5_pipeline = 0; } - this->m->pipeline_stack.pop_back(); + qw->m->pipeline_stack.pop_back(); Pl_Buffer* buf = dynamic_cast(p); if (bp && buf) { @@ -1086,7 +1102,7 @@ QPDFWriter::popPipelineStack(PointerHolder* bp) } delete p; } - this->m->pipeline = dynamic_cast(this->m->pipeline_stack.back()); + qw->m->pipeline = dynamic_cast(qw->m->pipeline_stack.back()); } void @@ -1103,7 +1119,7 @@ QPDFWriter::adjustAESStreamLength(size_t& length) } void -QPDFWriter::pushEncryptionFilter() +QPDFWriter::pushEncryptionFilter(PipelinePopper& pp) { if (this->m->encrypted && (! this->m->cur_data_key.empty())) { @@ -1125,18 +1141,18 @@ QPDFWriter::pushEncryptionFilter() } // Must call this unconditionally so we can call popPipelineStack // to balance pushEncryptionFilter(). - activatePipelineStack(); + activatePipelineStack(pp); } void -QPDFWriter::pushDiscardFilter() +QPDFWriter::pushDiscardFilter(PipelinePopper& pp) { pushPipeline(new Pl_Discard()); - activatePipelineStack(); + activatePipelineStack(pp); } void -QPDFWriter::pushMD5Pipeline() +QPDFWriter::pushMD5Pipeline(PipelinePopper& pp) { if (! this->m->id2.empty()) { @@ -1153,7 +1169,7 @@ QPDFWriter::pushMD5Pipeline() // Special case code in popPipelineStack clears this->m->md5_pipeline // upon deletion. pushPipeline(this->m->md5_pipeline); - activatePipelineStack(); + activatePipelineStack(pp); } void @@ -1769,8 +1785,8 @@ QPDFWriter::unparseObject(QPDFObjectHandle object, int level, for (int attempt = 1; attempt <= 2; ++attempt) { pushPipeline(new Pl_Buffer("stream data")); - activatePipelineStack(); - + PipelinePopper pp_stream_data(this, &stream_data); + activatePipelineStack(pp_stream_data); filtered = object.pipeStreamData( this->m->pipeline, @@ -1779,7 +1795,6 @@ QPDFWriter::unparseObject(QPDFObjectHandle object, int level, (filter ? (uncompress ? qpdf_dl_all : this->m->stream_decode_level) : qpdf_dl_none), false, (attempt == 1)); - popPipelineStack(&stream_data); if (filter && (! filtered)) { // Try again @@ -1808,11 +1823,14 @@ QPDFWriter::unparseObject(QPDFObjectHandle object, int level, adjustAESStreamLength(this->m->cur_stream_length); unparseObject(stream_dict, 0, flags, this->m->cur_stream_length, compress); + unsigned char last_char = '\0'; writeString("\nstream\n"); - pushEncryptionFilter(); - writeBuffer(stream_data); - unsigned char last_char = this->m->pipeline->getLastChar(); - popPipelineStack(); + { + PipelinePopper pp_enc(this); + pushEncryptionFilter(pp_enc); + writeBuffer(stream_data); + last_char = this->m->pipeline->getLastChar(); + } if (this->m->newline_before_endstream || (this->m->qdf_mode && (last_char != '\n'))) @@ -1911,9 +1929,11 @@ QPDFWriter::writeObjectStream(QPDFObjectHandle object) bool compressed = false; for (int pass = 1; pass <= 2; ++pass) { + // stream_buffer will be initialized only for pass 2 + PipelinePopper pp_ostream(this, &stream_buffer); if (pass == 1) { - pushDiscardFilter(); + pushDiscardFilter(pp_ostream); } else { @@ -1928,10 +1948,12 @@ QPDFWriter::writeObjectStream(QPDFObjectHandle object) // Take one pass at writing pairs of numbers so we can get // their size information - pushDiscardFilter(); - writeObjectStreamOffsets(offsets, first_obj); - first += this->m->pipeline->getCount(); - popPipelineStack(); + { + PipelinePopper pp_discard(this); + pushDiscardFilter(pp_discard); + writeObjectStreamOffsets(offsets, first_obj); + first += this->m->pipeline->getCount(); + } // Set up a stream to write the stream data into a buffer. Pipeline* next = pushPipeline(new Pl_Buffer("object stream")); @@ -1944,7 +1966,7 @@ QPDFWriter::writeObjectStream(QPDFObjectHandle object) new Pl_Flate("compress object stream", next, Pl_Flate::a_deflate)); } - activatePipelineStack(); + activatePipelineStack(pp_ostream); writeObjectStreamOffsets(offsets, first_obj); } @@ -1994,9 +2016,6 @@ QPDFWriter::writeObjectStream(QPDFObjectHandle object) this->m->xref[new_obj] = QPDFXRefEntry(2, new_id, count); } - - // stream_buffer will be initialized only for pass 2 - popPipelineStack(&stream_buffer); } // Write the object @@ -2037,9 +2056,11 @@ QPDFWriter::writeObjectStream(QPDFObjectHandle object) { QTC::TC("qpdf", "QPDFWriter encrypt object stream"); } - pushEncryptionFilter(); - writeBuffer(stream_buffer); - popPipelineStack(); + { + PipelinePopper pp_enc(this); + pushEncryptionFilter(pp_enc); + writeBuffer(stream_buffer); + } if (this->m->newline_before_endstream) { writeString("\n"); @@ -2779,10 +2800,13 @@ QPDFWriter::writeHintStream(int hint_id) { QTC::TC("qpdf", "QPDFWriter encrypted hint stream"); } - pushEncryptionFilter(); - writeBuffer(hint_buffer); - unsigned char last_char = this->m->pipeline->getLastChar(); - popPipelineStack(); + unsigned char last_char = '\0'; + { + PipelinePopper pp_enc(this); + pushEncryptionFilter(pp_enc); + writeBuffer(hint_buffer); + last_char = this->m->pipeline->getLastChar(); + } if (last_char != '\n') { @@ -2896,46 +2920,48 @@ QPDFWriter::writeXRefStream(int xref_id, int max_id, qpdf_offset_t max_offset, new Pl_PNGFilter( "pngify xref", p, Pl_PNGFilter::a_encode, esize)); } - activatePipelineStack(); - for (int i = first; i <= last; ++i) - { - QPDFXRefEntry& e = this->m->xref[i]; - switch (e.getType()) - { - case 0: - writeBinary(0, 1); - writeBinary(0, f1_size); - writeBinary(0, f2_size); - break; - - case 1: - { - qpdf_offset_t offset = e.getOffset(); - if ((hint_id != 0) && - (i != hint_id) && - (offset >= hint_offset)) - { - offset += hint_length; - } - writeBinary(1, 1); - writeBinary(QIntC::to_ulonglong(offset), f1_size); - writeBinary(0, f2_size); - } - break; - - case 2: - writeBinary(2, 1); - writeBinary(QIntC::to_ulonglong(e.getObjStreamNumber()), f1_size); - writeBinary(QIntC::to_ulonglong(e.getObjStreamIndex()), f2_size); - break; - - default: - throw std::logic_error("invalid type writing xref stream"); - break; - } - } PointerHolder xref_data; - popPipelineStack(&xref_data); + { + PipelinePopper pp_xref(this, &xref_data); + activatePipelineStack(pp_xref); + for (int i = first; i <= last; ++i) + { + QPDFXRefEntry& e = this->m->xref[i]; + switch (e.getType()) + { + case 0: + writeBinary(0, 1); + writeBinary(0, f1_size); + writeBinary(0, f2_size); + break; + + case 1: + { + qpdf_offset_t offset = e.getOffset(); + if ((hint_id != 0) && + (i != hint_id) && + (offset >= hint_offset)) + { + offset += hint_length; + } + writeBinary(1, 1); + writeBinary(QIntC::to_ulonglong(offset), f1_size); + writeBinary(0, f2_size); + } + break; + + case 2: + writeBinary(2, 1); + writeBinary(QIntC::to_ulonglong(e.getObjStreamNumber()), f1_size); + writeBinary(QIntC::to_ulonglong(e.getObjStreamIndex()), f2_size); + break; + + default: + throw std::logic_error("invalid type writing xref stream"); + break; + } + } + } openObject(xref_id); writeString("<<"); @@ -3156,6 +3182,8 @@ QPDFWriter::writeLinearized() // Write file in two passes. Part numbers refer to PDF spec 1.4. FILE* lin_pass1_file = 0; + PointerHolder pp_pass1 = new PipelinePopper(this); + PointerHolder pp_md5 = new PipelinePopper(this); for (int pass = 1; pass <= 2; ++pass) { if (pass == 1) @@ -3167,15 +3195,15 @@ QPDFWriter::writeLinearized() this->m->lin_pass1_filename.c_str(), "wb"); pushPipeline( new Pl_StdioFile("linearization pass1", lin_pass1_file)); - activatePipelineStack(); + activatePipelineStack(*pp_pass1); } else { - pushDiscardFilter(); + pushDiscardFilter(*pp_pass1); } if (this->m->deterministic_id) { - pushMD5Pipeline(); + pushMD5Pipeline(*pp_md5); } } @@ -3392,23 +3420,25 @@ QPDFWriter::writeLinearized() QTC::TC("qpdf", "QPDFWriter linearized deterministic ID", need_xref_stream ? 0 : 1); computeDeterministicIDData(); - popPipelineStack(); + pp_md5 = 0; assert(this->m->md5_pipeline == 0); } // Close first pass pipeline file_size = this->m->pipeline->getCount(); - popPipelineStack(); + pp_pass1 = 0; // Save hint offset since it will be set to zero by // calling openObject. qpdf_offset_t hint_offset = this->m->xref[hint_id].getOffset(); // Write hint stream to a buffer - pushPipeline(new Pl_Buffer("hint buffer")); - activatePipelineStack(); - writeHintStream(hint_id); - popPipelineStack(&hint_buffer); + { + pushPipeline(new Pl_Buffer("hint buffer")); + PipelinePopper pp_hint(this, &hint_buffer); + activatePipelineStack(pp_hint); + writeHintStream(hint_id); + } hint_length = QIntC::to_offset(hint_buffer->getSize()); // Restore hint offset @@ -3541,9 +3571,10 @@ QPDFWriter::registerProgressReporter(PointerHolder pr) void QPDFWriter::writeStandard() { + PointerHolder pp_md5 = new PipelinePopper(this); if (this->m->deterministic_id) { - pushMD5Pipeline(); + pushMD5Pipeline(*pp_md5); } // Start writing @@ -3597,7 +3628,7 @@ QPDFWriter::writeStandard() { QTC::TC("qpdf", "QPDFWriter standard deterministic ID", this->m->object_stream_to_objects.empty() ? 0 : 1); - popPipelineStack(); + pp_md5 = 0; assert(this->m->md5_pipeline == 0); } }