LibPDF: Harden the document/parser against errors

Userland/Applications/PDFViewer/PDFViewerWidget.cpp

@@ -105,7 +105,9 @@ void PDFViewerWidget::open_file(const String& path)
     auto file_result = Core::File::open(path, Core::OpenMode::ReadOnly);
     VERIFY(!file_result.is_error());
     m_buffer = file_result.value()->read_all();
-    auto document = adopt_ref(*new PDF::Document(m_buffer));
+    auto document = PDF::Document::create(m_buffer);
+    // FIXME: Show error dialog if the Document is invalid
+    VERIFY(document);
     m_viewer->set_document(document);
     m_total_page_label->set_text(String::formatted("of {}", document->get_page_count()));
     m_total_page_label->set_fixed_width(30);

Userland/Libraries/LibPDF/Document.cpp

@@ -34,20 +34,32 @@ String OutlineItem::to_string(int indent) const
     return builder.to_string();
 }
 
-Document::Document(const ReadonlyBytes& bytes)
-    : m_parser(Parser({}, bytes))
+RefPtr<Document> Document::create(const ReadonlyBytes& bytes)
 {
-    m_parser.set_document(this);
+    auto parser = adopt_ref(*new Parser({}, bytes));
+    auto document = adopt_ref(*new Document(parser));
 
-    VERIFY(m_parser.perform_validation());
-    auto [xref_table, trailer] = m_parser.parse_last_xref_table_and_trailer();
+    VERIFY(parser->perform_validation());
+    auto xref_table_and_trailer_opt = parser->parse_last_xref_table_and_trailer();
+    if (!xref_table_and_trailer_opt.has_value())
+        return {};
 
-    m_xref_table = xref_table;
-    m_trailer = trailer;
+    auto [xref_table, trailer] = xref_table_and_trailer_opt.value();
 
-    m_catalog = m_trailer->get_dict(this, CommonNames::Root);
-    build_page_tree();
-    build_outline();
+    document->m_xref_table = xref_table;
+    document->m_trailer = trailer;
+
+    document->m_catalog = document->m_trailer->get_dict(document, CommonNames::Root);
+    document->build_page_tree();
+    document->build_outline();
+
+    return document;
+}
+
+Document::Document(const NonnullRefPtr<Parser>& parser)
+    : m_parser(parser)
+{
+    m_parser->set_document(this);
 }
 
 Value Document::get_or_load_value(u32 index)
@@ -58,7 +70,7 @@ Value Document::get_or_load_value(u32 index)
 
     VERIFY(m_xref_table.has_object(index));
     auto byte_offset = m_xref_table.byte_offset_for_object(index);
-    auto indirect_value = m_parser.parse_indirect_value_at_offset(byte_offset);
+    auto indirect_value = m_parser->parse_indirect_value_at_offset(byte_offset);
     VERIFY(indirect_value->index() == index);
     value = indirect_value->value();
     m_values.set(index, value);
@@ -145,14 +157,19 @@ Value Document::resolve(const Value& value)
     return obj;
 }
 
-void Document::build_page_tree()
+bool Document::build_page_tree()
 {
+    if (!m_catalog->contains(CommonNames::Pages))
+        return false;
     auto page_tree = m_catalog->get_dict(this, CommonNames::Pages);
-    add_page_tree_node_to_page_tree(page_tree);
+    return add_page_tree_node_to_page_tree(page_tree);
 }
 
-void Document::add_page_tree_node_to_page_tree(NonnullRefPtr<DictObject> page_tree)
+bool Document::add_page_tree_node_to_page_tree(NonnullRefPtr<DictObject> page_tree)
 {
+    if (!page_tree->contains(CommonNames::Kids) || !page_tree->contains(CommonNames::Count))
+        return false;
+
     auto kids_array = page_tree->get_array(this, CommonNames::Kids);
     auto page_count = page_tree->get(CommonNames::Count).value().as_int();
 
@@ -163,20 +180,24 @@ void Document::add_page_tree_node_to_page_tree(NonnullRefPtr<DictObject> page_tr
         for (auto& value : *kids_array) {
             auto reference_index = value.as_ref_index();
             auto byte_offset = m_xref_table.byte_offset_for_object(reference_index);
-            auto maybe_page_tree_node = m_parser.conditionally_parse_page_tree_node_at_offset(byte_offset);
+            bool ok;
+            auto maybe_page_tree_node = m_parser->conditionally_parse_page_tree_node_at_offset(byte_offset, ok);
+            if (!ok)
+                return false;
             if (maybe_page_tree_node) {
-                add_page_tree_node_to_page_tree(maybe_page_tree_node.release_nonnull());
+                if (!add_page_tree_node_to_page_tree(maybe_page_tree_node.release_nonnull()))
+                    return false;
             } else {
                 m_page_object_indices.append(reference_index);
             }
         }
-
-        return;
-    }
-
+    } else {
         // We know all of the kids are leaf nodes
         for (auto& value : *kids_array)
             m_page_object_indices.append(value.as_ref_index());
+    }
+
+    return true;
 }
 
 void Document::build_outline()
