Update MdnsReader::Read to return ErrorOr

This patch fixes a TODO in MdnsReader::Read to check the opcode and
rcode in the message, and return a more useful ErrorOr object.

Bug: b/168240264
Change-Id: Id1d76cf54def9923acd8ce9d9ca6908e0f2b1f8f
Reviewed-on: https://chromium-review.googlesource.com/c/openscreen/+/2425116
Reviewed-by: Jordan Bayles <jophba@chromium.org>
Reviewed-by: Ryan Keane <rwkeane@google.com>
Commit-Queue: Jordan Bayles <jophba@chromium.org>

5 files changed, 57 insertions, 23 deletions

diff --git a/discovery/mdns/mdns_reader.cc b/discovery/mdns/mdns_reader.cc
index ec122e70..bd38e81b 100644
--- a/discovery/mdns/mdns_reader.cc
+++ b/discovery/mdns/mdns_reader.cc
@@ -322,8 +322,8 @@ bool MdnsReader::Read(MdnsQuestion* out) {
   return false;
 }
 
-bool MdnsReader::Read(MdnsMessage* out) {
-  OSP_DCHECK(out);
+ErrorOr<MdnsMessage> MdnsReader::Read() {
+  MdnsMessage out;
   Cursor cursor(this);
   Header header;
   std::vector<MdnsQuestion> questions;
@@ -334,27 +334,26 @@ bool MdnsReader::Read(MdnsMessage* out) {
       Read(header.answer_count, &answers) &&
       Read(header.authority_record_count, &authority_records) &&
       Read(header.additional_record_count, &additional_records)) {
-    // TODO(issuetracker.google.com/168240264): Skip messages with non-zero
-    // opcode and rcode.  One way to do this is to change the method signature
-    // to return ErrorOr<MdnsMessage> and return different error codes for
-    // failure to read and for messages that were read successfully but are
-    // non-conforming.
+    if (!IsValidFlagsSection(header.flags)) {
+      return Error::Code::kMdnsNonConformingFailure;
+    }
+
     ErrorOr<MdnsMessage> message = MdnsMessage::TryCreate(
         header.id, GetMessageType(header.flags), questions, answers,
         authority_records, additional_records);
     if (message.is_error()) {
-      return false;
+      return std::move(message.error());
     }
-    *out = std::move(message.value());
+    out = std::move(message.value());
 
     if (IsMessageTruncated(header.flags)) {
-      out->set_truncated();
+      out.set_truncated();
     }
 
     cursor.Commit();
-    return true;
+    return out;
   }
-  return false;
+  return Error::Code::kMdnsReadFailure;
 }
 
 bool MdnsReader::Read(IPAddress::Version version, IPAddress* out) {
diff --git a/discovery/mdns/mdns_reader.h b/discovery/mdns/mdns_reader.h
index bfe7eb04..2902e288 100644
--- a/discovery/mdns/mdns_reader.h
+++ b/discovery/mdns/mdns_reader.h
@@ -5,9 +5,11 @@
 #ifndef DISCOVERY_MDNS_MDNS_READER_H_
 #define DISCOVERY_MDNS_MDNS_READER_H_
 
+#include <utility>
 #include <vector>
 
 #include "discovery/mdns/mdns_records.h"
+#include "platform/base/error.h"
 #include "util/big_endian.h"
 
 namespace openscreen {
@@ -34,14 +36,16 @@ class MdnsReader : public BigEndianReader {
   bool Read(PtrRecordRdata* out);
   bool Read(TxtRecordRdata* out);
   bool Read(NsecRecordRdata* out);
+
   // Reads a DNS resource record with its RDATA.
   // The correct type of RDATA to be read is determined by the type
   // specified in the record.
   bool Read(MdnsRecord* out);
   bool Read(MdnsQuestion* out);
+
   // Reads multiple mDNS questions and records that are a part of
   // a mDNS message being read.
-  bool Read(MdnsMessage* out);
+  ErrorOr<MdnsMessage> Read();
 
  private:
   struct NsecBitMapField {
diff --git a/discovery/mdns/mdns_reader_fuzztest.cc b/discovery/mdns/mdns_reader_fuzztest.cc
index e28ff92d..9a97c89b 100644
--- a/discovery/mdns/mdns_reader_fuzztest.cc
+++ b/discovery/mdns/mdns_reader_fuzztest.cc
@@ -5,10 +5,15 @@
 #include "discovery/common/config.h"
 #include "discovery/mdns/mdns_reader.h"
 
+namespace openscreen {
+namespace discovery {
+void Fuzz(const uint8_t* data, size_t size) {
+  MdnsReader reader(Config{}, data, size);
+  reader.Read();
+}
+}  // namespace discovery
+}  // namespace openscreen
 extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
-  openscreen::discovery::Config config;
-  openscreen::discovery::MdnsReader reader(config, data, size);
-  openscreen::discovery::MdnsMessage message;
-  reader.Read(&message);
+  openscreen::discovery::Fuzz(data, size);
   return 0;
 }
diff --git a/discovery/mdns/mdns_reader_unittest.cc b/discovery/mdns/mdns_reader_unittest.cc
index 2882c8e2..08062e5d 100644
--- a/discovery/mdns/mdns_reader_unittest.cc
+++ b/discovery/mdns/mdns_reader_unittest.cc
@@ -30,6 +30,17 @@ void TestReadEntrySucceeds(const uint8_t* data,
   EXPECT_EQ(reader.remaining(), UINT64_C(0));
 }
 
+template <>
+void TestReadEntrySucceeds<MdnsMessage>(const uint8_t* data,
+                                        size_t size,
+                                        const MdnsMessage& expected) {
+  MdnsReader reader(Config{}, data, size);
+  const ErrorOr<MdnsMessage> message = reader.Read();
+  EXPECT_TRUE(message.is_value());
+  EXPECT_EQ(message.value(), expected);
+  EXPECT_EQ(reader.remaining(), UINT64_C(0));
+}
+
 template <class T>
 void TestReadEntryFails(const uint8_t* data, size_t size) {
   Config config;
@@ -41,6 +52,17 @@ void TestReadEntryFails(const uint8_t* data, size_t size) {
   EXPECT_EQ(reader.offset(), UINT64_C(0));
 }
 
+template <>
+void TestReadEntryFails<MdnsMessage>(const uint8_t* data, size_t size) {
+  Config config;
+  MdnsReader reader(config, data, size);
+  const ErrorOr<MdnsMessage> message = reader.Read();
+  EXPECT_TRUE(message.is_error());
+
+  // There should be no side effects for failing to read an entry. The
+  // underlying pointer should not have changed.
+  EXPECT_EQ(reader.offset(), UINT64_C(0));
+}
 }  // namespace
 
 TEST(MdnsReaderTest, ReadDomainName) {
diff --git a/discovery/mdns/mdns_receiver.cc b/discovery/mdns/mdns_receiver.cc
index bb8634d2..09982dd8 100644
--- a/discovery/mdns/mdns_receiver.cc
+++ b/discovery/mdns/mdns_receiver.cc
@@ -66,15 +66,19 @@ void MdnsReceiver::OnRead(UdpSocket* socket,
 
   TRACE_SCOPED(TraceCategory::kMdns, "MdnsReceiver::OnRead");
   MdnsReader reader(config_, packet.data(), packet.size());
-  MdnsMessage message;
-  if (!reader.Read(&message)) {
-    OSP_DVLOG << "mDNS message failed to parse...";
+  const ErrorOr<MdnsMessage> message = reader.Read();
+  if (message.is_error()) {
+    if (message.error().code() == Error::Code::kMdnsNonConformingFailure) {
+      OSP_DVLOG << "mDNS message dropped due to invalid rcode or opcode...";
+    } else {
+      OSP_DVLOG << "mDNS message failed to parse...";
+    }
     return;
   }
 
-  if (message.type() == MessageType::Response) {
+  if (message.value().type() == MessageType::Response) {
     for (ResponseClient* client : response_clients_) {
-      client->OnMessageReceived(message);
+      client->OnMessageReceived(message.value());
     }
     if (response_clients_.empty()) {
       OSP_DVLOG
@@ -82,7 +86,7 @@ void MdnsReceiver::OnRead(UdpSocket* socket,
     }
   } else {
     if (query_callback_) {
-      query_callback_(message, packet.source());
+      query_callback_(message.value(), packet.source());
     } else {
       OSP_DVLOG << "mDNS query message dropped. No query client registered...";
     }