From 5f3f78822b5d43e9b02082da5268d186ba7101c0 Mon Sep 17 00:00:00 2001 From: Jay Berkenbilt Date: Sat, 5 Feb 2022 08:15:07 -0500 Subject: [PATCH] Improve use of std::unique_ptr * Use unique_ptr in place of shared_ptr in some cases * unique_ptr for arrays does not require a custom deleter * use std::make_unique (c++14) where possible --- ChangeLog | 6 ++++++ TODO | 10 ++++++++++ include/qpdf/QUtil.hh | 4 ++++ libqpdf/AES_PDF_native.cc | 8 ++------ libqpdf/Pl_AES_PDF.cc | 4 +--- libqpdf/QPDFArgParser.cc | 2 +- libqpdf/QPDFJob.cc | 2 +- libqpdf/QPDFWriter.cc | 2 +- libqpdf/QPDF_encryption.cc | 2 +- libqpdf/QUtil.cc | 18 +++++++++++++----- libqpdf/qpdfjob-c.cc | 2 +- libtests/qtest/qutil/qutil.out | 1 + libtests/qutil.cc | 13 +++++++++++-- libtests/rc4.cc | 3 +-- 14 files changed, 54 insertions(+), 23 deletions(-) diff --git a/ChangeLog b/ChangeLog index 803f4dec..db2faa7f 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +2022-02-05 Jay Berkenbilt + + * Add QUtil::make_unique_cstr to return a std::unique_ptr + as an alternative to QUtil::copy_string and + QUtil::make_shared_cstr. + 2022-02-04 Jay Berkenbilt * New preprocessor symbols QPDF_MAJOR_VERSION, QPDF_MINOR_VERSION, diff --git a/TODO b/TODO index 222e200a..8166f090 100644 --- a/TODO +++ b/TODO @@ -1,6 +1,9 @@ 10.6 ==== +* Expose emptyPDF to the C API. Ensure that qpdf_get_qpdf_version is + always static. + * Consider doing one big commit to reformat the entire codebase using clang-format or a similar tool. Consider using blame.ignoreRevsFile or similar (or otherwise study git blame to see how to minimize the @@ -364,6 +367,13 @@ auto x_ph = std::make_shared(); X* x = x_ph.get(); Derived* x = new Derived(); PointerHolder x_ph(x) --> Derived* x = new Derived(); auto x_ph = std::shared_pointer(x); +Also remember + +auto x = std::shared_ptr(new T[5], std::default_delete()) +vs. +auto x = std::make_unique(5) + + PointerHolder in public API: QUtil::read_file_into_memory( diff --git a/include/qpdf/QUtil.hh b/include/qpdf/QUtil.hh index 6ff90c45..927fc1d8 100644 --- a/include/qpdf/QUtil.hh +++ b/include/qpdf/QUtil.hh @@ -161,6 +161,10 @@ namespace QUtil QPDF_DLL std::shared_ptr make_shared_cstr(std::string const&); + // Copy string as a unique_ptr to an array. + QPDF_DLL + std::unique_ptr make_unique_cstr(std::string const&); + // Returns lower-case hex-encoded version of the string, treating // each character in the input string as unsigned. The output // string will be twice as long as the input string. diff --git a/libqpdf/AES_PDF_native.cc b/libqpdf/AES_PDF_native.cc index 26df9540..afbc0bdc 100644 --- a/libqpdf/AES_PDF_native.cc +++ b/libqpdf/AES_PDF_native.cc @@ -19,12 +19,8 @@ AES_PDF_native::AES_PDF_native(bool encrypt, unsigned char const* key, nrounds(0) { size_t keybits = 8 * key_bytes; - this->key = std::unique_ptr( - new unsigned char[key_bytes], - std::default_delete()); - this->rk = std::unique_ptr( - new uint32_t[RKLENGTH(keybits)], - std::default_delete()); + this->key = std::make_unique(key_bytes); + this->rk = std::make_unique(RKLENGTH(keybits)); size_t rk_bytes = RKLENGTH(keybits) * sizeof(uint32_t); std::memcpy(this->key.get(), key, key_bytes); std::memset(this->rk.get(), 0, rk_bytes); diff --git a/libqpdf/Pl_AES_PDF.cc b/libqpdf/Pl_AES_PDF.cc index 6a58a321..de1f666f 100644 --- a/libqpdf/Pl_AES_PDF.cc +++ b/libqpdf/Pl_AES_PDF.cc @@ -25,9 +25,7 @@ Pl_AES_PDF::Pl_AES_PDF(char const* identifier, Pipeline* next, use_specified_iv(false), disable_padding(false) { - this->key = std::unique_ptr( - new unsigned char[key_bytes], - std::default_delete()); + this->key = std::make_unique(key_bytes); std::memcpy(this->key.get(), key, key_bytes); std::memset(this->inbuf, 0, this->buf_size); std::memset(this->outbuf, 0, this->buf_size); diff --git a/libqpdf/QPDFArgParser.cc b/libqpdf/QPDFArgParser.cc index 7e0ce7e4..b1658fea 100644 --- a/libqpdf/QPDFArgParser.cc +++ b/libqpdf/QPDFArgParser.cc @@ -20,7 +20,7 @@ QPDFArgParser::Members::Members( option_table(nullptr), final_check_handler(nullptr) { - auto tmp = QUtil::make_shared_cstr(argv[0]); + auto tmp = QUtil::make_unique_cstr(argv[0]); char* p = QUtil::getWhoami(tmp.get()); // Remove prefix added by libtool for consistency during testing. if (strncmp(p, "lt-", 3) == 0) diff --git a/libqpdf/QPDFJob.cc b/libqpdf/QPDFJob.cc index 41166f9e..646692d5 100644 --- a/libqpdf/QPDFJob.cc +++ b/libqpdf/QPDFJob.cc @@ -3379,7 +3379,7 @@ QPDFJob::setEncryptionOptions(QPDF& pdf, QPDFWriter& w) static void parse_version(std::string const& full_version_string, std::string& version, int& extension_level) { - auto vp = QUtil::make_shared_cstr(full_version_string); + auto vp = QUtil::make_unique_cstr(full_version_string); char* v = vp.get(); char* p1 = strchr(v, '.'); char* p2 = (p1 ? strchr(1 + p1, '.') : 0); diff --git a/libqpdf/QPDFWriter.cc b/libqpdf/QPDFWriter.cc index 66bf677a..5f7b39a5 100644 --- a/libqpdf/QPDFWriter.cc +++ b/libqpdf/QPDFWriter.cc @@ -1919,7 +1919,7 @@ QPDFWriter::unparseObject(QPDFObjectHandle object, int level, } else { - auto tmp_ph = QUtil::make_shared_cstr(val); + auto tmp_ph = QUtil::make_unique_cstr(val); char* tmp = tmp_ph.get(); size_t vlen = val.length(); RC4 rc4(QUtil::unsigned_char_pointer(this->m->cur_data_key), diff --git a/libqpdf/QPDF_encryption.cc b/libqpdf/QPDF_encryption.cc index 54c2dadc..5ce63e4c 100644 --- a/libqpdf/QPDF_encryption.cc +++ b/libqpdf/QPDF_encryption.cc @@ -1211,7 +1211,7 @@ QPDF::decryptString(std::string& str, int objid, int generation) size_t vlen = str.length(); // Using PointerHolder guarantees that tmp will // be freed even if rc4.process throws an exception. - auto tmp = QUtil::make_shared_cstr(str); + auto tmp = QUtil::make_unique_cstr(str); RC4 rc4(QUtil::unsigned_char_pointer(key), toI(key.length())); rc4.process(QUtil::unsigned_char_pointer(tmp.get()), vlen); str = std::string(tmp.get(), vlen); diff --git a/libqpdf/QUtil.cc b/libqpdf/QUtil.cc index e3666d02..503d79f9 100644 --- a/libqpdf/QUtil.cc +++ b/libqpdf/QUtil.cc @@ -744,6 +744,16 @@ QUtil::make_shared_cstr(std::string const& str) return result; } +std::unique_ptr +QUtil::make_unique_cstr(std::string const& str) +{ + auto result = std::make_unique(str.length() + 1); + // Use memcpy in case string contains nulls + result.get()[str.length()] = '\0'; + memcpy(result.get(), str.c_str(), str.length()); + return result; +} + std::string QUtil::hex_encode(std::string const& input) { @@ -2625,7 +2635,7 @@ call_main_from_wmain(bool, int argc, wchar_t const* const argv[], // other systems. That way the rest of qpdf.cc can just act like // arguments are UTF-8. - std::vector> utf8_argv; + std::vector> utf8_argv; for (int i = 0; i < argc; ++i) { std::string utf16; @@ -2638,11 +2648,9 @@ call_main_from_wmain(bool, int argc, wchar_t const* const argv[], QIntC::to_uchar(codepoint & 0xff))); } std::string utf8 = QUtil::utf16_to_utf8(utf16); - utf8_argv.push_back(QUtil::make_shared_cstr(utf8)); + utf8_argv.push_back(QUtil::make_unique_cstr(utf8)); } - auto utf8_argv_sp = - std::shared_ptr( - new char*[1+utf8_argv.size()], std::default_delete()); + auto utf8_argv_sp = std::make_unique(1+utf8_argv.size()); char** new_argv = utf8_argv_sp.get(); for (size_t i = 0; i < utf8_argv.size(); ++i) { diff --git a/libqpdf/qpdfjob-c.cc b/libqpdf/qpdfjob-c.cc index 10e13043..98bd82df 100644 --- a/libqpdf/qpdfjob-c.cc +++ b/libqpdf/qpdfjob-c.cc @@ -9,7 +9,7 @@ int qpdfjob_run_from_argv(char const* const argv[]) { - auto whoami_p = QUtil::make_shared_cstr(argv[0]); + auto whoami_p = QUtil::make_unique_cstr(argv[0]); auto whoami = QUtil::getWhoami(whoami_p.get()); QUtil::setLineBuf(stdout); diff --git a/libtests/qtest/qutil/qutil.out b/libtests/qtest/qutil/qutil.out index 58cc2334..8bab099b 100644 --- a/libtests/qtest/qutil/qutil.out +++ b/libtests/qtest/qutil/qutil.out @@ -23,6 +23,7 @@ one 7 compare okay compare okay +compare okay -2147483648 to int: PASSED 2147483647 to int: PASSED 2147483648 to int threw (integer out of range converting 2147483648 from a 8-byte signed type to a 4-byte signed type): PASSED diff --git a/libtests/qutil.cc b/libtests/qutil.cc index a74ae4fe..dcea9fd6 100644 --- a/libtests/qutil.cc +++ b/libtests/qutil.cc @@ -150,7 +150,7 @@ void string_conversion_test() std::cout << "compare failed" << std::endl; } delete [] tmp; - // Also test with make_shared_cstr + // Also test with make_shared_cstr and make_unique_cstr auto tmp2 = QUtil::make_shared_cstr(embedded_null); if (memcmp(tmp2.get(), embedded_null.c_str(), 7) == 0) { @@ -160,6 +160,15 @@ void string_conversion_test() { std::cout << "compare failed" << std::endl; } + auto tmp3 = QUtil::make_unique_cstr(embedded_null); + if (memcmp(tmp3.get(), embedded_null.c_str(), 7) == 0) + { + std::cout << "compare okay" << std::endl; + } + else + { + std::cout << "compare failed" << std::endl; + } std::string int_max_str = QUtil::int_to_string(INT_MAX); std::string int_min_str = QUtil::int_to_string(INT_MIN); @@ -417,7 +426,7 @@ void transcoding_test() void print_whoami(char const* str) { - auto dup = QUtil::make_shared_cstr(str); + auto dup = QUtil::make_unique_cstr(str); std::cout << QUtil::getWhoami(dup.get()) << std::endl; } diff --git a/libtests/rc4.cc b/libtests/rc4.cc index d3f1c7fa..b8abce88 100644 --- a/libtests/rc4.cc +++ b/libtests/rc4.cc @@ -14,8 +14,7 @@ static void other_tests() // Test cases not covered by the pipeline: string as key, convert // in place RC4 r(reinterpret_cast("quack")); - auto data = std::unique_ptr( - new unsigned char[6], std::default_delete()); + auto data = std::make_unique(6); memcpy(data.get(), "potato", 6); r.process(data.get(), 6); assert(memcmp(data.get(), "\xa5\x6f\xe7\x27\x2b\x5c", 6) == 0);