@@ -187,8 +208,8 @@ void Document::build_outline()
     auto outline_dict = m_catalog->get_dict(this, CommonNames::Outlines);
     if (!outline_dict->contains(CommonNames::First))
         return;
-
-    VERIFY(outline_dict->contains(CommonNames::Last));
+    if (!outline_dict->contains(CommonNames::Last))
+        return;
 
     auto first_ref = outline_dict->get_value(CommonNames::First);
     auto last_ref = outline_dict->get_value(CommonNames::Last);

Userland/Libraries/LibPDF/Document.h

@@ -73,7 +73,7 @@ struct OutlineDict final : public RefCounted<OutlineDict> {
 
 class Document final : public RefCounted<Document> {
 public:
-    explicit Document(const ReadonlyBytes& bytes);
+    static RefPtr<Document> create(const ReadonlyBytes& bytes);
 
     ALWAYS_INLINE const XRefTable& xref_table() const { return m_xref_table; }
     ALWAYS_INLINE const DictObject& trailer() const { return *m_trailer; }
@@ -123,20 +123,22 @@ public:
     }
 
 private:
+    explicit Document(const NonnullRefPtr<Parser>& parser);
+
     // FIXME: Currently, to improve performance, we don't load any pages at Document
     // construction, rather we just load the page structure and populate
     // m_page_object_indices. However, we can be even lazier and defer page tree node
     // parsing, as good PDF writers will layout the page tree in a balanced tree to
     // improve lookup time. This would reduce the initial overhead by not loading
     // every page tree node of, say, a 1000+ page PDF file.
-    void build_page_tree();
-    void add_page_tree_node_to_page_tree(NonnullRefPtr<DictObject> page_tree);
+    bool build_page_tree();
+    bool add_page_tree_node_to_page_tree(NonnullRefPtr<DictObject> page_tree);
 
     void build_outline();
     NonnullRefPtr<OutlineItem> build_outline_item(NonnullRefPtr<DictObject> outline_item_dict);
     NonnullRefPtrVector<OutlineItem> build_outline_item_chain(const Value& first_ref, const Value& last_ref);
 
-    Parser m_parser;
+    NonnullRefPtr<Parser> m_parser;
     XRefTable m_xref_table;
     RefPtr<DictObject> m_trailer;
     RefPtr<DictObject> m_catalog;

Userland/Libraries/LibPDF/Parser.cpp

@@ -24,8 +24,8 @@ static NonnullRefPtr<T> make_object(Args... args) requires(IsBaseOf<Object, T>)
 
 Vector<Command> Parser::parse_graphics_commands(const ReadonlyBytes& bytes)
 {
-    Parser parser(bytes);
-    return parser.parse_graphics_commands();
+    auto parser = adopt_ref(*new Parser(bytes));
+    return parser->parse_graphics_commands();
 }
 
 Parser::Parser(Badge<Document>, const ReadonlyBytes& bytes)
@@ -43,26 +43,34 @@ bool Parser::perform_validation()
     return !sloppy_is_linearized() && parse_header();
 }
 
