Re: [ping][PATCH v2.1][BZ #15339] Avoid returning EAI_SYSTEM when there's a network error

["Carlos O'Donell" <carlos at redhat dot com>]

On 05/21/2013 03:14 AM, Siddhesh Poyarekar wrote:
>> 	[BZ #15339]
>> 	* nss/getXXbyYY_r.c (REENTRANT_NAME): Set NETDB_INTERNAL only
>> 	when no services were used.
>> 	* sysdeps/posix/getaddrinfo.c (gaih_inet): Set h_errno.
>> 	Return EAI_SYSTEM if h_errno is NETDB_INTERNAL.
>>
>> commit e656c197931865499a33db8237265ce9de860363
>> Author: Siddhesh Poyarekar <siddhesh@redhat.com>
>> Date:   Wed May 15 20:46:11 2013 +0530
>>
>>     Set EAI_SYSTEM only when h_errno is NETDB_INTERNAL
>>     
>>     Fixes BZ #15339.
>>     
>>     NSS_STATUS_UNAVAIL may mean that a necessary input resource is not
>>     available.  This could occur in a number of cases including when the
>>     network is down, system runs out of file descriptors, etc.  The
>>     correct differentiator in such a case is the h_errno, which gives the
>>     nature of failure.  In case of failures other than a simple 'not
>>     found', we set h_errno as NETDB_INTERNAL and let errno be the
>>     identifier for the exact error.
>>
>> diff --git a/nss/getXXbyYY_r.c b/nss/getXXbyYY_r.c
>> index 1067744..9e4d110 100644
>> --- a/nss/getXXbyYY_r.c
>> +++ b/nss/getXXbyYY_r.c
>> @@ -284,11 +284,11 @@ done:
>>  #endif
>>    *result = status == NSS_STATUS_SUCCESS ? resbuf : NULL;
>>  #ifdef NEED_H_ERRNO
>> -  if (status == NSS_STATUS_UNAVAIL)
>> -    /* Either we failed to lookup the functions or the functions themselves
>> -       had a system error.  Set NETDB_INTERNAL here to let the caller know
>> -       that the errno may have the real reason for failure.  */
>> -      *h_errnop = NETDB_INTERNAL;
>> +  if (status == NSS_STATUS_UNAVAIL && !any_service && errno != ENOENT)
>> +    /* This happens when we weren't able to use a service for reasons other
>> +       than the module not being found.  In such a case, we'd want to tell the
>> +       caller that errno has the real reason for failure.  */
>> +    *h_errnop = NETDB_INTERNAL;
>>    else if (status != NSS_STATUS_SUCCESS && !any_service)
>>      /* We were not able to use any service.  */
>>      *h_errnop = NO_RECOVERY;

OK.

In the event that the module was not found, we fall through and
eventually just `return errno;' which indicates ENOENT to the caller.

>> diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
>> index d368306..bde7dfe 100644
>> --- a/sysdeps/posix/getaddrinfo.c
>> +++ b/sysdeps/posix/getaddrinfo.c
>> @@ -1035,7 +1035,14 @@ gaih_inet (const char *name, const struct gaih_service *service,
>>  			}
>>  		    }

The code before this is:

                  nss_gethostbyname3_r fct = NULL;
                  if (req->ai_flags & AI_CANONNAME)
                    /* No need to use this function if we do not look for
                       the canonical name.  The function does not exist in
                       all NSS modules and therefore the lookup would
                       often fail.  */
                    fct = __nss_lookup_function (nip, "gethostbyname3_r");
                  if (fct == NULL)
                    /* We are cheating here.  The gethostbyname2_r
                       function does not have the same interface as
                       gethostbyname3_r but the extra arguments the
                       latter takes are added at the end.  So the
                       gethostbyname2_r code will just ignore them.  */
                    fct = __nss_lookup_function (nip, "gethostbyname2_r");

                  if (fct != NULL)
                    {


>>  		  else
>> -		    status = NSS_STATUS_UNAVAIL;
>> +		    {
>> +		      status = NSS_STATUS_UNAVAIL;
>> +		      /* Could not load any of the lookup functions.  Indicate
>> +		         an internal error if the failure was due to a system
>> +			 error other than the file not being found.  */
>> +		      if (errno != 0 && errno != ENOENT)
>> +			__set_h_errno (NETDB_INTERNAL);
>> +		    }
>>  		}

The real function being called is gethostbyname3_r, but if that for any
reason returns NULL we try gethostbyname2_r, which might also fail.
Here it would seem to me that we set errno potentially based on the errors
detected in calling the backup function instead of saving the real error
from the function we wanted to call.

It looks to me like we need to save errno after the failed call to
gethostbyname3_r and use that to set h_errno.

Comments?

>>  
>>  	      if (nss_next_action (nip, status) == NSS_ACTION_RETURN)
>> @@ -1049,7 +1056,7 @@ gaih_inet (const char *name, const struct gaih_service *service,
>>  
>>  	  _res.options |= old_res_options & RES_USE_INET6;
>>  
>> -	  if (status == NSS_STATUS_UNAVAIL)
>> +	  if (h_errno == NETDB_INTERNAL)

OK.

>>  	    {
>>  	      result = GAIH_OKIFUNSPEC | -EAI_SYSTEM;
>>  	      goto free_and_return;

Cheers,
Carlos.