From fe3dc0247ae8c1f11cd2ebfe719c476eb9190b45 Mon Sep 17 00:00:00 2001
From: Christian Walther <walther@indel.ch>
Date: Wed, 4 Mar 2020 17:47:03 +0100
Subject: [PATCH 2/3] mdns: fix legacy question repeating

When repeating the question in the response to a legacy query, guessing what the question was from the answers given is not reliable, the question needs to be stored.

Specifically, the previous code guessed wrong in the following situations:
- when the question was for type or class ANY
- with the upcoming support for alias names, when the question was about the alias name
---
 src/apps/mdns/mdns.c              |  25 ++++----
 src/apps/mdns/mdns_out.c          | 117 ++++++--------------------------------
 src/include/lwip/apps/mdns_priv.h |   9 +++
 3 files changed, 37 insertions(+), 114 deletions(-)

diff --git a/src/apps/mdns/mdns.c b/src/apps/mdns/mdns.c
index 4be32f1c..26f8a27e 100644
--- a/src/apps/mdns/mdns.c
+++ b/src/apps/mdns/mdns.c
@@ -190,12 +190,6 @@ static struct mdns_packet *pending_tc_questions;
 /* pool of received packets */
 LWIP_MEMPOOL_DECLARE(MDNS_PKTS, MDNS_MAX_STORED_PKTS, sizeof (struct mdns_packet), "Stored mDNS packets")
 