-Parser::XRefTableAndTrailer Parser::parse_last_xref_table_and_trailer()
+Optional<Parser::XRefTableAndTrailer> Parser::parse_last_xref_table_and_trailer()
 {
     m_reader.move_to(m_reader.bytes().size() - 1);
-    VERIFY(navigate_to_before_eof_marker());
-    navigate_to_after_startxref();
-    VERIFY(!m_reader.done());
+    if (!navigate_to_before_eof_marker())
+        return {};
+    if (!navigate_to_after_startxref())
+        return {};
+    if (m_reader.done())
+        return {};
 
     m_reader.set_reading_forwards();
     auto xref_offset_value = parse_number();
-    VERIFY(xref_offset_value.is_int());
+    if (!xref_offset_value.is_int())
+        return {};
     auto xref_offset = xref_offset_value.as_int();
 
     m_reader.move_to(xref_offset);
     auto xref_table = parse_xref_table();
+    if (!xref_table.has_value())
+        return {};
     auto trailer = parse_file_trailer();
+    if (!trailer)
+        return {};
 
-    return { xref_table, trailer };
+    return XRefTableAndTrailer { xref_table.value(), trailer.release_nonnull() };
 }
 
-NonnullRefPtr<IndirectValue> Parser::parse_indirect_value_at_offset(size_t offset)
+RefPtr<IndirectValue> Parser::parse_indirect_value_at_offset(size_t offset)
 {
     m_reader.set_reading_forwards();
     m_reader.move_to(offset);
@@ -103,11 +111,13 @@ bool Parser::parse_header()
     return true;
 }
 
-XRefTable Parser::parse_xref_table()
+Optional<XRefTable> Parser::parse_xref_table()
 {
-    VERIFY(m_reader.matches("xref"));
+    if (!m_reader.matches("xref"))
+        return {};
     m_reader.move_by(4);
-    consume_eol();
+    if (!consume_eol())
+        return {};
 
     XRefTable table;
 
@@ -125,23 +135,28 @@ XRefTable Parser::parse_xref_table()
         for (int i = 0; i < object_count; i++) {
             auto offset_string = String(m_reader.bytes().slice(m_reader.offset(), 10));
             m_reader.move_by(10);
-            consume(' ');
+            if (!consume(' '))
+                return {};
 
             auto generation_string = String(m_reader.bytes().slice(m_reader.offset(), 5));
             m_reader.move_by(5);
-            consume(' ');
+            if (!consume(' '))
+                return {};
 
             auto letter = m_reader.read();
-            VERIFY(letter == 'n' || letter == 'f');
+            if (letter != 'n' && letter != 'f')
+                return {};
 
             // The line ending sequence can be one of the following:
             // SP CR, SP LF, or CR LF
             if (m_reader.matches(' ')) {
                 consume();
                 auto ch = consume();
-                VERIFY(ch == '\r' || ch == '\n');
+                if (ch != '\r' && ch != '\n')
+                    return {};
             } else {
-                VERIFY(m_reader.matches("\r\n"));
+                if (!m_reader.matches("\r\n"))
+                    return {};
                 m_reader.move_by(2);
             }
 
@@ -157,23 +172,27 @@ XRefTable Parser::parse_xref_table()
     return table;
 }
 
-NonnullRefPtr<DictObject> Parser::parse_file_trailer()
+RefPtr<DictObject> Parser::parse_file_trailer()
 {
-    VERIFY(m_reader.matches("trailer"));
+    if (!m_reader.matches("trailer"))
+        return {};
     m_reader.move_by(7);
     consume_whitespace();
     auto dict = parse_dict();
+    if (!dict)
+        return {};
 
-    VERIFY(m_reader.matches("startxref"));
+    if (!m_reader.matches("startxref"))
+        return {};
     m_reader.move_by(9);
     consume_whitespace();
 
     m_reader.move_until([&](auto) { return matches_eol(); });
-    consume_eol();
-    VERIFY(m_reader.matches("%%EOF"));
+    VERIFY(consume_eol());
+    if (!m_reader.matches("%%EOF"))
+        return {};
     m_reader.move_by(5);
     consume_whitespace();
-    VERIFY(m_reader.done());
 
     return dict;
 }
@@ -291,8 +310,10 @@ Value Parser::parse_value()
 
     if (m_reader.matches("<<")) {
         auto dict = parse_dict();
+        if (!dict)
+            return {};
         if (m_reader.matches("stream\n"))
-            return parse_stream(dict);
+            return parse_stream(dict.release_nonnull());
         return dict;
     }
 
@@ -335,23 +356,28 @@ Value Parser::parse_possible_indirect_value_or_ref()
     return first_number;
 }
 
