[#958652] Set EAI_SYSTEM only when h_errno is NETDB_INTERNAL

[Carlos O'Donell]

On 05/16/2013 02:06 AM, Siddhesh Poyarekar wrote:
> Hi,
> 
> A patch I had pushed into 2.16 had an error which resulted in the
> regression described in the aforementioned bug as well as the bug
> described in bz @15339.  Attached patch fixes this.  If it looks OK,
> I'll apply it in rawhide and f19.  I have also posted it upstream:

This class of change is interesting, in that nobody can easily review
it because it was a test-driven change. The patch adjusts the code
in response to feedback from tests run by users with observed versus
expected output being adjusted.

Reviewing the code is easy, it looks right, it isn't invalid C,
and it's only three lines of change.

However, the existing implementation is so complex and subtle that
it would take me days to review this code well.

What would help in the review of this, and the upstream review, is
to have a detailed breakdown of the analysis you did while fixing
this.

Do you have such an analysis?

I wish we had some white-box testing for these internals, similar 
to the testing I did for gettext with systemtap where you inject 
a failure on purpose.

We need to look at better testing for all of the networking
interfaces for glibc.

> http://sourceware.org/ml/libc-alpha/2013-05/msg00567.html
> 
> Siddhesh
> 
> commit 69cbe0434e5ddf223ed17141ae9527fb83fde1fa
> Author: Siddhesh Poyarekar <siddhesh at redhat.com>
> Date:   Wed May 15 20:06:20 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;
> diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
> index d368306..cdd869d 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,
>  			}
>  		    }
>  		  else
> -		    status = NSS_STATUS_UNAVAIL;
> +		    {
> +		      status = NSS_STATUS_UNAVAIL;
> +		      /* Could not load any of the lookup functions.  Indicate
> +		         an internal error if the file was found but some other
> +		         error led to the failure.  */
> +		      if (errno != 0 && errno != ENOENT)
> +			__set_h_errno (NETDB_INTERNAL);
> +		    }
>  		}
>  
>  	      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)
>  	    {
>  	      result = GAIH_OKIFUNSPEC | -EAI_SYSTEM;
>  	      goto free_and_return;
> _______________________________________________
> glibc mailing list
> glibc at lists.fedoraproject.org
> https://admin.fedoraproject.org/mailman/listinfo/glibc
>