From 6c39aa87638f7a6f96a97627ac112fb2022bd3a7 Mon Sep 17 00:00:00 2001 From: Jay Berkenbilt Date: Sat, 22 Jun 2019 14:24:49 -0400 Subject: In shippable code, favor smart pointers (fixes #235) Use PointerHolder in several places where manually memory allocation and deallocation were being used. This helps to protect against memory leaks when exceptions are thrown in surprising places. --- ChangeLog | 3 +++ include/qpdf/ClosedFileInputSource.hh | 2 +- include/qpdf/Pl_Flate.hh | 2 +- libqpdf/ClosedFileInputSource.cc | 9 +++------ libqpdf/Pl_AES_PDF.cc | 24 ++++++++++++++---------- libqpdf/Pl_Flate.cc | 17 +++++++---------- libqpdf/Pl_PNGFilter.cc | 20 ++++++++++---------- libqpdf/Pl_RC4.cc | 12 +++++------- libqpdf/Pl_TIFFPredictor.cc | 17 ++++++++--------- libqpdf/QPDFWriter.cc | 8 ++++---- libqpdf/QPDF_encryption.cc | 5 +++-- libqpdf/QUtil.cc | 7 +++---- libqpdf/qpdf-c.cc | 24 ++++++++---------------- libqpdf/qpdf/Pl_AES_PDF.hh | 4 ++-- libqpdf/qpdf/Pl_PNGFilter.hh | 8 ++++---- libqpdf/qpdf/Pl_RC4.hh | 2 +- libqpdf/qpdf/Pl_TIFFPredictor.hh | 2 +- zlib-flate/zlib-flate.cc | 7 +++---- 18 files changed, 81 insertions(+), 92 deletions(-) diff --git a/ChangeLog b/ChangeLog index 1d9018df..1dc18fec 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,8 @@ 2019-06-22 Jay Berkenbilt + * Favor PointerHolder over manual memory allocation in shippable + code where possible. Fixes #235. + * If pkg-config is available, use it to local libjpeg and zlib. If not, fall back to old behavior. Fixes #324. diff --git a/include/qpdf/ClosedFileInputSource.hh b/include/qpdf/ClosedFileInputSource.hh index ee8557fc..09b63bbd 100644 --- a/include/qpdf/ClosedFileInputSource.hh +++ b/include/qpdf/ClosedFileInputSource.hh @@ -82,7 +82,7 @@ class ClosedFileInputSource: public InputSource std::string filename; qpdf_offset_t offset; - FileInputSource* fis; + PointerHolder fis; bool stay_open; }; PointerHolder m; diff --git a/include/qpdf/Pl_Flate.hh b/include/qpdf/Pl_Flate.hh index 2884ecc5..f25adfbb 100644 --- a/include/qpdf/Pl_Flate.hh +++ b/include/qpdf/Pl_Flate.hh @@ -58,7 +58,7 @@ class Pl_Flate: public Pipeline Members(size_t out_bufsize, action_e action); Members(Members const&); - unsigned char* outbuf; + PointerHolder outbuf; size_t out_bufsize; action_e action; bool initialized; diff --git a/libqpdf/ClosedFileInputSource.cc b/libqpdf/ClosedFileInputSource.cc index e9c9b3bd..f68a9a31 100644 --- a/libqpdf/ClosedFileInputSource.cc +++ b/libqpdf/ClosedFileInputSource.cc @@ -4,14 +4,12 @@ ClosedFileInputSource::Members::Members(char const* filename) : filename(filename), offset(0), - fis(0), stay_open(false) { } ClosedFileInputSource::Members::~Members() { - delete fis; } ClosedFileInputSource::ClosedFileInputSource(char const* filename) : @@ -26,7 +24,7 @@ ClosedFileInputSource::~ClosedFileInputSource() void ClosedFileInputSource::before() { - if (0 == this->m->fis) + if (0 == this->m->fis.getPointer()) { this->m->fis = new FileInputSource(); this->m->fis->setFilename(this->m->filename.c_str()); @@ -44,7 +42,6 @@ ClosedFileInputSource::after() { return; } - delete this->m->fis; this->m->fis = 0; } @@ -84,7 +81,7 @@ void ClosedFileInputSource::rewind() { this->m->offset = 0; - if (this->m->fis) + if (this->m->fis.getPointer()) { this->m->fis->rewind(); } @@ -112,7 +109,7 @@ void ClosedFileInputSource::stayOpen(bool val) { this->m->stay_open = val; - if ((! val) && this->m->fis) + if ((! val) && this->m->fis.getPointer()) { after(); } diff --git a/libqpdf/Pl_AES_PDF.cc b/libqpdf/Pl_AES_PDF.cc index c2c921e6..1099ab13 100644 --- a/libqpdf/Pl_AES_PDF.cc +++ b/libqpdf/Pl_AES_PDF.cc @@ -25,29 +25,31 @@ Pl_AES_PDF::Pl_AES_PDF(char const* identifier, Pipeline* next, { size_t keybits = 8 * key_bytes; assert(key_bytes == KEYLENGTH(keybits)); - this->key = new unsigned char[key_bytes]; - this->rk = new uint32_t[RKLENGTH(keybits)]; + this->key = PointerHolder( + true, new unsigned char[key_bytes]); + this->rk = PointerHolder( + true, new uint32_t[RKLENGTH(keybits)]); size_t rk_bytes = RKLENGTH(keybits) * sizeof(uint32_t); - std::memcpy(this->key, key, key_bytes); - std::memset(this->rk, 0, rk_bytes); + std::memcpy(this->key.getPointer(), key, key_bytes); + std::memset(this->rk.getPointer(), 0, rk_bytes); std::memset(this->inbuf, 0, this->buf_size); std::memset(this->outbuf, 0, this->buf_size); std::memset(this->cbc_block, 0, this->buf_size); if (encrypt) { - this->nrounds = rijndaelSetupEncrypt(this->rk, this->key, keybits); + this->nrounds = rijndaelSetupEncrypt( + this->rk.getPointer(), this->key.getPointer(), keybits); } else { - this->nrounds = rijndaelSetupDecrypt(this->rk, this->key, keybits); + this->nrounds = rijndaelSetupDecrypt( + this->rk.getPointer(), this->key.getPointer(), keybits); } assert(this->nrounds == NROUNDS(keybits)); } Pl_AES_PDF::~Pl_AES_PDF() { - delete [] this->key; - delete [] this->rk; } void @@ -222,7 +224,8 @@ Pl_AES_PDF::flush(bool strip_padding) this->inbuf[i] ^= this->cbc_block[i]; } } - rijndaelEncrypt(this->rk, this->nrounds, this->inbuf, this->outbuf); + rijndaelEncrypt(this->rk.getPointer(), + this->nrounds, this->inbuf, this->outbuf); if (this->cbc_mode) { memcpy(this->cbc_block, this->outbuf, this->buf_size); @@ -230,7 +233,8 @@ Pl_AES_PDF::flush(bool strip_padding) } else { - rijndaelDecrypt(this->rk, this->nrounds, this->inbuf, this->outbuf); + rijndaelDecrypt(this->rk.getPointer(), + this->nrounds, this->inbuf, this->outbuf); if (this->cbc_mode) { for (unsigned int i = 0; i < this->buf_size; ++i) diff --git a/libqpdf/Pl_Flate.cc b/libqpdf/Pl_Flate.cc index c42d3c28..a782255b 100644 --- a/libqpdf/Pl_Flate.cc +++ b/libqpdf/Pl_Flate.cc @@ -13,7 +13,8 @@ Pl_Flate::Members::Members(size_t out_bufsize, initialized(false), zdata(0) { - this->outbuf = new unsigned char[out_bufsize]; + this->outbuf = PointerHolder( + true, new unsigned char[out_bufsize]); // Indirect through zdata to reach the z_stream so we don't have // to include zlib.h in Pl_Flate.hh. This means people using // shared library versions of qpdf don't have to have zlib @@ -34,15 +35,12 @@ Pl_Flate::Members::Members(size_t out_bufsize, zstream.opaque = 0; zstream.next_in = 0; zstream.avail_in = 0; - zstream.next_out = this->outbuf; + zstream.next_out = this->outbuf.getPointer(); zstream.avail_out = QIntC::to_uint(out_bufsize); } Pl_Flate::Members::~Members() { - delete [] this->outbuf; - this->outbuf = 0; - if (this->initialized) { z_stream& zstream = *(static_cast(this->zdata)); @@ -74,7 +72,7 @@ Pl_Flate::~Pl_Flate() void Pl_Flate::write(unsigned char* data, size_t len) { - if (this->m->outbuf == 0) + if (this->m->outbuf.getPointer() == 0) { throw std::logic_error( this->identifier + @@ -186,8 +184,8 @@ Pl_Flate::handleData(unsigned char* data, size_t len, int flush) QIntC::to_ulong(this->m->out_bufsize - zstream.avail_out); if (ready > 0) { - this->getNext()->write(this->m->outbuf, ready); - zstream.next_out = this->m->outbuf; + this->getNext()->write(this->m->outbuf.getPointer(), ready); + zstream.next_out = this->m->outbuf.getPointer(); zstream.avail_out = QIntC::to_uint(this->m->out_bufsize); } } @@ -205,7 +203,7 @@ Pl_Flate::finish() { try { - if (this->m->outbuf) + if (this->m->outbuf.getPointer()) { if (this->m->initialized) { @@ -226,7 +224,6 @@ Pl_Flate::finish() checkError("End", err); } - delete [] this->m->outbuf; this->m->outbuf = 0; } } diff --git a/libqpdf/Pl_PNGFilter.cc b/libqpdf/Pl_PNGFilter.cc index a39c17f9..6a3bc5b8 100644 --- a/libqpdf/Pl_PNGFilter.cc +++ b/libqpdf/Pl_PNGFilter.cc @@ -45,12 +45,14 @@ Pl_PNGFilter::Pl_PNGFilter(char const* identifier, Pipeline* next, "PNGFilter created with invalid columns value"); } this->bytes_per_row = bpr & UINT_MAX; - this->buf1 = new unsigned char[this->bytes_per_row + 1]; - this->buf2 = new unsigned char[this->bytes_per_row + 1]; - memset(this->buf1, 0, this->bytes_per_row + 1); - memset(this->buf2, 0, this->bytes_per_row + 1); - this->cur_row = this->buf1; - this->prev_row = this->buf2; + this->buf1 = PointerHolder( + true, new unsigned char[this->bytes_per_row + 1]); + this->buf2 = PointerHolder( + true, new unsigned char[this->bytes_per_row + 1]); + memset(this->buf1.getPointer(), 0, this->bytes_per_row + 1); + memset(this->buf2.getPointer(), 0, this->bytes_per_row + 1); + this->cur_row = this->buf1.getPointer(); + this->prev_row = this->buf2.getPointer(); // number of bytes per incoming row this->incoming = (action == a_encode ? @@ -60,8 +62,6 @@ Pl_PNGFilter::Pl_PNGFilter(char const* identifier, Pipeline* next, Pl_PNGFilter::~Pl_PNGFilter() { - delete [] buf1; - delete [] buf2; } void @@ -81,7 +81,7 @@ Pl_PNGFilter::write(unsigned char* data, size_t len) // Swap rows unsigned char* t = this->prev_row; this->prev_row = this->cur_row; - this->cur_row = t ? t : this->buf2; + this->cur_row = t ? t : this->buf2.getPointer(); memset(this->cur_row, 0, this->bytes_per_row + 1); left = this->incoming; this->pos = 0; @@ -269,7 +269,7 @@ Pl_PNGFilter::finish() processRow(); } this->prev_row = 0; - this->cur_row = buf1; + this->cur_row = buf1.getPointer(); this->pos = 0; memset(this->cur_row, 0, this->bytes_per_row + 1); diff --git a/libqpdf/Pl_RC4.cc b/libqpdf/Pl_RC4.cc index 407490ca..6c8fd3c6 100644 --- a/libqpdf/Pl_RC4.cc +++ b/libqpdf/Pl_RC4.cc @@ -8,19 +8,18 @@ Pl_RC4::Pl_RC4(char const* identifier, Pipeline* next, out_bufsize(out_bufsize), rc4(key_data, key_len) { - this->outbuf = new unsigned char[out_bufsize]; + this->outbuf = PointerHolder( + true, new unsigned char[out_bufsize]); } Pl_RC4::~Pl_RC4() { - delete [] this->outbuf; - this->outbuf = 0; } void Pl_RC4::write(unsigned char* data, size_t len) { - if (this->outbuf == 0) + if (this->outbuf.getPointer() == 0) { throw std::logic_error( this->identifier + @@ -35,16 +34,15 @@ Pl_RC4::write(unsigned char* data, size_t len) size_t bytes = (bytes_left < this->out_bufsize ? bytes_left : out_bufsize); bytes_left -= bytes; - rc4.process(p, bytes, outbuf); + rc4.process(p, bytes, outbuf.getPointer()); p += bytes; - getNext()->write(outbuf, bytes); + getNext()->write(outbuf.getPointer(), bytes); } } void Pl_RC4::finish() { - delete [] this->outbuf; this->outbuf = 0; this->getNext()->finish(); } diff --git a/libqpdf/Pl_TIFFPredictor.cc b/libqpdf/Pl_TIFFPredictor.cc index 42cfbb1a..30487511 100644 --- a/libqpdf/Pl_TIFFPredictor.cc +++ b/libqpdf/Pl_TIFFPredictor.cc @@ -16,7 +16,6 @@ Pl_TIFFPredictor::Pl_TIFFPredictor(char const* identifier, Pipeline* next, columns(columns), samples_per_pixel(samples_per_pixel), bits_per_sample(bits_per_sample), - cur_row(0), pos(0) { if (samples_per_pixel < 1) @@ -38,13 +37,13 @@ Pl_TIFFPredictor::Pl_TIFFPredictor(char const* identifier, Pipeline* next, "TIFFPredictor created with invalid columns value"); } this->bytes_per_row = bpr & UINT_MAX; - this->cur_row = new unsigned char[this->bytes_per_row]; - memset(this->cur_row, 0, this->bytes_per_row); + this->cur_row = PointerHolder( + true, new unsigned char[this->bytes_per_row]); + memset(this->cur_row.getPointer(), 0, this->bytes_per_row); } Pl_TIFFPredictor::~Pl_TIFFPredictor() { - delete [] cur_row; } void @@ -55,20 +54,20 @@ Pl_TIFFPredictor::write(unsigned char* data, size_t len) while (len >= left) { // finish off current row - memcpy(this->cur_row + this->pos, data + offset, left); + memcpy(this->cur_row.getPointer() + this->pos, data + offset, left); offset += left; len -= left; processRow(); // Prepare for next row - memset(this->cur_row, 0, this->bytes_per_row); + memset(this->cur_row.getPointer(), 0, this->bytes_per_row); left = this->bytes_per_row; this->pos = 0; } if (len) { - memcpy(this->cur_row + this->pos, data + offset, len); + memcpy(this->cur_row.getPointer() + this->pos, data + offset, len); } this->pos += len; } @@ -79,7 +78,7 @@ Pl_TIFFPredictor::processRow() QTC::TC("libtests", "Pl_TIFFPredictor processRow", (action == a_decode ? 0 : 1)); BitWriter bw(this->getNext()); - BitStream in(this->cur_row, this->bytes_per_row); + BitStream in(this->cur_row.getPointer(), this->bytes_per_row); std::vector prev; for (unsigned int i = 0; i < this->samples_per_pixel; ++i) { @@ -118,6 +117,6 @@ Pl_TIFFPredictor::finish() processRow(); } this->pos = 0; - memset(this->cur_row, 0, this->bytes_per_row); + memset(this->cur_row.getPointer(), 0, this->bytes_per_row); getNext()->finish(); } diff --git a/libqpdf/QPDFWriter.cc b/libqpdf/QPDFWriter.cc index 7702dc34..41d5ab30 100644 --- a/libqpdf/QPDFWriter.cc +++ b/libqpdf/QPDFWriter.cc @@ -1835,21 +1835,21 @@ QPDFWriter::unparseObject(QPDFObjectHandle object, int level, this->m->cur_data_key.length()); pl.write(QUtil::unsigned_char_pointer(val), val.length()); pl.finish(); - Buffer* buf = bufpl.getBuffer(); + PointerHolder buf = bufpl.getBuffer(); val = QPDF_String( std::string(reinterpret_cast(buf->getBuffer()), buf->getSize())).unparse(true); - delete buf; } else { - char* tmp = QUtil::copy_string(val); + PointerHolder tmp_ph = + PointerHolder(true, QUtil::copy_string(val)); + char* tmp = tmp_ph.getPointer(); size_t vlen = val.length(); RC4 rc4(QUtil::unsigned_char_pointer(this->m->cur_data_key), QIntC::to_int(this->m->cur_data_key.length())); rc4.process(QUtil::unsigned_char_pointer(tmp), vlen); val = QPDF_String(std::string(tmp, vlen)).unparse(); - delete [] tmp; } } else diff --git a/libqpdf/QPDF_encryption.cc b/libqpdf/QPDF_encryption.cc index d72d6fb7..b5f16e0c 100644 --- a/libqpdf/QPDF_encryption.cc +++ b/libqpdf/QPDF_encryption.cc @@ -204,7 +204,9 @@ iterate_rc4(unsigned char* data, size_t data_len, unsigned char* okey, int key_len, int iterations, bool reverse) { - unsigned char* key = new unsigned char[QIntC::to_size(key_len)]; + PointerHolder key_ph = PointerHolder( + true, new unsigned char[QIntC::to_size(key_len)]); + unsigned char* key = key_ph.getPointer(); for (int i = 0; i < iterations; ++i) { int const xor_value = (reverse ? iterations - 1 - i : i); @@ -215,7 +217,6 @@ iterate_rc4(unsigned char* data, size_t data_len, RC4 rc4(key, QIntC::to_int(key_len)); rc4.process(data, data_len); } - delete [] key; } static std::string diff --git a/libqpdf/QUtil.cc b/libqpdf/QUtil.cc index 151832fb..a3bf1744 100644 --- a/libqpdf/QUtil.cc +++ b/libqpdf/QUtil.cc @@ -670,10 +670,9 @@ QUtil::get_env(std::string const& var, std::string* value) if (value) { - char* t = new char[len + 1]; - ::GetEnvironmentVariable(var.c_str(), t, len); - *value = t; - delete [] t; + PointerHolder t = PointerHolder(true, new char[len + 1]); + ::GetEnvironmentVariable(var.c_str(), t.getPointer(), len); + *value = t.getPointer(); } return true; diff --git a/libqpdf/qpdf-c.cc b/libqpdf/qpdf-c.cc index f42b0e16..fa9045ea 100644 --- a/libqpdf/qpdf-c.cc +++ b/libqpdf/qpdf-c.cc @@ -22,8 +22,8 @@ struct _qpdf_data _qpdf_data(); ~_qpdf_data(); - QPDF* qpdf; - QPDFWriter* qpdf_writer; + PointerHolder qpdf; + PointerHolder qpdf_writer; PointerHolder error; _qpdf_error tmp_error; @@ -36,22 +36,16 @@ struct _qpdf_data unsigned long long size; char const* password; bool write_memory; - Buffer* output_buffer; + PointerHolder output_buffer; }; _qpdf_data::_qpdf_data() : - qpdf(0), - qpdf_writer(0), - write_memory(false), - output_buffer(0) + write_memory(false) { } _qpdf_data::~_qpdf_data() { - delete qpdf_writer; - delete qpdf; - delete output_buffer; } // must set qpdf->filename and qpdf->password @@ -451,14 +445,12 @@ QPDF_BOOL qpdf_allow_modify_all(qpdf_data qpdf) static void qpdf_init_write_internal(qpdf_data qpdf) { - if (qpdf->qpdf_writer) + if (qpdf->qpdf_writer.getPointer()) { QTC::TC("qpdf", "qpdf-c called qpdf_init_write multiple times"); - delete qpdf->qpdf_writer; qpdf->qpdf_writer = 0; - if (qpdf->output_buffer) + if (qpdf->output_buffer.getPointer()) { - delete qpdf->output_buffer; qpdf->output_buffer = 0; qpdf->write_memory = false; qpdf->filename = 0; @@ -496,7 +488,7 @@ size_t qpdf_get_buffer_length(qpdf_data qpdf) { qpdf_get_buffer_internal(qpdf); size_t result = 0; - if (qpdf->output_buffer) + if (qpdf->output_buffer.getPointer()) { result = qpdf->output_buffer->getSize(); } @@ -507,7 +499,7 @@ unsigned char const* qpdf_get_buffer(qpdf_data qpdf) { unsigned char const* result = 0; qpdf_get_buffer_internal(qpdf); - if (qpdf->output_buffer) + if (qpdf->output_buffer.getPointer()) { result = qpdf->output_buffer->getBuffer(); } diff --git a/libqpdf/qpdf/Pl_AES_PDF.hh b/libqpdf/qpdf/Pl_AES_PDF.hh index 4aaf68bb..31cc28f9 100644 --- a/libqpdf/qpdf/Pl_AES_PDF.hh +++ b/libqpdf/qpdf/Pl_AES_PDF.hh @@ -55,8 +55,8 @@ class Pl_AES_PDF: public Pipeline bool cbc_mode; bool first; size_t offset; // offset into memory buffer - unsigned char* key; - uint32_t* rk; + PointerHolder key; + PointerHolder rk; unsigned char inbuf[buf_size]; unsigned char outbuf[buf_size]; unsigned char cbc_block[buf_size]; diff --git a/libqpdf/qpdf/Pl_PNGFilter.hh b/libqpdf/qpdf/Pl_PNGFilter.hh index f6745319..3530a73a 100644 --- a/libqpdf/qpdf/Pl_PNGFilter.hh +++ b/libqpdf/qpdf/Pl_PNGFilter.hh @@ -41,10 +41,10 @@ class Pl_PNGFilter: public Pipeline action_e action; unsigned int bytes_per_row; unsigned int bytes_per_pixel; - unsigned char* cur_row; - unsigned char* prev_row; - unsigned char* buf1; - unsigned char* buf2; + unsigned char* cur_row; // points to buf1 or buf2 + unsigned char* prev_row; // points to buf1 or buf2 + PointerHolder buf1; + PointerHolder buf2; size_t pos; size_t incoming; }; diff --git a/libqpdf/qpdf/Pl_RC4.hh b/libqpdf/qpdf/Pl_RC4.hh index 9885f0ad..250ab2b8 100644 --- a/libqpdf/qpdf/Pl_RC4.hh +++ b/libqpdf/qpdf/Pl_RC4.hh @@ -24,7 +24,7 @@ class Pl_RC4: public Pipeline virtual void finish(); private: - unsigned char* outbuf; + PointerHolder outbuf; size_t out_bufsize; RC4 rc4; }; diff --git a/libqpdf/qpdf/Pl_TIFFPredictor.hh b/libqpdf/qpdf/Pl_TIFFPredictor.hh index b4391f8e..484f4df4 100644 --- a/libqpdf/qpdf/Pl_TIFFPredictor.hh +++ b/libqpdf/qpdf/Pl_TIFFPredictor.hh @@ -32,7 +32,7 @@ class Pl_TIFFPredictor: public Pipeline unsigned int bytes_per_row; unsigned int samples_per_pixel; unsigned int bits_per_sample; - unsigned char* cur_row; + PointerHolder cur_row; size_t pos; }; diff --git a/zlib-flate/zlib-flate.cc b/zlib-flate/zlib-flate.cc index 9d4a62e2..d1c74d4d 100644 --- a/zlib-flate/zlib-flate.cc +++ b/zlib-flate/zlib-flate.cc @@ -61,8 +61,9 @@ int main(int argc, char* argv[]) QUtil::binary_stdout(); QUtil::binary_stdin(); - Pl_StdioFile* out = new Pl_StdioFile("stdout", stdout); - Pl_Flate* flate = new Pl_Flate("flate", out, action); + PointerHolder out = new Pl_StdioFile("stdout", stdout); + PointerHolder flate = + new Pl_Flate("flate", out.getPointer(), action); try { @@ -81,8 +82,6 @@ int main(int argc, char* argv[]) } } flate->finish(); - delete flate; - delete out; } catch (std::exception& e) { -- cgit v1.2.3-54-g00ecf