Fix some bugs around null values in dictionaries

Make it so that a key with a null value is always treated as not being
present. This was inconsistent before.

libqpdf/QPDF_Dictionary.cc

@@ -24,12 +24,11 @@ std::string
 QPDF_Dictionary::unparse()
 {
     std::string result = "<< ";
-    for (std::map<std::string, QPDFObjectHandle>::iterator iter =
-             this->items.begin();
-         iter != this->items.end();
-         ++iter) {
-        result += QPDF_Name::normalizeName((*iter).first) + " " +
-            (*iter).second.unparse() + " ";
+    for (auto& iter : this->items) {
+        if (!iter.second.isNull()) {
+            result += QPDF_Name::normalizeName(iter.first) + " " +
+                iter.second.unparse() + " ";
+        }
     }
     result += ">>";
     return result;
@@ -39,12 +38,11 @@ JSON
 QPDF_Dictionary::getJSON()
 {
     JSON j = JSON::makeDictionary();
-    for (std::map<std::string, QPDFObjectHandle>::iterator iter =
-             this->items.begin();
-         iter != this->items.end();
-         ++iter) {
-        j.addDictionaryMember(
-            QPDF_Name::normalizeName((*iter).first), (*iter).second.getJSON());
+    for (auto& iter : this->items) {
+        if (!iter.second.isNull()) {
+            j.addDictionaryMember(
+                QPDF_Name::normalizeName(iter.first), iter.second.getJSON());
+        }
     }
     return j;
 }
@@ -78,9 +76,10 @@ QPDF_Dictionary::getKey(std::string const& key)
 {
     // PDF spec says fetching a non-existent key from a dictionary
     // returns the null object.
-    if (this->items.count(key)) {
+    auto item = this->items.find(key);
+    if (item != this->items.end()) {
         // May be a null object
-        return (*(this->items.find(key))).second;
+        return item->second;
     } else {
         QPDFObjectHandle null = QPDFObjectHandle::newNull();
         QPDF* qpdf = 0;
@@ -97,12 +96,9 @@ std::set<std::string>
 QPDF_Dictionary::getKeys()
 {
     std::set<std::string> result;
-    for (std::map<std::string, QPDFObjectHandle>::const_iterator iter =
-             this->items.begin();
-         iter != this->items.end();
-         ++iter) {
-        if (hasKey((*iter).first)) {
-            result.insert((*iter).first);
+    for (auto& iter : this->items) {
+        if (!iter.second.isNull()) {
+            result.insert(iter.first);
         }
     }
     return result;

qpdf/qtest/qpdf.test

@@ -1419,6 +1419,17 @@ foreach (my $i = 1; $i <= 3; ++$i)
                  $td->NORMALIZE_NEWLINES);
 }
 
+show_ntests();
+# ----------
+$td->notify("--- Dictionary keys ---");
+$n_tests += 1;
+
+$td->runtest("dictionary keys",
+             {$td->COMMAND => "test_driver 87 - -"},
+             {$td->STRING => "test 87 done\n",
+                  $td->EXIT_STATUS => 0},
+             $td->NORMALIZE_NEWLINES);
+
 show_ntests();
 # ----------
 $td->notify("--- Stream data ---");

qpdf/qtest/qpdf/bad34-recover.out

@@ -2,7 +2,7 @@ WARNING: bad34.pdf: file is damaged
 WARNING: bad34.pdf (object 4 0, offset 322): expected n n obj
 WARNING: bad34.pdf: Attempting to reconstruct cross-reference table
 /QTest is indirect and has type stream (10)
-/QTest is a stream.  Dictionary: << /Length 44 /Quack 9 0 R >>
+/QTest is a stream.  Dictionary: << /Length 44 >>
 Raw stream data:
 BT
   /F1 24 Tf

qpdf/qtest/qpdf/good11.out

@@ -1,6 +1,6 @@
 /QTest is direct and has type dictionary (9)
 /QTest is a dictionary
   /a is direct
-unparse: << /a (a) /b 8 0 R >>
-unparseResolved: << /a (a) /b 8 0 R >>
+unparse: << /a (a) >>
+unparseResolved: << /a (a) >>
 test 1 done

qpdf/test_driver.cc

@@ -3158,6 +3158,26 @@ test_86(QPDF& pdf, char const* arg2)
     assert(h.getUTF8Value() == utf8_val);
 }
 
+static void
+test_87(QPDF& pdf, char const* arg2)
+{
+    // Explicitly demonstrate null dictionary values being the same as
+    // missing keys.
+    auto dict = "<< /A 1 /B null >>"_qpdf;
+    assert(dict.unparse() == "<< /A 1 >>");
+    assert(dict.getKeys() == std::set<std::string>({"/A"}));
+    dict.replaceKey("/A", QPDFObjectHandle::newNull());
+    assert(dict.unparse() == "<< >>");
+    assert(dict.getKeys() == std::set<std::string>());
+    dict = QPDFObjectHandle::newDictionary({
+        {"/A", "2"_qpdf},
+        {"/B", QPDFObjectHandle::newNull()},
+    });
+    assert(dict.unparse() == "<< /A 2 >>");
+    assert(dict.getKeys() == std::set<std::string>({"/A"}));
+    assert(dict.getJSON().unparse() == "{\n  \"/A\": 2\n}");
+}
+
 void
 runtest(int n, char const* filename1, char const* arg2)
 {
@@ -3165,7 +3185,7 @@ runtest(int n, char const* filename1, char const* arg2)
     // the test suite to see how the test is invoked to find the file
     // that the test is supposed to operate on.
 
-    std::set<int> ignore_filename = {61, 81, 83, 84, 85, 86};
+    std::set<int> ignore_filename = {61, 81, 83, 84, 85, 86, 87};
 
     if (n == 0) {
         // Throw in some random test cases that don't fit anywhere
@@ -3259,7 +3279,7 @@ runtest(int n, char const* filename1, char const* arg2)
         {72, test_72}, {73, test_73}, {74, test_74}, {75, test_75},
         {76, test_76}, {77, test_77}, {78, test_78}, {79, test_79},
         {80, test_80}, {81, test_81}, {82, test_82}, {83, test_83},
-        {84, test_84}, {85, test_85}, {86, test_86},
+        {84, test_84}, {85, test_85}, {86, test_86}, {87, test_87},
     };
 
     auto fn = test_functions.find(n);