In shippable code, favor smart pointers (fixes qpdf#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

@@ -1,5 +1,8 @@
 2019-06-22  Jay Berkenbilt  <[email protected]>
 
+	* 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.
 

include/qpdf/ClosedFileInputSource.hh

@@ -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;

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<unsigned char> outbuf;
         size_t out_bufsize;
         action_e action;
         bool initialized;

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();
     }

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<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,15 +224,17 @@ 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);
 	}
     }
     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)

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<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;
         }
     }

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<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);
 

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<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();
 }

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<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();
 }

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<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