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.

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.
Jay Berkenbilt
1 parent 12400475
Showing 18 changed files with 81 additions and 92 deletions
ChangeLog
include/qpdf/ClosedFileInputSource.hh
include/qpdf/Pl_Flate.hh
libqpdf/ClosedFileInputSource.cc
libqpdf/Pl_AES_PDF.cc
libqpdf/Pl_Flate.cc
libqpdf/Pl_PNGFilter.cc
libqpdf/Pl_RC4.cc
libqpdf/Pl_TIFFPredictor.cc
libqpdf/QPDFWriter.cc
libqpdf/QPDF_encryption.cc
libqpdf/QUtil.cc
libqpdf/qpdf-c.cc
libqpdf/qpdf/Pl_AES_PDF.hh
libqpdf/qpdf/Pl_PNGFilter.hh
libqpdf/qpdf/Pl_RC4.hh
libqpdf/qpdf/Pl_TIFFPredictor.hh
zlib-flate/zlib-flate.cc
 2019-06-22  Jay Berkenbilt  <ejb@ql.org>
+	* 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.
@@ -82,7 +82,7 @@ class ClosedFileInputSource: public InputSource
         std::string filename;
         qpdf_offset_t offset;
-        FileInputSource* fis;
+        PointerHolder<FileInputSource> fis;
         bool stay_open;
     };
     PointerHolder<Members> m;
@@ -58,7 +58,7 @@ class Pl_Flate: public Pipeline
         Members(size_t out_bufsize, action_e action);
         Members(Members const&);
-        unsigned char* outbuf;
+        PointerHolder<unsigned char> outbuf;
         size_t out_bufsize;
         action_e action;
         bool initialized;
@@ -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();
     }
@@ -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<unsigned char>(
+        true, new unsigned char[key_bytes]);
+    this->rk = PointerHolder<uint32_t>(
+        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)
@@ -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<unsigned char>(
+        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<z_stream*>(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;
         }
     }
@@ -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<unsigned char>(
+        true, new unsigned char[this->bytes_per_row + 1]);
+    this->buf2 = PointerHolder<unsigned char>(
+        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);
@@ -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<unsigned char>(
+        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();
 }
@@ -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<unsigned char>(
+        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<long long> 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();
 }
@@ -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<Buffer> buf = bufpl.getBuffer();
 		val = QPDF_String(
 		    std::string(reinterpret_cast<char*>(buf->getBuffer()),
 				buf->getSize())).unparse(true);
-		delete buf;
 	    }
 	    else
 	    {
-		char* tmp = QUtil::copy_string(val);
+		PointerHolder<char> tmp_ph =
+                    PointerHolder<char>(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
@@ -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<unsigned char> key_ph = PointerHolder<unsigned char>(
+        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
@@ -670,10 +670,9 @@ QUtil::get_env(std::string const&amp; var, std::string* value)
     if (value)
     {
-	char* t = new char[len + 1];
-        ::GetEnvironmentVariable(var.c_str(), t, len);
-	*value = t;
-	delete [] t;
+	PointerHolder<char> t = PointerHolder<char>(true, new char[len + 1]);
+        ::GetEnvironmentVariable(var.c_str(), t.getPointer(), len);
+	*value = t.getPointer();
     }
     return true;
@@ -22,8 +22,8 @@ struct _qpdf_data
     _qpdf_data();
     ~_qpdf_data();
-    QPDF* qpdf;
-    QPDFWriter* qpdf_writer;
+    PointerHolder<QPDF> qpdf;
+    PointerHolder<QPDFWriter> qpdf_writer;
     PointerHolder<QPDFExc> 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<Buffer> 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();
     }
@@ -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<unsigned char> key;
+    PointerHolder<uint32_t> rk;
     unsigned char inbuf[buf_size];
     unsigned char outbuf[buf_size];
     unsigned char cbc_block[buf_size];
@@ -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<unsigned char> buf1;
+    PointerHolder<unsigned char> buf2;
     size_t pos;
     size_t incoming;
 };
@@ -24,7 +24,7 @@ class Pl_RC4: public Pipeline
     virtual void finish();
   private:
-    unsigned char* outbuf;
+    PointerHolder<unsigned char> outbuf;
     size_t out_bufsize;
     RC4 rc4;
 };
@@ -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<unsigned char> cur_row;
     size_t pos;
 };
@@ -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<Pl_StdioFile> out = new Pl_StdioFile("stdout", stdout);
+    PointerHolder<Pl_Flate> 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)
     {