Obscure bug fix copying foreign streams in special cases (fixes #449)

Specifically, if a stream had its stream data replaced and had
indirect /Filter or /DecodeParms, it would result in non-silent loss
of data and/or internal error.

9 files changed, 54 insertions, 14 deletions

diff --git a/ChangeLog b/ChangeLog
index 81812449..4a59b6a7 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,11 @@
 2020-10-21  Jay Berkenbilt  <ejb@ql.org>
 
+	* Bug fix: properly handle copying foreign streams that have
+	indirect /Filter or /DecodeParms keys when stream data has been
+	replaced. The circumstances leading to this bug are very unusual
+	but would cause qpdf to either generate an internal error or some
+	other kind of warning situation if it would occur. Fixes #449.
+
 	* Qpdf's build and CI has been migrated from Azure Pipelines
 	(Azure Devops) to GitHub Actions.
 
diff --git a/TODO b/TODO
index 32cbd901..3f4e0976 100644
--- a/TODO
+++ b/TODO
@@ -7,7 +7,6 @@ Candidates for upcoming release
 * Open "next" issues
   * bugs
     * #473: zsh completion with directories
-    * #449: internal error with case to reproduce (from pikepdf)
     * #444: concatenated stream/whitespace bug
   * Non-bugs
     * #446: recognize edited QDF files
diff --git a/libqpdf/QPDF.cc b/libqpdf/QPDF.cc
index 1cbef133..3b6d70ee 100644
--- a/libqpdf/QPDF.cc
+++ b/libqpdf/QPDF.cc
@@ -2313,7 +2313,8 @@ QPDF::reserveObjects(QPDFObjectHandle foreign, ObjCopier& obj_copier,
     if (foreign.isReserved())
     {
         throw std::logic_error(
-            "QPDF: attempting to copy a foreign reserved object");
+            "QPDF: attempting to copy a foreign reserved object: " +
+            QUtil::int_to_string(foreign.getObjectID()));
     }
 
     if (foreign.isPagesObject())
@@ -2486,6 +2487,16 @@ QPDF::replaceForeignIndirectObjects(
         }
         PointerHolder<Buffer> stream_buffer =
             stream->getStreamDataBuffer();
+        // Note: at this stage, dictionary keys may still be reserved.
+        // We have to handle that explicitly if we access anything.
+        auto get_as_direct = [&old_dict] (std::string const& key) {
+            QPDFObjectHandle obj = old_dict.getKey(key);
+            obj.makeDirect();
+            return obj;
+        };
+
+        QPDFObjectHandle filter = get_as_direct("/Filter");
+        QPDFObjectHandle decode_parms = get_as_direct("/DecodeParms");
         if ((foreign_stream_qpdf->m->immediate_copy_from) &&
             (stream_buffer.getPointer() == 0))
         {
@@ -2495,8 +2506,7 @@ QPDF::replaceForeignIndirectObjects(
             // have to keep duplicating the memory.
             QTC::TC("qpdf", "QPDF immediate copy stream data");
             foreign.replaceStreamData(foreign.getRawStreamData(),
-                                      dict.getKey("/Filter"),
-                                      dict.getKey("/DecodeParms"));
+                                      filter, decode_parms);
             stream_buffer = stream->getStreamDataBuffer();
         }
         PointerHolder<QPDFObjectHandle::StreamDataProvider> stream_provider =
@@ -2504,9 +2514,7 @@ QPDF::replaceForeignIndirectObjects(
         if (stream_buffer.getPointer())
         {
             QTC::TC("qpdf", "QPDF copy foreign stream with buffer");
-            result.replaceStreamData(stream_buffer,
-                                     dict.getKey("/Filter"),
-                                     dict.getKey("/DecodeParms"));
+            result.replaceStreamData(stream_buffer, filter, decode_parms);
         }
         else if (stream_provider.getPointer())
         {
@@ -2515,8 +2523,7 @@ QPDF::replaceForeignIndirectObjects(
             this->m->copied_stream_data_provider->registerForeignStream(
                 local_og, foreign);
             result.replaceStreamData(this->m->copied_streams,
-                                     dict.getKey("/Filter"),
-                                     dict.getKey("/DecodeParms"));
+                                     filter, decode_parms);
         }
         else
         {
@@ -2534,8 +2541,7 @@ QPDF::replaceForeignIndirectObjects(
             this->m->copied_stream_data_provider->registerForeignStream(
                 local_og, foreign_stream_data);
             result.replaceStreamData(this->m->copied_streams,
-                                     dict.getKey("/Filter"),
-                                     dict.getKey("/DecodeParms"));
+                                     filter, decode_parms);
         }
     }
     else
diff --git a/make/msvc.mk b/make/msvc.mk
index c8bc57bc..cfeaa45a 100644
--- a/make/msvc.mk
+++ b/make/msvc.mk
@@ -66,7 +66,7 @@ endef
 # Usage: $(call makelib,objs,library,ldflags,libs,current,revision,age)
 define makelib
 	cl -nologo -O2 -Zi -Gy -EHsc -MD -LD -Fe$(basename $(2))$(shell expr $(5) - $(7)).dll $(1) \
-		-link -SUBSYSTEM:CONSOLE,5.01 -incremental:no \
+		-link -SUBSYSTEM:CONSOLE -incremental:no \
 		$(foreach L,$(subst -L,,$(3)),-LIBPATH:$(L)) \
 		$(foreach L,$(subst -l,,$(4)),$(L).lib)
 	if [ -f $(basename $(2))$(shell expr $(5) - $(7)).dll.manifest ]; then \
@@ -81,7 +81,7 @@ endef
 define makebin
 	cl -nologo -O2 -Zi -Gy -EHsc -MD $(1) \
 		$(if $(5),$(5),$(WINDOWS_MAIN_XLINK_FLAGS)) \
-		-link -SUBSYSTEM:CONSOLE,5.01 -incremental:no -OUT:$(2) \
+		-link -SUBSYSTEM:CONSOLE -incremental:no -OUT:$(2) \
 		$(foreach L,$(subst -L,,$(3)),-LIBPATH:$(L)) \
 		$(foreach L,$(subst -l,,$(4)),$(L).lib)
 	if [ -f $(2).manifest ]; then \
diff --git a/qpdf/qtest/qpdf.test b/qpdf/qtest/qpdf.test
index c2a38fa3..a0ff2a57 100644
--- a/qpdf/qtest/qpdf.test
+++ b/qpdf/qtest/qpdf.test
@@ -2417,7 +2417,7 @@ $td->runtest("check output",
 show_ntests();
 # ----------
 $td->notify("--- Copy Foreign Objects ---");
-$n_tests += 7;
+$n_tests += 10;
 
 foreach my $d ([25, 1], [26, 2], [27, 3])
 {
@@ -2438,6 +2438,19 @@ $td->runtest("copy objects error",
              {$td->FILE => "copy-foreign-objects-errors.out",
               $td->EXIT_STATUS => 0},
              $td->NORMALIZE_NEWLINES);
+
+$td->runtest("indirect filters",
+             {$td->COMMAND => "test_driver 69 indirect-filter.pdf"},
+             {$td->STRING => "test 69 done\n", $td->EXIT_STATUS => 0},
+             $td->NORMALIZE_NEWLINES);
+foreach my $i (0, 1)
+{
+    $td->runtest("check output",
+                 {$td->FILE => "auto-$i.pdf"},
+                 {$td->FILE => "indirect-filter-out-$i.pdf"});
+}
+
+
 show_ntests();
 # ----------
 $td->notify("--- Error Condition Tests ---");
diff --git a/qpdf/qtest/qpdf/indirect-filter-out-0.pdf b/qpdf/qtest/qpdf/indirect-filter-out-0.pdf
new file mode 100644
index 00000000..79e80601
--- /dev/null
+++ b/qpdf/qtest/qpdf/indirect-filter-out-0.pdf
diff --git a/qpdf/qtest/qpdf/indirect-filter-out-1.pdf b/qpdf/qtest/qpdf/indirect-filter-out-1.pdf
new file mode 100644
index 00000000..0e8aac8e
--- /dev/null
+++ b/qpdf/qtest/qpdf/indirect-filter-out-1.pdf
diff --git a/qpdf/qtest/qpdf/indirect-filter.pdf b/qpdf/qtest/qpdf/indirect-filter.pdf
new file mode 100644
index 00000000..9717bb89
--- /dev/null
+++ b/qpdf/qtest/qpdf/indirect-filter.pdf
diff --git a/qpdf/test_driver.cc b/qpdf/test_driver.cc
index 07445dea..167c509b 100644
--- a/qpdf/test_driver.cc
+++ b/qpdf/test_driver.cc
@@ -2195,6 +2195,22 @@ void runtest(int n, char const* filename1, char const* arg2)
 	    std::cout << "raw stream data okay" << std::endl;
 	}
     }
+    else if (n == 69)
+    {
+        pdf.setImmediateCopyFrom(true);
+        auto pages = pdf.getAllPages();
+        for (size_t i = 0; i < pages.size(); ++i)
+        {
+            QPDF out;
+            out.emptyPDF();
+            out.addPage(pages.at(i), false);
+            std::string outname = std::string("auto-") +
+                QUtil::uint_to_string(i) + ".pdf";
+            QPDFWriter w(out, outname.c_str());
+            w.setStaticID(true);
+            w.write();
+        }
+    }
     else
     {
 	throw std::runtime_error(std::string("invalid test ") +