-struct mdns_question {
-  struct mdns_rr_info info;
-  /** unicast reply requested */
-  u16_t unicast;
-};
-
 struct mdns_answer_list {
   u16_t offset[MDNS_PROBE_TIEBREAK_MAX_ANSWERS];
   u16_t size;
@@ -1078,31 +1072,33 @@ mdns_parse_pkt_questions(struct netif *netif, struct mdns_packet *pkt,
   err_t res;
 
   while (pkt->questions_left) {
-    struct mdns_question q;
+    /* parse it in a location where it will still be around in case this is a
+     * legacy query and we will have to repeat it */
+    struct mdns_question* q = &reply->question;
 
-    res = mdns_read_question(pkt, &q);
+    res = mdns_read_question(pkt, q);
     if (res != ERR_OK) {
       LWIP_DEBUGF(MDNS_DEBUG, ("MDNS: Failed to parse question, skipping query packet\n"));
       return res;
     }
 
     LWIP_DEBUGF(MDNS_DEBUG, ("MDNS: Query for domain "));
-    mdns_domain_debug_print(&q.info.domain);
-    LWIP_DEBUGF(MDNS_DEBUG, (" type %d class %d\n", q.info.type, q.info.klass));
+    mdns_domain_debug_print(&q->info.domain);
+    LWIP_DEBUGF(MDNS_DEBUG, (" type %d class %d\n", q->info.type, q->info.klass));
 
-    if (q.unicast) {
+    if (q->unicast) {
       /* Reply unicast if it is requested in the question */
       reply->unicast_reply_requested = 1;
     }
 
-    reply->host_replies |= check_host(netif, &q.info, &reply->host_reverse_v6_replies);
+    reply->host_replies |= check_host(netif, &q->info, &reply->host_reverse_v6_replies);
 
     for (i = 0; i < MDNS_MAX_SERVICES; i++) {
       service = mdns->services[i];
       if (!service) {
         continue;
       }
-      reply->serv_replies[i] |= check_service(service, &q.info);
+      reply->serv_replies[i] |= check_service(service, &q->info);
     }
   }
 
@@ -1365,6 +1361,9 @@ mdns_add_msg_to_delayed(struct mdns_outmsg *dest, struct mdns_outmsg *src)
   dest->cache_flush = src->cache_flush;
   dest->tx_id = src->tx_id;
   dest->legacy_query = src->legacy_query;
+  if (src->legacy_query) {
+	  SMEMCPY(&dest->question, &src->question, sizeof(struct mdns_question));
+  }
 }
 
 /**
diff --git a/src/apps/mdns/mdns_out.c b/src/apps/mdns/mdns_out.c
index 150ea8ea..c6418819 100644
--- a/src/apps/mdns/mdns_out.c
+++ b/src/apps/mdns/mdns_out.c
@@ -255,19 +255,7 @@ mdns_add_a_answer(struct mdns_outpacket *reply, struct mdns_outmsg *msg,
 {
   err_t res;
   u32_t ttl = MDNS_TTL_120;
-  /* When answering to a legacy querier, we need to repeat the question and
-   * limit the ttl to the short legacy ttl */
   if(msg->legacy_query) {
-    /* Repeating the question only needs to be done for the question asked
-     * (max one question), not for the additional records. */
-    if(reply->questions < 1) {
-      LWIP_DEBUGF(MDNS_DEBUG, ("MDNS: Add question for legacy query\n"));
-      res = mdns_add_question(reply, &netif_mdns_data(netif)->name, DNS_RRTYPE_A, DNS_RRCLASS_IN, 0);
-      if (res != ERR_OK) {
-        return res;
-      }
-      reply->questions = 1;
-    }
     /* ttl of legacy answer may not be greater then 10 seconds */
     ttl = MDNS_TTL_10;
   }
@@ -286,19 +274,7 @@ mdns_add_hostv4_ptr_answer(struct mdns_outpacket *reply, struct mdns_outmsg *msg
   u32_t ttl = MDNS_TTL_120;
   struct mdns_domain revhost;
   mdns_build_reverse_v4_domain(&revhost, netif_ip4_addr(netif));
-  /* When answering to a legacy querier, we need to repeat the question and
-   * limit the ttl to the short legacy ttl */
   if(msg->legacy_query) {
-    /* Repeating the question only needs to be done for the question asked
-     * (max one question), not for the additional records. */
-    if(reply->questions < 1) {
-      LWIP_DEBUGF(MDNS_DEBUG, ("MDNS: Add question for legacy query\n"));
-      res = mdns_add_question(reply, &revhost, DNS_RRTYPE_PTR, DNS_RRCLASS_IN, 0);
-      if (res != ERR_OK) {
-        return res;
-      }
-      reply->questions = 1;
-    }
     /* ttl of legacy answer may not be greater then 10 seconds */
     ttl = MDNS_TTL_10;
   }
@@ -316,19 +292,7 @@ mdns_add_aaaa_answer(struct mdns_outpacket *reply, struct mdns_outmsg *msg,
 {
   err_t res;
   u32_t ttl = MDNS_TTL_120;
-  /* When answering to a legacy querier, we need to repeat the question and
-   * limit the ttl to the short legacy ttl */
   if(msg->legacy_query) {
-    /* Repeating the question only needs to be done for the question asked
-     * (max one question), not for the additional records. */
-    if(reply->questions < 1) {
-      LWIP_DEBUGF(MDNS_DEBUG, ("MDNS: Add question for legacy query\n"));
-      res = mdns_add_question(reply, &netif_mdns_data(netif)->name, DNS_RRTYPE_AAAA, DNS_RRCLASS_IN, 0);
-      if (res != ERR_OK) {
-        return res;
-      }
-      reply->questions = 1;
-    }
     /* ttl of legacy answer may not be greater then 10 seconds */
     ttl = MDNS_TTL_10;
   }
@@ -347,19 +311,7 @@ mdns_add_hostv6_ptr_answer(struct mdns_outpacket *reply, struct mdns_outmsg *msg
   u32_t ttl = MDNS_TTL_120;
   struct mdns_domain revhost;
   mdns_build_reverse_v6_domain(&revhost, netif_ip6_addr(netif, addrindex));
-  /* When answering to a legacy querier, we need to repeat the question and
-   * limit the ttl to the short legacy ttl */
   if(msg->legacy_query) {
-    /* Repeating the question only needs to be done for the question asked
-     * (max one question), not for the additional records. */
-    if(reply->questions < 1) {
-      LWIP_DEBUGF(MDNS_DEBUG, ("MDNS: Add question for legacy query\n"));
-      res = mdns_add_question(reply, &revhost, DNS_RRTYPE_PTR, DNS_RRCLASS_IN, 0);
-      if (res != ERR_OK) {
-        return res;
-      }
-      reply->questions = 1;
-    }
     /* ttl of legacy answer may not be greater then 10 seconds */
     ttl = MDNS_TTL_10;
   }
@@ -379,19 +331,7 @@ mdns_add_servicetype_ptr_answer(struct mdns_outpacket *reply, struct mdns_outmsg
   struct mdns_domain service_type, service_dnssd;
   mdns_build_service_domain(&service_type, service, 0);
   mdns_build_dnssd_domain(&service_dnssd);
-  /* When answering to a legacy querier, we need to repeat the question and
-   * limit the ttl to the short legacy ttl */
   if(msg->legacy_query) {
-    /* Repeating the question only needs to be done for the question asked
-     * (max one question), not for the additional records. */
-    if(reply->questions < 1) {
-      LWIP_DEBUGF(MDNS_DEBUG, ("MDNS: Add question for legacy query\n"));
-      res = mdns_add_question(reply, &service_dnssd, DNS_RRTYPE_PTR, DNS_RRCLASS_IN, 0);
-      if (res != ERR_OK) {
-        return res;
-      }
-      reply->questions = 1;
-    }
     /* ttl of legacy answer may not be greater then 10 seconds */
     ttl = MDNS_TTL_10;
   }
@@ -410,19 +350,7 @@ mdns_add_servicename_ptr_answer(struct mdns_outpacket *reply, struct mdns_outmsg
   struct mdns_domain service_type, service_instance;
   mdns_build_service_domain(&service_type, service, 0);
   mdns_build_service_domain(&service_instance, service, 1);
-  /* When answering to a legacy querier, we need to repeat the question and
-   * limit the ttl to the short legacy ttl */
   if(msg->legacy_query) {
-    /* Repeating the question only needs to be done for the question asked
-     * (max one question), not for the additional records. */
-    if(reply->questions < 1) {
-      LWIP_DEBUGF(MDNS_DEBUG, ("MDNS: Add question for legacy query\n"));
-      res = mdns_add_question(reply, &service_type, DNS_RRTYPE_PTR, DNS_RRCLASS_IN, 0);
-      if (res != ERR_OK) {
-        return res;
-      }
-      reply->questions = 1;
-    }
     /* ttl of legacy answer may not be greater then 10 seconds */
     ttl = MDNS_TTL_10;
   }
@@ -448,18 +376,6 @@ mdns_add_srv_answer(struct mdns_outpacket *reply, struct mdns_outmsg *msg,
      * name compression MUST NOT be performed on SRV records.
      */
     srvhost.skip_compression = 1;
-    /* When answering to a legacy querier, we need to repeat the question and
-     * limit the ttl to the short legacy ttl.
-     * Repeating the question only needs to be done for the question asked
-     * (max one question), not for the additional records. */
-    if(reply->questions < 1) {
-      LWIP_DEBUGF(MDNS_DEBUG, ("MDNS: Add question for legacy query\n"));
-      res = mdns_add_question(reply, &service_instance, DNS_RRTYPE_SRV, DNS_RRCLASS_IN, 0);
-      if (res != ERR_OK) {
-        return res;
-      }
-      reply->questions = 1;
-    }
     /* ttl of legacy answer may not be greater then 10 seconds */
     ttl = MDNS_TTL_10;
   }
@@ -482,19 +398,7 @@ mdns_add_txt_answer(struct mdns_outpacket *reply, struct mdns_outmsg *msg,
   struct mdns_domain service_instance;
   mdns_build_service_domain(&service_instance, service, 1);
   mdns_prepare_txtdata(service);
-  /* When answering to a legacy querier, we need to repeat the question and
-   * limit the ttl to the short legacy ttl */
   if(msg->legacy_query) {
-    /* Repeating the question only needs to be done for the question asked
-     * (max one question), not for the additional records. */
-    if(reply->questions < 1) {
-      LWIP_DEBUGF(MDNS_DEBUG, ("MDNS: Add question for legacy query\n"));
-      res = mdns_add_question(reply, &service_instance, DNS_RRTYPE_TXT, DNS_RRCLASS_IN, 0);
-      if (res != ERR_OK) {
-        return res;
-      }
-      reply->questions = 1;
-    }
     /* ttl of legacy answer may not be greater then 10 seconds */
     ttl = MDNS_TTL_10;
   }
@@ -506,14 +410,14 @@ mdns_add_txt_answer(struct mdns_outpacket *reply, struct mdns_outmsg *msg,
 
 
 static err_t
-mdns_add_probe_questions_to_outpacket(struct mdns_outpacket *outpkt, struct mdns_outmsg *msg,
-                                      struct netif *netif)
+mdns_add_questions_to_outpacket(struct mdns_outpacket *outpkt, struct mdns_outmsg *msg,
+                                struct netif *netif)
 {
   err_t res;
   int i;
   struct mdns_host *mdns = netif_mdns_data(netif);
 
-  /* Write host questions (probing or legacy query) */
+  /* Write host questions (probing) */
   if(msg->host_questions & QUESTION_PROBE_HOST_ANY) {
     res = mdns_add_any_host_question(outpkt, mdns, 1);
     if (res != ERR_OK) {
@@ -521,7 +425,7 @@ mdns_add_probe_questions_to_outpacket(struct mdns_outpacket *outpkt, struct mdns
     }
     outpkt->questions++;
   }
-  /* Write service questions (probing or legacy query) */
+  /* Write service questions (probing) */
   for (i = 0; i < MDNS_MAX_SERVICES; i++) {
     struct mdns_service* service = mdns->services[i];
     if (!service) {
@@ -535,6 +439,17 @@ mdns_add_probe_questions_to_outpacket(struct mdns_outpacket *outpkt, struct mdns
       outpkt->questions++;
     }
   }
+
+  /* Repeat legacy question (max one) */
+  if (msg->legacy_query && outpkt->questions < 1) {
+      LWIP_DEBUGF(MDNS_DEBUG, ("MDNS: Add question for legacy query\n"));
+      res = mdns_add_question(outpkt, &msg->question.info.domain, msg->question.info.type, msg->question.info.klass, 0);
+      if (res != ERR_OK) {
+        return res;
+      }
+      outpkt->questions = 1;
+  }
+
   return ERR_OK;
 }
 
@@ -583,7 +498,7 @@ mdns_create_outpacket(struct netif *netif, struct mdns_outmsg *msg,
   }
 #endif
 
-  res = mdns_add_probe_questions_to_outpacket(outpkt, msg, netif);
+  res = mdns_add_questions_to_outpacket(outpkt, msg, netif);
   if (res != ERR_OK) {
     return res;
   }
diff --git a/src/include/lwip/apps/mdns_priv.h b/src/include/lwip/apps/mdns_priv.h
index 6a9d2f0e..46ba53fe 100644
--- a/src/include/lwip/apps/mdns_priv.h
+++ b/src/include/lwip/apps/mdns_priv.h
@@ -127,6 +127,13 @@ struct mdns_outpacket {
   u16_t domain_offsets[NUM_DOMAIN_OFFSETS];
 };
 
+/** parsed mDNS question */
+struct mdns_question {
+  struct mdns_rr_info info;
+  /** unicast reply requested */
+  u16_t unicast;
+};
+
 /** mDNS output message */
 struct mdns_outmsg {
   /** Identifier. Used in legacy queries */
@@ -156,6 +163,8 @@ struct mdns_outmsg {
   u8_t host_reverse_v6_replies;
   /* Reply bitmask per service */
   u8_t serv_replies[MDNS_MAX_SERVICES];
+  /* Used to repeat the question in legacy queries */
+  struct mdns_question question;
 #ifdef LWIP_MDNS_SEARCH
   /** Search query to send */
   struct mdns_request *query;
-- 
2.11.0 (Apple Git-81)

