[ECOS] DNS race condition and patch

[ECOS] DNS race condition and patch

[Will Lentz]

[-- Attachment #1: Type: text/plain, Size: 923 bytes --]

Hi,

There's a race condition in
packages/net/ns/dns/current/include/dns_impl.inl and dns.c.

Here's a quick example of how the current code may cause a problem:
 1) Call cyg_dns_res_start() in thread #1.
    Assume the connect() call blocks for a few seconds.
    Note that at this point 's' is valid, but 'ptdindex' in
uninitialized or invalid.
 2) While thread #1 is blocked, call gethostbyname() (or
gethostbyaddr()) in thread #2.
    The call to free_stored_hent() assumes 'ptdindex' is valid, but it
is not.
    If you have asserts on, cyg_thread_get_data(ptdindex) will hit an
assert or return
    a bogus pointer value that may get passed to free().

The attached fix:
 - puts 's' and free_stored_hent()/ptdindex inside the mutex lock so
they are
   always consistent with each other.
 - makes 'init' file-visible so it can protect against accessing an
uninitialized mutex.

Thanks,
Will

[-- Attachment #2: patch_dns.txt --]
[-- Type: text/plain, Size: 3208 bytes --]

diff -ru orig/include/dns_impl.inl new/include/dns_impl.inl
--- orig/include/dns_impl.inl	2009-07-14 15:17:05.000000000 -0700
+++ new/include/dns_impl.inl	2009-07-14 15:25:52.000000000 -0700
@@ -382,15 +382,18 @@
     CYG_REPORT_FUNCNAMETYPE( "gethostbyaddr", "returning %08x" );
     CYG_REPORT_FUNCARG3( "addr=%08x, len=%d, type=%d", addr, len, type );
 
-    if ( !addr || 0 == len) {
+    if ( !init || !addr || 0 == len) {
         CYG_REPORT_RETVAL( NULL );
         return NULL;
     }
 
     CYG_CHECK_DATA_PTR( addr, "addr is not a valid pointer!" );
 
+    cyg_drv_mutex_lock(&dns_mutex);
+
     /* Has the socket to the DNS server been opened? */
     if (s < 0) {
+        cyg_drv_mutex_unlock(&dns_mutex);
         CYG_REPORT_RETVAL( NULL );
         return NULL;
     }
@@ -401,12 +404,11 @@
 
     /* Only IPv4 addresses accepted */
     if ((type != AF_INET) || (len != sizeof(struct in_addr))) {
+        cyg_drv_mutex_unlock(&dns_mutex);
         CYG_REPORT_RETVAL( NULL );
         return NULL;
     }
 
-    cyg_drv_mutex_lock(&dns_mutex);
-
     /* Build the 'hostname' we want to lookup. */
     sprintf(hostname, "%d.%d.%d.%d.IN-ADDR.ARPA.",
             (unsigned char)addr[3],
@@ -479,15 +481,18 @@
     CYG_REPORT_FUNCNAMETYPE( "gethostbyname", "returning %08x" );
     CYG_REPORT_FUNCARG1( "hostname=%08x", hostname );
 
-    if ( !hostname ) {
+    if ( !init || !hostname ) {
         CYG_REPORT_RETVAL( NULL );
         return NULL;
     }
 
     CYG_CHECK_DATA_PTR( hostname, "hostname is not a valid pointer!" );
 
+    cyg_drv_mutex_lock(&dns_mutex);
+
     /* Has the socket to the DNS server been opened? */
     if (s < 0) {
+        cyg_drv_mutex_unlock(&dns_mutex);
         CYG_REPORT_RETVAL( NULL );
         return NULL;
     }
@@ -500,6 +505,7 @@
          /* It could be a dot address */
          if ((hent = dot_hostname(hostname)) != NULL) {
               store_hent(hent);
+              cyg_drv_mutex_unlock(&dns_mutex);
               CYG_REPORT_RETVAL( hent );
               return hent;
          }
@@ -507,14 +513,15 @@
          /* It could be a colon seperated IPv6 address */
          if ((hent = colon_hostname(hostname)) != NULL) {
               store_hent(hent);
+              cyg_drv_mutex_unlock(&dns_mutex);
               CYG_REPORT_RETVAL( hent );
               return hent;
          }
 #endif
          CYG_REPORT_RETVAL( hent );
+         cyg_drv_mutex_unlock(&dns_mutex);
          return hent;
     }
-    cyg_drv_mutex_lock(&dns_mutex);
 
     if (domainname) {
         if ((strlen(hostname) + strlen(domainname)) > 254) {
diff -ru orig/src/dns.c new/src/dns.c
--- orig/src/dns.c	2009-07-14 15:16:53.000000000 -0700
+++ new/src/dns.c	2009-07-14 15:24:58.000000000 -0700
@@ -79,6 +79,7 @@
 
 #include <cyg/ns/dns/dns_priv.h>
 
+static int init =0;               /* Is DNS initialized? */
 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 */
@@ -262,7 +263,6 @@
 
 int cyg_dns_res_start(char * dns_server) {
 
-    static int init =0;
     struct addrinfo * res;
     int err;
 

[-- Attachment #3: Type: text/plain, Size: 148 bytes --]

-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

[ECOS] Re: DNS race condition and patch

[John Dallaway]

Hi Will

Will Lentz wrote:

> There's a race condition in
> packages/net/ns/dns/current/include/dns_impl.inl and dns.c.

Could you raise a formal bug report for this please? You can do this at:

  http://bugs.ecos.sourceware.org/enter_bug.cgi?product=eCos

Many thanks

John Dallaway

-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

[ECOS] RE: DNS race condition and patch

[Will Lentz]

Hi John,

It's now bug #1000802.

Thanks,
Will

-----Original Message-----
From: John Dallaway [mailto:john@dallaway.org.uk] 
Sent: Wednesday, July 15, 2009 1:23 AM
To: Will Lentz
Cc: ecos-discuss@ecos.sourceware.org
Subject: Re: DNS race condition and patch

Hi Will

Will Lentz wrote:

> There's a race condition in
> packages/net/ns/dns/current/include/dns_impl.inl and dns.c.

Could you raise a formal bug report for this please? You can do this at:

  http://bugs.ecos.sourceware.org/enter_bug.cgi?product=eCos

Many thanks

John Dallaway

--
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss