Re: [ECOS] A bug in DNS lookup.

[Jonathan Larmour <jifl at eCosCentric dot com>]

Frank Huang wrote:
> Hi,
> 
> I found a "signed and unsigned" bug in eCos DNS lookup code. If you guys
> agree that, please fix it and put it in the 3.0 release.
> 
> In dns_impl.inl, there is a function build_query() which build the DNS
> query packet. It uses the following line code to set the transaction ID.
> 
> 
> dns_hdr->id = htons(id++);
> 
> The type of dns_hdr->id is a unsigned 16 bit, but the id in dns.c is a
> short integer.

dns_hdr->id is in fact an unsigned int, but in a 16-bit bitfield. This is
different from being a natural 16-bit quantity like an unsigned short, and
is I think the real cause of the problem.

> According to the protocol, this transaction ID will be
> increased frequently, so when the id increased from 0x7fff to 0x8000, it
> corrupts the next element's data which is a flag. The flag indicates the
> type of the packet. It should be indicated as "standard query" but it
> becomes to "standard query response" when it hits the bug.
> 
> I force my system keep doing DNS lookup, it hits the bug in about 1 hour
> with about 32000 lookup.
> 
> My fixing is that set the id in dns.c to unsigned short integer. The
> path of the files I am talking about is under eocs/packages/net/ns/dns/.

Thanks, for the report. The attached patch includes your suggestion, and
just to be explicit, includes a cast to a cyg_uint16.

Jifl
-- 
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
------["Si fractum non sit, noli id reficere"]------       Opinions==mine

Index: ChangeLog
===================================================================
RCS file: /cvs/ecos/ecos-opt/net/net/ns/dns/current/ChangeLog,v
retrieving revision 1.21
diff -u -5 -p -r1.21 ChangeLog
--- ChangeLog	19 May 2006 10:14:33 -0000	1.21
+++ ChangeLog	12 Aug 2008 16:51:28 -0000
@@ -1,5 +1,12 @@
+2008-08-12  Jonathan Larmour  <jifl@eCosCentric.com>
+
+	* src/dns.c: id global should be unsigned, in line with DNS header.
+	* include/dns_impl.inl (build_query): Guarantee id in header is only
+	16-bits.
+	Thanks to Frank Huang for the report.
+
 2006-05-19  Andrew Lunn  <andrew.lunn@ascom.ch>
 
 	* tests/dns1.c (dns_test_thread): Use CYG_NELEM from infra.
 
 2005-07-29  Andrew Lunn  <andrew.lunn@ascom.ch>
Index: include/dns_impl.inl
===================================================================
RCS file: /cvs/ecos/ecos-opt/net/net/ns/dns/current/include/dns_impl.inl,v
retrieving revision 1.7
diff -u -5 -p -r1.7 dns_impl.inl
--- include/dns_impl.inl	30 Jul 2005 11:26:29 -0000	1.7
+++ include/dns_impl.inl	12 Aug 2008 16:51:28 -0000
@@ -197,11 +197,11 @@ build_query(const unsigned char * msg, c
     unsigned char *ptr;
     int len;
 
     /* Fill out the header */
     dns_hdr = (struct dns_header *) msg;
-    dns_hdr->id = htons(id++);
+    dns_hdr->id = (cyg_uint16)htons(id++);
     dns_hdr->rd = true;
     dns_hdr->opcode = DNS_QUERY;
     dns_hdr->qdcount = htons(1);
   
     /* Now the question we want to ask */
Index: src/dns.c
===================================================================
RCS file: /cvs/ecos/ecos-opt/net/net/ns/dns/current/src/dns.c,v
retrieving revision 1.10
diff -u -5 -p -r1.10 dns.c
--- src/dns.c	30 Jul 2005 11:26:29 -0000	1.10
+++ src/dns.c	12 Aug 2008 16:51:28 -0000
@@ -78,11 +78,11 @@
 #include <string.h>
 #include <stdlib.h>
 
 #include <cyg/ns/dns/dns_priv.h>
 
-static short id = 0;              /* ID of the last query */
+static cyg_uint16 id = 0;         /* ID of the last query */
 static int s = -1;                /* Socket to the DNS server */
 static cyg_drv_mutex_t dns_mutex; /* Mutex to stop multiple queries as once */
 static cyg_ucount32 ptdindex;     /* Index for the per thread data */
 static char * domainname=NULL;    /* Domain name used for queries */