LibDNS: Make DNS packet parsing fallible

Previously, a DNS packet containing an invalid name would be returned
with an empty name. With this change, an error is returned if any error
is encountered during parsing.

Meta/Lagom/Fuzzers/FuzzDNSPacket.cpp

@@ -9,10 +9,10 @@
 extern "C" int LLVMFuzzerTestOneInput(uint8_t const* data, size_t size)
 {
     AK::set_debug_enabled(false);
-    auto maybe_packet = DNS::Packet::from_raw_packet({ data, size });
+    auto packet_or_error = DNS::Packet::from_raw_packet({ data, size });
-    if (!maybe_packet.has_value())
+    if (packet_or_error.is_error())
         return 0;
 
-    (void)maybe_packet.value().to_byte_buffer();
+    (void)packet_or_error.release_value().to_byte_buffer();
     return 0;
 }

Userland/Libraries/LibDNS/Name.cpp

@@ -21,15 +21,15 @@ Name::Name(DeprecatedString const& name)
         m_name = name;
 }
 
-Name Name::parse(ReadonlyBytes data, size_t& offset, size_t recursion_level)
+ErrorOr<Name> Name::parse(ReadonlyBytes data, size_t& offset, size_t recursion_level)
 {
     if (recursion_level > 4)
-        return {};
+        return Name {};
 
     StringBuilder builder;
     while (true) {
         if (offset >= data.size())
-            return {};
+            return Error::from_string_literal("Unexpected EOF when parsing name");
         u8 b = data[offset++];
         if (b == '\0') {
             // This terminates the name.
@@ -37,15 +37,15 @@ Name Name::parse(ReadonlyBytes data, size_t& offset, size_t recursion_level)
         } else if ((b & 0xc0) == 0xc0) {
             // The two bytes tell us the offset when to continue from.
             if (offset >= data.size())
-                return {};
+                return Error::from_string_literal("Unexpected EOF when parsing name");
             size_t dummy = (b & 0x3f) << 8 | data[offset++];
-            auto rest_of_name = parse(data, dummy, recursion_level + 1);
+            auto rest_of_name = TRY(parse(data, dummy, recursion_level + 1));
             builder.append(rest_of_name.as_string());
             return builder.to_deprecated_string();
         } else {
             // This is the length of a part.
             if (offset + b >= data.size())
-                return {};
+                return Error::from_string_literal("Unexpected EOF when parsing name");
             builder.append({ data.offset_pointer(offset), b });
             builder.append('.');
             offset += b;

Userland/Libraries/LibDNS/Name.h

@@ -17,7 +17,7 @@ public:
     Name() = default;
     Name(DeprecatedString const&);
 
-    static Name parse(ReadonlyBytes data, size_t& offset, size_t recursion_level = 0);
+    static ErrorOr<Name> parse(ReadonlyBytes data, size_t& offset, size_t recursion_level = 0);
 
     size_t serialized_size() const;
     DeprecatedString const& as_string() const { return m_name; }

Userland/Libraries/LibDNS/Packet.cpp

@@ -97,11 +97,11 @@ private:
 
 static_assert(sizeof(DNSRecordWithoutName) == 10);
 
-Optional<Packet> Packet::from_raw_packet(ReadonlyBytes bytes)
+ErrorOr<Packet> Packet::from_raw_packet(ReadonlyBytes bytes)
 {
     if (bytes.size() < sizeof(PacketHeader)) {
-        dbgln("DNS response not large enough ({} out of {}) to be a DNS packet.", bytes.size(), sizeof(PacketHeader));
+        dbgln_if(LOOKUPSERVER_DEBUG, "DNS response not large enough ({} out of {}) to be a DNS packet", bytes.size(), sizeof(PacketHeader));
-        return {};
+        return Error::from_string_literal("DNS response not large enough to be a DNS packet");
     }
 
     auto const& header = *bit_cast<PacketHeader const*>(bytes.data());
@@ -123,13 +123,13 @@ Optional<Packet> Packet::from_raw_packet(ReadonlyBytes bytes)
     size_t offset = sizeof(PacketHeader);
 
     for (u16 i = 0; i < header.question_count(); i++) {
-        auto name = Name::parse(bytes, offset);
+        auto name = TRY(Name::parse(bytes, offset));
         struct RawDNSAnswerQuestion {
             NetworkOrdered<u16> record_type;
             NetworkOrdered<u16> class_code;
         };
         if (offset >= bytes.size() || bytes.size() - offset < sizeof(RawDNSAnswerQuestion))
-            return {};
+            return Error::from_string_literal("Unexpected EOF when parsing DNS packet");
 
         auto const& record_and_class = *bit_cast<RawDNSAnswerQuestion const*>(bytes.offset_pointer(offset));
         u16 class_code = record_and_class.class_code & ~MDNS_WANTS_UNICAST_RESPONSE;
@@ -141,21 +141,21 @@ Optional<Packet> Packet::from_raw_packet(ReadonlyBytes bytes)
     }
 
     for (u16 i = 0; i < header.answer_count(); ++i) {
-        auto name = Name::parse(bytes, offset);
+        auto name = TRY(Name::parse(bytes, offset));
         if (offset >= bytes.size() || bytes.size() - offset < sizeof(DNSRecordWithoutName))
-            return {};
+            return Error::from_string_literal("Unexpected EOF when parsing DNS packet");
 
         auto const& record = *bit_cast<DNSRecordWithoutName const*>(bytes.offset_pointer(offset));
         offset += sizeof(DNSRecordWithoutName);
         if (record.data_length() > bytes.size() - offset)
-            return {};
+            return Error::from_string_literal("Unexpected EOF when parsing DNS packet");
 
         DeprecatedString data;
 
         switch ((RecordType)record.type()) {
         case RecordType::PTR: {
             size_t dummy_offset = offset;
-            data = Name::parse(bytes, dummy_offset).as_string();
+            data = TRY(Name::parse(bytes, dummy_offset)).as_string();
             break;
         }
         case RecordType::CNAME:

Userland/Libraries/LibDNS/Packet.h

@@ -24,7 +24,7 @@ class Packet {
 public:
     Packet() = default;
 
-    static Optional<Packet> from_raw_packet(ReadonlyBytes bytes);
+    static ErrorOr<Packet> from_raw_packet(ReadonlyBytes bytes);
     ErrorOr<ByteBuffer> to_byte_buffer() const;
 
     bool is_query() const { return !m_query_or_response; }

Userland/Services/LookupServer/DNSServer.cpp

@@ -29,12 +29,7 @@ ErrorOr<void> DNSServer::handle_client()
 {
     sockaddr_in client_address;
     auto buffer = TRY(receive(1024, client_address));
-    auto optional_request = Packet::from_raw_packet(buffer);
+    auto request = TRY(Packet::from_raw_packet(buffer));
-    if (!optional_request.has_value()) {
-        dbgln("Got an invalid DNS packet");
-        return {};
-    }
-    auto& request = optional_request.value();
 
     if (!request.is_query()) {
         dbgln("It's not a request");

Userland/Services/LookupServer/LookupServer.cpp

@@ -269,11 +269,11 @@ ErrorOr<Vector<Answer>> LookupServer::lookup(Name const& name, DeprecatedString
 
     did_get_response = true;
 
-    auto o_response = Packet::from_raw_packet({ response_buffer, nrecv });
+    auto response_or_error = Packet::from_raw_packet({ response_buffer, nrecv });
-    if (!o_response.has_value())
+    if (response_or_error.is_error())
         return Vector<Answer> {};
 
-    auto& response = o_response.value();
+    auto response = response_or_error.release_value();
 
     if (response.id() != request.id()) {
         dbgln("LookupServer: ID mismatch ({} vs {}) :(", response.id(), request.id());

Userland/Services/LookupServer/MulticastDNS.cpp

@@ -56,13 +56,7 @@ MulticastDNS::MulticastDNS(Core::EventReceiver* parent)
 ErrorOr<void> MulticastDNS::handle_packet()
 {
     auto buffer = TRY(receive(1024));
-    auto optional_packet = Packet::from_raw_packet(buffer);
+    auto packet = TRY(Packet::from_raw_packet(buffer));
-    if (!optional_packet.has_value()) {
-        dbgln("Got an invalid mDNS packet");
-        return {};
-    }
-    auto& packet = optional_packet.value();
-
     if (packet.is_query())
         handle_query(packet);
     return {};
@@ -175,12 +169,12 @@ ErrorOr<Vector<Answer>> MulticastDNS::lookup(Name const& name, RecordType record
         auto buffer = TRY(receive(1024));
         if (buffer.is_empty())
             return Vector<Answer> {};
-        auto optional_packet = Packet::from_raw_packet(buffer);
+        auto packet_or_error = Packet::from_raw_packet(buffer);
-        if (!optional_packet.has_value()) {
+        if (packet_or_error.is_error()) {
-            dbgln("Got an invalid mDNS packet");
+            dbgln("Got an invalid mDNS packet: {}", packet_or_error.release_error());
             continue;
         }
-        auto& packet = optional_packet.value();
+        auto packet = packet_or_error.release_value();
 
         if (packet.is_query())
             continue;