[ECOS] 'id' rollover bug in dns.c

[ECOS] 'id' rollover bug in dns.c

[Will Lentz]

Hi,
 
There is a rollover bug in the current CVS copy of dns.c. Around line
237 there is the following code:
            /* Reply to an old query. Ignore it */
            if (ntohs(dns_hdr->id) != (id-1)) {

If dns_hdr->id == 5 and id == 6, then the 'if' condition is false as
expected.

If dns_hdr->id == 0xFFFF and id == 0, then the 'if' condition is
incorrectly true.

The simple patch below fixes the issue:
--- dns_old.c   2009-10-01 16:01:41.000000000 -0700
+++ dns.c       2009-10-01 16:01:45.000000000 -0700
@@ -234,7 +234,7 @@
             }

             /* Reply to an old query. Ignore it */
-            if (ntohs(dns_hdr->id) != (id-1)) {
+            if (ntohs(dns_hdr->id) != (cyg_uint16)(id-1)) {
                 continue;
             }
             finished = true;


Cheers,
Will

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

Re: [ECOS] 'id' rollover bug in dns.c

[Jay Foster]

I discovered that issue some time ago and resolved it in a different 
manner that also allows multiple concurrent DNS queries to be sent from 
different threads.  The solution was to change the declaration of 'id' 
to be unsigned to match the definition of id in the dns_header structure.

-static short id = 0;
+static unsigned short id = 0;

Then declare a new variable in send_recv()

     unsigned short req_id;

Then assign this variable with the ID value contained in the DNS request

     dns_hdr = (struct dns_header *) &msg_buf[0];
+    req_id = ((struct dns_header *)msg)->id;
     do {
         /* Send a request to each configured DNS server */

Then compare the ID in the responses with req_id

                 if (dns_hdr->id != req_id) {
                     continue;
                 }

This avoids ignoring valid DNS responses when the global variable, id, 
is incremented by another concurrent DNS request.

Jay

On 10/1/2009 4:11 PM, Will Lentz wrote:
> Hi,
>
> There is a rollover bug in the current CVS copy of dns.c. Around line
> 237 there is the following code:
>              /* Reply to an old query. Ignore it */
>              if (ntohs(dns_hdr->id) != (id-1)) {
>
> If dns_hdr->id == 5 and id == 6, then the 'if' condition is false as
> expected.
>
> If dns_hdr->id == 0xFFFF and id == 0, then the 'if' condition is
> incorrectly true.
>
> The simple patch below fixes the issue:
> --- dns_old.c   2009-10-01 16:01:41.000000000 -0700
> +++ dns.c       2009-10-01 16:01:45.000000000 -0700
> @@ -234,7 +234,7 @@
>               }
>
>               /* Reply to an old query. Ignore it */
> -            if (ntohs(dns_hdr->id) != (id-1)) {
> +            if (ntohs(dns_hdr->id) != (cyg_uint16)(id-1)) {
>                   continue;
>               }
>               finished = true;
>
>
> Cheers,
> Will
>
>    

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

RE: [ECOS] 'id' rollover bug in dns.c

[Will Lentz]

Thanks!  That's a better solution.

Will 

-----Original Message-----
From: Jay Foster [mailto:jay@systech.com] 
Sent: Thursday, October 01, 2009 4:38 PM
To: Will Lentz
Cc: ecos-discuss@ecos.sourceware.org
Subject: Re: [ECOS] 'id' rollover bug in dns.c

I discovered that issue some time ago and resolved it in a different 
manner that also allows multiple concurrent DNS queries to be sent from 
different threads.  The solution was to change the declaration of 'id' 
to be unsigned to match the definition of id in the dns_header
structure.

-static short id = 0;
+static unsigned short id = 0;

Then declare a new variable in send_recv()

     unsigned short req_id;

Then assign this variable with the ID value contained in the DNS request

     dns_hdr = (struct dns_header *) &msg_buf[0];
+    req_id = ((struct dns_header *)msg)->id;
     do {
         /* Send a request to each configured DNS server */

Then compare the ID in the responses with req_id

                 if (dns_hdr->id != req_id) {
                     continue;
                 }

This avoids ignoring valid DNS responses when the global variable, id, 
is incremented by another concurrent DNS request.

Jay

On 10/1/2009 4:11 PM, Will Lentz wrote:
> Hi,
>
> There is a rollover bug in the current CVS copy of dns.c. Around line
> 237 there is the following code:
>              /* Reply to an old query. Ignore it */
>              if (ntohs(dns_hdr->id) != (id-1)) {
>
> If dns_hdr->id == 5 and id == 6, then the 'if' condition is false as
> expected.
>
> If dns_hdr->id == 0xFFFF and id == 0, then the 'if' condition is
> incorrectly true.
>
> The simple patch below fixes the issue:
> --- dns_old.c   2009-10-01 16:01:41.000000000 -0700
> +++ dns.c       2009-10-01 16:01:45.000000000 -0700
> @@ -234,7 +234,7 @@
>               }
>
>               /* Reply to an old query. Ignore it */
> -            if (ntohs(dns_hdr->id) != (id-1)) {
> +            if (ntohs(dns_hdr->id) != (cyg_uint16)(id-1)) {
>                   continue;
>               }
>               finished = true;
>
>
> Cheers,
> Will
>
>    

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

RE: [ECOS] 'id' rollover bug in dns.c

[Will Lentz]

Although... 'id' is protected by locking dns_mutex, so multiple
concurrent DNS queries shouldn't be an issue.

Cheers,
Will

-----Original Message-----
From: Will Lentz 
Sent: Thursday, October 01, 2009 4:53 PM
To: 'Jay Foster'
Cc: ecos-discuss@ecos.sourceware.org
Subject: RE: [ECOS] 'id' rollover bug in dns.c

Thanks!  That's a better solution.

Will 

-----Original Message-----
From: Jay Foster [mailto:jay@systech.com] 
Sent: Thursday, October 01, 2009 4:38 PM
To: Will Lentz
Cc: ecos-discuss@ecos.sourceware.org
Subject: Re: [ECOS] 'id' rollover bug in dns.c

I discovered that issue some time ago and resolved it in a different 
manner that also allows multiple concurrent DNS queries to be sent from 
different threads.  The solution was to change the declaration of 'id' 
to be unsigned to match the definition of id in the dns_header
structure.

-static short id = 0;
+static unsigned short id = 0;

Then declare a new variable in send_recv()

     unsigned short req_id;

Then assign this variable with the ID value contained in the DNS request

     dns_hdr = (struct dns_header *) &msg_buf[0];
+    req_id = ((struct dns_header *)msg)->id;
     do {
         /* Send a request to each configured DNS server */

Then compare the ID in the responses with req_id

                 if (dns_hdr->id != req_id) {
                     continue;
                 }

This avoids ignoring valid DNS responses when the global variable, id, 
is incremented by another concurrent DNS request.

Jay

On 10/1/2009 4:11 PM, Will Lentz wrote:
> Hi,
>
> There is a rollover bug in the current CVS copy of dns.c. Around line
> 237 there is the following code:
>              /* Reply to an old query. Ignore it */
>              if (ntohs(dns_hdr->id) != (id-1)) {
>
> If dns_hdr->id == 5 and id == 6, then the 'if' condition is false as
> expected.
>
> If dns_hdr->id == 0xFFFF and id == 0, then the 'if' condition is
> incorrectly true.
>
> The simple patch below fixes the issue:
> --- dns_old.c   2009-10-01 16:01:41.000000000 -0700
> +++ dns.c       2009-10-01 16:01:45.000000000 -0700
> @@ -234,7 +234,7 @@
>               }
>
>               /* Reply to an old query. Ignore it */
> -            if (ntohs(dns_hdr->id) != (id-1)) {
> +            if (ntohs(dns_hdr->id) != (cyg_uint16)(id-1)) {
>                   continue;
>               }
>               finished = true;
>
>
> Cheers,
> Will
>
>    

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

Re: [ECOS] 'id' rollover bug in dns.c

[Jay Foster]

Perhaps.  My DNS code has several other changes to allow this and other 
things.  It's been a while since I've examined this so I have forgotten 
the details.
Jay

On 10/1/2009 5:15 PM, Will Lentz wrote:
> Although... 'id' is protected by locking dns_mutex, so multiple
> concurrent DNS queries shouldn't be an issue.
>
> Cheers,
> Will
>
> -----Original Message-----
> From: Will Lentz
> Sent: Thursday, October 01, 2009 4:53 PM
> To: 'Jay Foster'
> Cc: ecos-discuss@ecos.sourceware.org
> Subject: RE: [ECOS] 'id' rollover bug in dns.c
>
> Thanks!  That's a better solution.
>
> Will
>
> -----Original Message-----
> From: Jay Foster [mailto:jay@systech.com]
> Sent: Thursday, October 01, 2009 4:38 PM
> To: Will Lentz
> Cc: ecos-discuss@ecos.sourceware.org
> Subject: Re: [ECOS] 'id' rollover bug in dns.c
>
> I discovered that issue some time ago and resolved it in a different
> manner that also allows multiple concurrent DNS queries to be sent from
> different threads.  The solution was to change the declaration of 'id'
> to be unsigned to match the definition of id in the dns_header
> structure.
>
> -static short id = 0;
> +static unsigned short id = 0;
>
> Then declare a new variable in send_recv()
>
>       unsigned short req_id;
>
> Then assign this variable with the ID value contained in the DNS request
>
>       dns_hdr = (struct dns_header *)&msg_buf[0];
> +    req_id = ((struct dns_header *)msg)->id;
>       do {
>           /* Send a request to each configured DNS server */
>
> Then compare the ID in the responses with req_id
>
>                   if (dns_hdr->id != req_id) {
>                       continue;
>                   }
>
> This avoids ignoring valid DNS responses when the global variable, id,
> is incremented by another concurrent DNS request.
>
> Jay
>
> On 10/1/2009 4:11 PM, Will Lentz wrote:
>    
>> Hi,
>>
>> There is a rollover bug in the current CVS copy of dns.c. Around line
>> 237 there is the following code:
>>               /* Reply to an old query. Ignore it */
>>               if (ntohs(dns_hdr->id) != (id-1)) {
>>
>> If dns_hdr->id == 5 and id == 6, then the 'if' condition is false as
>> expected.
>>
>> If dns_hdr->id == 0xFFFF and id == 0, then the 'if' condition is
>> incorrectly true.
>>
>> The simple patch below fixes the issue:
>> --- dns_old.c   2009-10-01 16:01:41.000000000 -0700
>> +++ dns.c       2009-10-01 16:01:45.000000000 -0700
>> @@ -234,7 +234,7 @@
>>                }
>>
>>                /* Reply to an old query. Ignore it */
>> -            if (ntohs(dns_hdr->id) != (id-1)) {
>> +            if (ntohs(dns_hdr->id) != (cyg_uint16)(id-1)) {
>>                    continue;
>>                }
>>                finished = true;
>>
>>
>> Cheers,
>> Will
>>
>>
>>      
>    

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

RE: [ECOS] 'id' rollover bug in dns.c

[Will Lentz]

I entered the following bug so hopefully either fix will get into CVS:
https://bugzilla.ecoscentric.com/show_bug.cgi?id=1000835 


-----Original Message-----
From: Jay Foster [mailto:jay@systech.com] 
Sent: Thursday, October 01, 2009 6:12 PM
To: Will Lentz
Cc: ecos-discuss@ecos.sourceware.org
Subject: Re: [ECOS] 'id' rollover bug in dns.c

Perhaps.  My DNS code has several other changes to allow this and other 
things.  It's been a while since I've examined this so I have forgotten 
the details.
Jay

On 10/1/2009 5:15 PM, Will Lentz wrote:
> Although... 'id' is protected by locking dns_mutex, so multiple
> concurrent DNS queries shouldn't be an issue.
>
> Cheers,
> Will
>
> -----Original Message-----
> From: Will Lentz
> Sent: Thursday, October 01, 2009 4:53 PM
> To: 'Jay Foster'
> Cc: ecos-discuss@ecos.sourceware.org
> Subject: RE: [ECOS] 'id' rollover bug in dns.c
>
> Thanks!  That's a better solution.
>
> Will
>
> -----Original Message-----
> From: Jay Foster [mailto:jay@systech.com]
> Sent: Thursday, October 01, 2009 4:38 PM
> To: Will Lentz
> Cc: ecos-discuss@ecos.sourceware.org
> Subject: Re: [ECOS] 'id' rollover bug in dns.c
>
> I discovered that issue some time ago and resolved it in a different
> manner that also allows multiple concurrent DNS queries to be sent
from
> different threads.  The solution was to change the declaration of 'id'
> to be unsigned to match the definition of id in the dns_header
> structure.
>
> -static short id = 0;
> +static unsigned short id = 0;
>
> Then declare a new variable in send_recv()
>
>       unsigned short req_id;
>
> Then assign this variable with the ID value contained in the DNS
request
>
>       dns_hdr = (struct dns_header *)&msg_buf[0];
> +    req_id = ((struct dns_header *)msg)->id;
>       do {
>           /* Send a request to each configured DNS server */
>
> Then compare the ID in the responses with req_id
>
>                   if (dns_hdr->id != req_id) {
>                       continue;
>                   }
>
> This avoids ignoring valid DNS responses when the global variable, id,
> is incremented by another concurrent DNS request.
>
> Jay
>
> On 10/1/2009 4:11 PM, Will Lentz wrote:
>    
>> Hi,
>>
>> There is a rollover bug in the current CVS copy of dns.c. Around line
>> 237 there is the following code:
>>               /* Reply to an old query. Ignore it */
>>               if (ntohs(dns_hdr->id) != (id-1)) {
>>
>> If dns_hdr->id == 5 and id == 6, then the 'if' condition is false as
>> expected.
>>
>> If dns_hdr->id == 0xFFFF and id == 0, then the 'if' condition is
>> incorrectly true.
>>
>> The simple patch below fixes the issue:
>> --- dns_old.c   2009-10-01 16:01:41.000000000 -0700
>> +++ dns.c       2009-10-01 16:01:45.000000000 -0700
>> @@ -234,7 +234,7 @@
>>                }
>>
>>                /* Reply to an old query. Ignore it */
>> -            if (ntohs(dns_hdr->id) != (id-1)) {
>> +            if (ntohs(dns_hdr->id) != (cyg_uint16)(id-1)) {
>>                    continue;
>>                }
>>                finished = true;
>>
>>
>> Cheers,
>> Will
>>
>>
>>      
>    

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