diff mbox

[2/4] resolv: fix nsaddr_list array indexing

Message ID 1402673533-13243-3-git-send-email-aurelien@aurel32.net
State Changes Requested, archived
Headers

Commit Message

Aurelien Jarno June 13, 2014, 3:32 p.m. UTC 
  This commit is basically a revert of commits 6f5c3117 and 16b7dc27.
Those commits changed the indexing of nsaddr_list array when reading
resolv.conf, with the goal to simplify the mapping between the
nsaddr_list and nsaddrs, by making sure we don't have any name server at
the same both arrays. However the code in __libc_res_nsend hasn't been
updated, and still assume that all nsaddr_list entries are located at
the beginning of the array, without any gap. This causes some issues
when IPv6 name servers are in use.

2014-06-13  Aurelien Jarno  <aurelien@aurel32.net>
 
        [BZ #17053]
	* resolv/res_init.c (__res_vinit): Fill in IPv4 name server
	information using the nserv index. Only count IPv4 name servers
	in statp->nscount.

Comments

Siddhesh Poyarekar June 20, 2014, 2:56 a.m. UTC | #1 
On Fri, Jun 13, 2014 at 05:32:11PM +0200, Aurelien Jarno wrote:
> This commit is basically a revert of commits 6f5c3117 and 16b7dc27.
> Those commits changed the indexing of nsaddr_list array when reading
> resolv.conf, with the goal to simplify the mapping between the
> nsaddr_list and nsaddrs, by making sure we don't have any name server at
> the same both arrays. However the code in __libc_res_nsend hasn't been
> updated, and still assume that all nsaddr_list entries are located at
> the beginning of the array, without any gap. This causes some issues
> when IPv6 name servers are in use.
> 
> 2014-06-13  Aurelien Jarno  <aurelien@aurel32.net>
>  
>         [BZ #17053]
> 	* resolv/res_init.c (__res_vinit): Fill in IPv4 name server
> 	information using the nserv index. Only count IPv4 name servers
> 	in statp->nscount.

Looks good to me.

Siddhesh

>  
> diff --git a/resolv/res_init.c b/resolv/res_init.c
> index bdec4d9..37004ab 100644
> --- a/resolv/res_init.c
> +++ b/resolv/res_init.c
> @@ -308,9 +308,9 @@ __res_vinit(res_state statp, int preinit) {
>  			cp++;
>  		    if ((*cp != '\0') && (*cp != '\n')
>  			&& __inet_aton(cp, &a)) {
> -			statp->nsaddr_list[nservall].sin_addr = a;
> -			statp->nsaddr_list[nservall].sin_family = AF_INET;
> -			statp->nsaddr_list[nservall].sin_port =
> +			statp->nsaddr_list[nserv].sin_addr = a;
> +			statp->nsaddr_list[nserv].sin_family = AF_INET;
> +			statp->nsaddr_list[nserv].sin_port =
>  				htons(NAMESERVER_PORT);
>  			nserv++;
>  #ifdef _LIBC
> @@ -414,7 +414,7 @@ __res_vinit(res_state statp, int preinit) {
>  		    continue;
>  		}
>  	    }
> -	    statp->nscount = nservall;
> +	    statp->nscount = nserv;
>  #ifdef _LIBC
>  	    if (nservall - nserv > 0) {
>  		statp->_u._ext.nscount6 = nservall - nserv;
> -- 
> 2.0.0
>
H.J. Lu Jan. 6, 2015, 4:39 p.m. UTC | #2 
On Thu, Jun 19, 2014 at 7:56 PM, Siddhesh Poyarekar <siddhesh@redhat.com> wrote:
> On Fri, Jun 13, 2014 at 05:32:11PM +0200, Aurelien Jarno wrote:
>> This commit is basically a revert of commits 6f5c3117 and 16b7dc27.
>> Those commits changed the indexing of nsaddr_list array when reading
>> resolv.conf, with the goal to simplify the mapping between the
>> nsaddr_list and nsaddrs, by making sure we don't have any name server at
>> the same both arrays. However the code in __libc_res_nsend hasn't been
>> updated, and still assume that all nsaddr_list entries are located at
>> the beginning of the array, without any gap. This causes some issues
>> when IPv6 name servers are in use.
>>
>> 2014-06-13  Aurelien Jarno  <aurelien@aurel32.net>
>>
>>         [BZ #17053]
>>       * resolv/res_init.c (__res_vinit): Fill in IPv4 name server
>>       information using the nserv index. Only count IPv4 name servers
>>       in statp->nscount.
>
> Looks good to me.
>
> Siddhesh
>
>>
>> diff --git a/resolv/res_init.c b/resolv/res_init.c
>> index bdec4d9..37004ab 100644
>> --- a/resolv/res_init.c
>> +++ b/resolv/res_init.c
>> @@ -308,9 +308,9 @@ __res_vinit(res_state statp, int preinit) {
>>                       cp++;
>>                   if ((*cp != '\0') && (*cp != '\n')
>>                       && __inet_aton(cp, &a)) {
>> -                     statp->nsaddr_list[nservall].sin_addr = a;
>> -                     statp->nsaddr_list[nservall].sin_family = AF_INET;
>> -                     statp->nsaddr_list[nservall].sin_port =
>> +                     statp->nsaddr_list[nserv].sin_addr = a;
>> +                     statp->nsaddr_list[nserv].sin_family = AF_INET;
>> +                     statp->nsaddr_list[nserv].sin_port =
>>                               htons(NAMESERVER_PORT);
>>                       nserv++;
>>  #ifdef _LIBC
>> @@ -414,7 +414,7 @@ __res_vinit(res_state statp, int preinit) {
>>                   continue;
>>               }
>>           }
>> -         statp->nscount = nservall;
>> +         statp->nscount = nserv;
>>  #ifdef _LIBC
>>           if (nservall - nserv > 0) {
>>               statp->_u._ext.nscount6 = nservall - nserv;
>> --
>> 2.0.0
>>

There is a concern:

https://lists.debian.org/debian-glibc/2014/08/msg00068.html
diff mbox

Patch

diff --git a/resolv/res_init.c b/resolv/res_init.c
index bdec4d9..37004ab 100644
--- a/resolv/res_init.c
+++ b/resolv/res_init.c
@@ -308,9 +308,9 @@  __res_vinit(res_state statp, int preinit) {
 			cp++;
 		    if ((*cp != '\0') && (*cp != '\n')
 			&& __inet_aton(cp, &a)) {
-			statp->nsaddr_list[nservall].sin_addr = a;
-			statp->nsaddr_list[nservall].sin_family = AF_INET;
-			statp->nsaddr_list[nservall].sin_port =
+			statp->nsaddr_list[nserv].sin_addr = a;
+			statp->nsaddr_list[nserv].sin_family = AF_INET;
+			statp->nsaddr_list[nserv].sin_port =
 				htons(NAMESERVER_PORT);
 			nserv++;
 #ifdef _LIBC
@@ -414,7 +414,7 @@  __res_vinit(res_state statp, int preinit) {
 		    continue;
 		}
 	    }
-	    statp->nscount = nservall;
+	    statp->nscount = nserv;
 #ifdef _LIBC
 	    if (nservall - nserv > 0) {
 		statp->_u._ext.nscount6 = nservall - nserv;