-NonnullRefPtr<IndirectValue> Parser::parse_indirect_value(int index, int generation)
+RefPtr<IndirectValue> Parser::parse_indirect_value(int index, int generation)
 {
-    VERIFY(m_reader.matches("obj"));
+    if (!m_reader.matches("obj"))
+        return {};
     m_reader.move_by(3);
     if (matches_eol())
         consume_eol();
     auto value = parse_value();
-    VERIFY(m_reader.matches("endobj"));
+    if (!m_reader.matches("endobj"))
+        return {};
 
     return make_object<IndirectValue>(index, generation, value);
 }
 
-NonnullRefPtr<IndirectValue> Parser::parse_indirect_value()
+RefPtr<IndirectValue> Parser::parse_indirect_value()
 {
     auto first_number = parse_number();
+    if (!first_number.is_int())
+        return {};
     auto second_number = parse_number();
-    VERIFY(first_number.is_int() && second_number.is_int());
+    if (!second_number.is_int())
+        return {};
     return parse_indirect_value(first_number.as_int(), second_number.as_int());
 }
 
@@ -387,9 +413,10 @@ Value Parser::parse_number()
     return Value(static_cast<int>(f));
 }
 
-NonnullRefPtr<NameObject> Parser::parse_name()
+RefPtr<NameObject> Parser::parse_name()
 {
-    consume('/');
+    if (!consume('/'))
+        return {};
     StringBuilder builder;
 
     while (true) {
@@ -400,7 +427,8 @@ NonnullRefPtr<NameObject> Parser::parse_name()
             int hex_value = 0;
             for (int i = 0; i < 2; i++) {
                 auto ch = consume();
-                VERIFY(isxdigit(ch));
+                if (!isxdigit(ch))
+                    return {};
                 hex_value *= 16;
                 if (ch <= '9') {
                     hex_value += ch - '0';
@@ -420,7 +448,7 @@ NonnullRefPtr<NameObject> Parser::parse_name()
     return make_object<NameObject>(builder.to_string());
 }
 
-NonnullRefPtr<StringObject> Parser::parse_string()
+RefPtr<StringObject> Parser::parse_string()
 {
     ScopeGuard guard([&] { consume_whitespace(); });
 
@@ -435,6 +463,9 @@ NonnullRefPtr<StringObject> Parser::parse_string()
         is_binary_string = true;
     }
 
+    if (string.is_null())
+        return {};
+
     if (string.bytes().starts_with(Array<u8, 2> { 0xfe, 0xff })) {
         // The string is encoded in UTF16-BE
         string = TextCodec::decoder_for("utf-16be")->to_utf8(string.substring(2));
@@ -449,7 +480,8 @@ NonnullRefPtr<StringObject> Parser::parse_string()
 
 String Parser::parse_literal_string()
 {
-    consume('(');
+    if (!consume('('))
+        return {};
     StringBuilder builder;
     auto opened_parens = 0;
 
@@ -470,7 +502,9 @@ String Parser::parse_literal_string()
                 continue;
             }
 
-            VERIFY(!m_reader.done());
+            if (m_reader.done())
+                return {};
+
             auto ch = consume();
             switch (ch) {
             case 'n':
@@ -520,13 +554,16 @@ String Parser::parse_literal_string()
         }
     }
 
-    VERIFY(opened_parens == 0);
+    if (opened_parens != 0)
+        return {};
+
     return builder.to_string();
 }
 
 String Parser::parse_hex_string()
 {
-    consume('<');
+    if (!consume('<'))
+        return {};
     StringBuilder builder;
 
     while (true) {
@@ -546,7 +583,9 @@ String Parser::parse_hex_string()
                     builder.append(static_cast<char>(hex_value));
                     return builder.to_string();
                 }
-                VERIFY(isxdigit(ch));
+
+                if (!isxdigit(ch))
+                    return {};
 
                 hex_value *= 16;
                 if (ch <= '9') {
@@ -561,25 +600,31 @@ String Parser::parse_hex_string()
     }
 }
 
-NonnullRefPtr<ArrayObject> Parser::parse_array()
+RefPtr<ArrayObject> Parser::parse_array()
 {
-    consume('[');
+    if (!consume('['))
+        return {};
     consume_whitespace();
     Vector<Value> values;
 
-    while (!m_reader.matches(']'))
-        values.append(parse_value());
+    while (!m_reader.matches(']')) {
+        auto value = parse_value();
+        if (!value)
+            return {};
+        values.append(value);
+    }
 
-    consume(']');
+    if (!consume(']'))
+        return {};
     consume_whitespace();
 
     return make_object<ArrayObject>(values);
 }
 
-NonnullRefPtr<DictObject> Parser::parse_dict()
+RefPtr<DictObject> Parser::parse_dict()
 {
-    consume('<');
-    consume('<');
+    if (!consume('<') || !consume('<'))
+        return {};
     consume_whitespace();
     HashMap<FlyString, Value> map;
 
@@ -587,28 +632,38 @@ NonnullRefPtr<DictObject> Parser::parse_dict()
         if (m_reader.matches(">>"))
             break;
         auto name = parse_name();
+        if (!name)
+            return {};
         auto value = parse_value();
+        if (!value)
+            return {};
         map.set(name->name(), value);
     }
 
-    consume('>');
-    consume('>');
+    if (!consume('>') || !consume('>'))
+        return {};
     consume_whitespace();
 
     return make_object<DictObject>(map);
 }
 
-RefPtr<DictObject> Parser::conditionally_parse_page_tree_node_at_offset(size_t offset)
+RefPtr<DictObject> Parser::conditionally_parse_page_tree_node_at_offset(size_t offset, bool& ok)
 {
+    ok = true;
+
     m_reader.move_to(offset);
     parse_number();
     parse_number();
-    VERIFY(m_reader.matches("obj"));
+    if (!m_reader.matches("obj")) {
+        ok = false;
+        return {};
+    }
+
     m_reader.move_by(3);
     consume_whitespace();
 
-    consume('<');
-    consume('<');
+    if (!consume('<') || !consume('<'))
+        return {};
     consume_whitespace();
     HashMap<FlyString, Value> map;
 
@@ -616,12 +671,21 @@ RefPtr<DictObject> Parser::conditionally_parse_page_tree_node_at_offset(size_t o
         if (m_reader.matches(">>"))
             break;
         auto name = parse_name();
+        if (!name) {
+            ok = false;
+            return {};
+        }
+
         auto name_string = name->name();
         if (!name_string.is_one_of(CommonNames::Type, CommonNames::Parent, CommonNames::Kids, CommonNames::Count)) {
             // This is a page, not a page tree node
             return {};
         }
         auto value = parse_value();
+        if (!value) {
+            ok = false;
+            return {};
+        }
         if (name_string == CommonNames::Type) {
             if (!value.is_object())
                 return {};
@@ -635,18 +699,20 @@ RefPtr<DictObject> Parser::conditionally_parse_page_tree_node_at_offset(size_t o
         map.set(name->name(), value);
     }
 
-    consume('>');
-    consume('>');
+    if (!consume('>') || !consume('>'))
+        return {};
     consume_whitespace();
 
     return make_object<DictObject>(map);
 }
 
-NonnullRefPtr<StreamObject> Parser::parse_stream(NonnullRefPtr<DictObject> dict)
+RefPtr<StreamObject> Parser::parse_stream(NonnullRefPtr<DictObject> dict)
 {
-    VERIFY(m_reader.matches("stream"));
+    if (!m_reader.matches("stream"))
+        return {};
     m_reader.move_by(6);
-    consume_eol();
+    if (!consume_eol())
+        return {};
 
     ReadonlyBytes bytes;
 
@@ -681,8 +747,8 @@ NonnullRefPtr<StreamObject> Parser::parse_stream(NonnullRefPtr<DictObject> dict)
     if (dict->contains(CommonNames::Filter)) {
         auto filter_type = dict->get_name(m_document, CommonNames::Filter)->name();
         auto maybe_bytes = Filter::decode(bytes, filter_type);
-        // FIXME: Handle error condition
-        VERIFY(maybe_bytes.has_value());
+        if (!maybe_bytes.has_value())
+            return {};
         return make_object<EncodedStreamObject>(dict, move(maybe_bytes.value()));
     }
 
@@ -752,14 +818,15 @@ bool Parser::matches_regular_character() const
     return !matches_delimiter() && !matches_whitespace();
 }
 
-void Parser::consume_eol()
+bool Parser::consume_eol()
 {
     if (m_reader.matches("\r\n")) {
         consume(2);
-    } else {
-        auto consumed = consume();
-        VERIFY(consumed == 0xd || consumed == 0xa);
+        return true;
     }
+
+    auto consumed = consume();
+    return consumed == 0xd || consumed == 0xa;
 }
 
 bool Parser::consume_whitespace()
@@ -777,9 +844,15 @@ char Parser::consume()
     return m_reader.read();
 }
 
-void Parser::consume(char ch)
+void Parser::consume(int amount)
 {
-    VERIFY(consume() == ch);
+    for (size_t i = 0; i < static_cast<size_t>(amount); i++)
+        consume();
+}
+
+bool Parser::consume(char ch)
+{
+    return consume() == ch;
 }
 
 }

Userland/Libraries/LibPDF/Parser.h

@@ -16,7 +16,7 @@ namespace PDF {
 
 class Document;
 
-class Parser {
+class Parser final : public RefCounted<Parser> {
 public:
     static Vector<Command> parse_graphics_commands(const ReadonlyBytes&);
 
@@ -30,18 +30,18 @@ public:
         XRefTable xref_table;
         NonnullRefPtr<DictObject> trailer;
     };
-    XRefTableAndTrailer parse_last_xref_table_and_trailer();
+    Optional<XRefTableAndTrailer> parse_last_xref_table_and_trailer();
 
-    NonnullRefPtr<IndirectValue> parse_indirect_value_at_offset(size_t offset);
+    RefPtr<IndirectValue> parse_indirect_value_at_offset(size_t offset);
 
-    RefPtr<DictObject> conditionally_parse_page_tree_node_at_offset(size_t offset);
+    RefPtr<DictObject> conditionally_parse_page_tree_node_at_offset(size_t offset, bool& ok);
 
 private:
     explicit Parser(const ReadonlyBytes&);
 
     bool parse_header();
-    XRefTable parse_xref_table();
-    NonnullRefPtr<DictObject> parse_file_trailer();
+    Optional<XRefTable> parse_xref_table();
+    RefPtr<DictObject> parse_file_trailer();
 
     bool navigate_to_before_eof_marker();
     bool navigate_to_after_startxref();
@@ -58,16 +58,16 @@ private:
 
     Value parse_value();
     Value parse_possible_indirect_value_or_ref();
-    NonnullRefPtr<IndirectValue> parse_indirect_value(int index, int generation);
-    NonnullRefPtr<IndirectValue> parse_indirect_value();
+    RefPtr<IndirectValue> parse_indirect_value(int index, int generation);
+    RefPtr<IndirectValue> parse_indirect_value();
     Value parse_number();
-    NonnullRefPtr<NameObject> parse_name();
-    NonnullRefPtr<StringObject> parse_string();
+    RefPtr<NameObject> parse_name();
+    RefPtr<StringObject> parse_string();
     String parse_literal_string();
     String parse_hex_string();
-    NonnullRefPtr<ArrayObject> parse_array();
-    NonnullRefPtr<DictObject> parse_dict();
-    NonnullRefPtr<StreamObject> parse_stream(NonnullRefPtr<DictObject> dict);
+    RefPtr<ArrayObject> parse_array();
+    RefPtr<DictObject> parse_dict();
+    RefPtr<StreamObject> parse_stream(NonnullRefPtr<DictObject> dict);
 
     Vector<Command> parse_graphics_commands();
 
@@ -77,10 +77,11 @@ private:
     bool matches_delimiter() const;
     bool matches_regular_character() const;
 
-    void consume_eol();
+    bool consume_eol();
     bool consume_whitespace();
     char consume();
-    void consume(char);
+    void consume(int amount);
+    bool consume(char);
 
     Reader m_reader;
     RefPtr<Document> m_document;

Userland/Libraries/LibPDF/Value.h

@@ -61,6 +61,16 @@ public:
         m_as_ref = (generation_index << 14) | index;
     }
 
+    template<IsObject T>
+    Value(RefPtr<T> obj)
+        : m_type(obj ? Type::Object : Type::Empty)
+    {
+        if (obj) {
+            obj->ref();
+            m_as_object = obj;
+        }
+    }
+
     template<IsObject T>
     Value(NonnullRefPtr<T> obj)
         : m_type(Type::Object)