[PATCH] getaddrinfo: Always allocate canonical name on the heap

[PATCH] getaddrinfo: Always allocate canonical name on the heap

[Florian Weimer]

A further simplification could eliminate the canon variable in
gaih_inet and replace it with canonbuf.  However, canonbuf is
used as a flag in the nscd code, which makes this somewhat
non-straightforward.

2017-05-11  Florian Weimer  <fweimer@redhat.com>

	* sysdeps/posix/getaddrinfo.c (getcanonname): New function.
	(gaih_inet): Remove malloc_canonbuf variable.  Call getcanonname.

diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
index dc02b11..d92db70 100644
--- a/sysdeps/posix/getaddrinfo.c
+++ b/sysdeps/posix/getaddrinfo.c
@@ -308,6 +308,30 @@ typedef enum nss_status (*nss_getcanonname_r)
    int *errnop, int *h_errnop);
 extern service_user *__nss_hosts_database attribute_hidden;
 
+/* This function is called if a canonical name is requested, but if
+   the service function did not provide it.  It tries to obtain the
+   name using getcanonname_r from the same service NIP.  If the name
+   cannot be canonicalized, return a copy of NAME.  Return NULL on
+   memory allocation failure.  The returned string is allocated on the
+   heap; the caller has to free it.  */
+static char *
+getcanonname (service_user *nip, struct gaih_addrtuple *at, const char *name)
+{
+  nss_getcanonname_r cfct = __nss_lookup_function (nip, "getcanonname_r");
+  char *s = (char *) name;
+  if (cfct != NULL)
+    {
+      char buf[256];
+      int herrno;
+      int rc;
+      if (DL_CALL_FCT (cfct, (at->name ?: name, buf, sizeof (buf),
+			      &s, &rc, &herrno)) != NSS_STATUS_SUCCESS)
+	/* If the canonical name cannot be determined, use the passed
+	   string.  */
+	s = (char *) name;
+    }
+  return strdup (name);
+}
 
 static int
 gaih_inet (const char *name, const struct gaih_service *service,
@@ -443,7 +467,6 @@ gaih_inet (const char *name, const struct gaih_service *service,
 
   bool malloc_name = false;
   struct gaih_addrtuple *addrmem = NULL;
-  bool malloc_canonbuf = false;
   char *canonbuf = NULL;
   int result = 0;
 
@@ -702,22 +725,13 @@ gaih_inet (const char *name, const struct gaih_service *service,
 			(*pat)->name = NULL;
 		      else if (canonbuf == NULL)
 			{
-			  size_t canonlen = strlen (air->canon) + 1;
-			  if ((req->ai_flags & AI_CANONIDN) != 0
-			      && __libc_use_alloca (alloca_used + canonlen))
-			    canonbuf = alloca_account (canonlen, alloca_used);
-			  else
+			  canonbuf = strdup (air->canon);
+			  if (canonbuf == NULL)
 			    {
-			      canonbuf = malloc (canonlen);
-			      if (canonbuf == NULL)
-				{
-				  result = -EAI_MEMORY;
-				  goto free_and_return;
-				}
-			      malloc_canonbuf = true;
+			      result = -EAI_MEMORY;
+			      goto free_and_return;
 			    }
-			  canon = (*pat)->name = memcpy (canonbuf, air->canon,
-							 canonlen);
+			  canon = (*pat)->name = canonbuf;
 			}
 
 		      if (air->family[i] == AF_INET
@@ -924,55 +938,16 @@ gaih_inet (const char *name, const struct gaih_service *service,
 			  if ((req->ai_flags & AI_CANONNAME) != 0
 			      && canon == NULL)
 			    {
-			      /* If we need the canonical name, get it
-				 from the same service as the result.  */
-			      nss_getcanonname_r cfct;
-			      int herrno;
-
-			      cfct = __nss_lookup_function (nip,
-							    "getcanonname_r");
-			      if (cfct != NULL)
+			      canonbuf = getcanonname (nip, at, name);
+			      if (canonbuf == NULL)
 				{
-				  const size_t max_fqdn_len = 256;
-				  if ((req->ai_flags & AI_CANONIDN) != 0
-				      && __libc_use_alloca (alloca_used
-							    + max_fqdn_len))
-				    canonbuf = alloca_account (max_fqdn_len,
-							       alloca_used);
-				  else
-				    {
-				      canonbuf = malloc (max_fqdn_len);
-				      if (canonbuf == NULL)
-					{
-					  _res.options
-					    |= old_res_options
-					       & DEPRECATED_RES_USE_INET6;
-					  result = -EAI_MEMORY;
-					  goto free_and_return;
-					}
-				      malloc_canonbuf = true;
-				    }
-				  char *s;
-
-				  if (DL_CALL_FCT (cfct, (at->name ?: name,
-							  canonbuf,
-							  max_fqdn_len,
-							  &s, &rc, &herrno))
-				      == NSS_STATUS_SUCCESS)
-				    canon = s;
-				  else
-				    {
-				      /* If the canonical name cannot be
-					 determined, use the passed in
-					 string.  */
-				      if (malloc_canonbuf)
-					{
-					  free (canonbuf);
-					  malloc_canonbuf = false;
-					}
-				      canon = name;
-				    }
+				  _res.options
+				    |= old_res_options
+				    & DEPRECATED_RES_USE_INET6;
+				  result = -EAI_MEMORY;
+				  goto free_and_return;
 				}
+			      canon = canonbuf;
 			    }
 			  status = NSS_STATUS_SUCCESS;
 			}
@@ -1118,9 +1093,10 @@ gaih_inet (const char *name, const struct gaih_service *service,
 #ifdef HAVE_LIBIDN
 	      make_copy:
 #endif
-		if (malloc_canonbuf)
-		  /* We already allocated the string using malloc.  */
-		  malloc_canonbuf = false;
+		if (canonbuf != NULL)
+		  /* We already allocated the string using malloc, but
+		     the buffer is now owned by canon.  */
+		  canonbuf = NULL;
 		else
 		  {
 		    canon = __strdup (canon);
@@ -1215,8 +1191,7 @@ gaih_inet (const char *name, const struct gaih_service *service,
   if (malloc_name)
     free ((char *) name);
   free (addrmem);
-  if (malloc_canonbuf)
-    free (canonbuf);
+  free (canonbuf);
 
   return result;
 }

Re: [PATCH] getaddrinfo: Always allocate canonical name on the heap

[Siddhesh Poyarekar]

On Thursday 11 May 2017 03:37 PM, Florian Weimer wrote:
> A further simplification could eliminate the canon variable in
> gaih_inet and replace it with canonbuf.  However, canonbuf is
> used as a flag in the nscd code, which makes this somewhat
> non-straightforward.
> 
> 2017-05-11  Florian Weimer  <fweimer@redhat.com>
> 
> 	* sysdeps/posix/getaddrinfo.c (getcanonname): New function.
> 	(gaih_inet): Remove malloc_canonbuf variable.  Call getcanonname.

LGTM (and the G here is not just good, it's great), thank you for
cleaning up!

Siddhesh

> diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
> index dc02b11..d92db70 100644
> --- a/sysdeps/posix/getaddrinfo.c
> +++ b/sysdeps/posix/getaddrinfo.c
> @@ -308,6 +308,30 @@ typedef enum nss_status (*nss_getcanonname_r)
>     int *errnop, int *h_errnop);
>  extern service_user *__nss_hosts_database attribute_hidden;
>  
> +/* This function is called if a canonical name is requested, but if
> +   the service function did not provide it.  It tries to obtain the
> +   name using getcanonname_r from the same service NIP.  If the name
> +   cannot be canonicalized, return a copy of NAME.  Return NULL on
> +   memory allocation failure.  The returned string is allocated on the
> +   heap; the caller has to free it.  */
> +static char *
> +getcanonname (service_user *nip, struct gaih_addrtuple *at, const char *name)
> +{
> +  nss_getcanonname_r cfct = __nss_lookup_function (nip, "getcanonname_r");
> +  char *s = (char *) name;
> +  if (cfct != NULL)
> +    {
> +      char buf[256];
> +      int herrno;
> +      int rc;
> +      if (DL_CALL_FCT (cfct, (at->name ?: name, buf, sizeof (buf),
> +			      &s, &rc, &herrno)) != NSS_STATUS_SUCCESS)
> +	/* If the canonical name cannot be determined, use the passed
> +	   string.  */
> +	s = (char *) name;
> +    }
> +  return strdup (name);
> +}
>  
>  static int
>  gaih_inet (const char *name, const struct gaih_service *service,
> @@ -443,7 +467,6 @@ gaih_inet (const char *name, const struct gaih_service *service,
>  
>    bool malloc_name = false;
>    struct gaih_addrtuple *addrmem = NULL;
> -  bool malloc_canonbuf = false;
>    char *canonbuf = NULL;
>    int result = 0;
>  
> @@ -702,22 +725,13 @@ gaih_inet (const char *name, const struct gaih_service *service,
>  			(*pat)->name = NULL;
>  		      else if (canonbuf == NULL)
>  			{
> -			  size_t canonlen = strlen (air->canon) + 1;
> -			  if ((req->ai_flags & AI_CANONIDN) != 0
> -			      && __libc_use_alloca (alloca_used + canonlen))
> -			    canonbuf = alloca_account (canonlen, alloca_used);
> -			  else
> +			  canonbuf = strdup (air->canon);
> +			  if (canonbuf == NULL)
>  			    {
> -			      canonbuf = malloc (canonlen);
> -			      if (canonbuf == NULL)
> -				{
> -				  result = -EAI_MEMORY;
> -				  goto free_and_return;
> -				}
> -			      malloc_canonbuf = true;
> +			      result = -EAI_MEMORY;
> +			      goto free_and_return;
>  			    }
> -			  canon = (*pat)->name = memcpy (canonbuf, air->canon,
> -							 canonlen);
> +			  canon = (*pat)->name = canonbuf;
>  			}
>  
>  		      if (air->family[i] == AF_INET
> @@ -924,55 +938,16 @@ gaih_inet (const char *name, const struct gaih_service *service,
>  			  if ((req->ai_flags & AI_CANONNAME) != 0
>  			      && canon == NULL)
>  			    {
> -			      /* If we need the canonical name, get it
> -				 from the same service as the result.  */
> -			      nss_getcanonname_r cfct;
> -			      int herrno;
> -
> -			      cfct = __nss_lookup_function (nip,
> -							    "getcanonname_r");
> -			      if (cfct != NULL)
> +			      canonbuf = getcanonname (nip, at, name);
> +			      if (canonbuf == NULL)
>  				{
> -				  const size_t max_fqdn_len = 256;
> -				  if ((req->ai_flags & AI_CANONIDN) != 0
> -				      && __libc_use_alloca (alloca_used
> -							    + max_fqdn_len))
> -				    canonbuf = alloca_account (max_fqdn_len,
> -							       alloca_used);
> -				  else
> -				    {
> -				      canonbuf = malloc (max_fqdn_len);
> -				      if (canonbuf == NULL)
> -					{
> -					  _res.options
> -					    |= old_res_options
> -					       & DEPRECATED_RES_USE_INET6;
> -					  result = -EAI_MEMORY;
> -					  goto free_and_return;
> -					}
> -				      malloc_canonbuf = true;
> -				    }
> -				  char *s;
> -
> -				  if (DL_CALL_FCT (cfct, (at->name ?: name,
> -							  canonbuf,
> -							  max_fqdn_len,
> -							  &s, &rc, &herrno))
> -				      == NSS_STATUS_SUCCESS)
> -				    canon = s;
> -				  else
> -				    {
> -				      /* If the canonical name cannot be
> -					 determined, use the passed in
> -					 string.  */
> -				      if (malloc_canonbuf)
> -					{
> -					  free (canonbuf);
> -					  malloc_canonbuf = false;
> -					}
> -				      canon = name;
> -				    }
> +				  _res.options
> +				    |= old_res_options
> +				    & DEPRECATED_RES_USE_INET6;
> +				  result = -EAI_MEMORY;
> +				  goto free_and_return;
>  				}
> +			      canon = canonbuf;
>  			    }
>  			  status = NSS_STATUS_SUCCESS;
>  			}
> @@ -1118,9 +1093,10 @@ gaih_inet (const char *name, const struct gaih_service *service,
>  #ifdef HAVE_LIBIDN
>  	      make_copy:
>  #endif
> -		if (malloc_canonbuf)
> -		  /* We already allocated the string using malloc.  */
> -		  malloc_canonbuf = false;
> +		if (canonbuf != NULL)
> +		  /* We already allocated the string using malloc, but
> +		     the buffer is now owned by canon.  */
> +		  canonbuf = NULL;
>  		else
>  		  {
>  		    canon = __strdup (canon);
> @@ -1215,8 +1191,7 @@ gaih_inet (const char *name, const struct gaih_service *service,
>    if (malloc_name)
>      free ((char *) name);
>    free (addrmem);
> -  if (malloc_canonbuf)
> -    free (canonbuf);
> +  free (canonbuf);
>  
>    return result;
>  }
> 

Re: [PATCH] getaddrinfo: Always allocate canonical name on the heap

[Adhemerval Zanella]

On 11/05/2017 07:07, Florian Weimer wrote:
> A further simplification could eliminate the canon variable in
> gaih_inet and replace it with canonbuf.  However, canonbuf is
> used as a flag in the nscd code, which makes this somewhat
> non-straightforward.
> 
> 2017-05-11  Florian Weimer  <fweimer@redhat.com>
> 
> 	* sysdeps/posix/getaddrinfo.c (getcanonname): New function.
> 	(gaih_inet): Remove malloc_canonbuf variable.  Call getcanonname.
> 
> diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
> index dc02b11..d92db70 100644
> --- a/sysdeps/posix/getaddrinfo.c
> +++ b/sysdeps/posix/getaddrinfo.c
> @@ -308,6 +308,30 @@ typedef enum nss_status (*nss_getcanonname_r)
>     int *errnop, int *h_errnop);
>  extern service_user *__nss_hosts_database attribute_hidden;
>  
> +/* This function is called if a canonical name is requested, but if
> +   the service function did not provide it.  It tries to obtain the
> +   name using getcanonname_r from the same service NIP.  If the name
> +   cannot be canonicalized, return a copy of NAME.  Return NULL on
> +   memory allocation failure.  The returned string is allocated on the
> +   heap; the caller has to free it.  */
> +static char *
> +getcanonname (service_user *nip, struct gaih_addrtuple *at, const char *name)
> +{
> +  nss_getcanonname_r cfct = __nss_lookup_function (nip, "getcanonname_r");
> +  char *s = (char *) name;
> +  if (cfct != NULL)
> +    {
> +      char buf[256];
> +      int herrno;
> +      int rc;
> +      if (DL_CALL_FCT (cfct, (at->name ?: name, buf, sizeof (buf),
> +			      &s, &rc, &herrno)) != NSS_STATUS_SUCCESS)
> +	/* If the canonical name cannot be determined, use the passed
> +	   string.  */
> +	s = (char *) name;
> +    }
> +  return strdup (name);
> +}

I think you need to use __strdup here, I am seeing check-local-plt failures on
master.

Re: [PATCH] getaddrinfo: Always allocate canonical name on the heap

[Florian Weimer]

On 06/02/2017 03:56 PM, Adhemerval Zanella wrote:
>> +static char *
>> +getcanonname (service_user *nip, struct gaih_addrtuple *at, const char *name)
>> +{

>> +  return strdup (name);
>> +}

> I think you need to use __strdup here, I am seeing check-local-plt failures on
> master.

On which architecture?  I don't get see it on x86-64.

I can push a fix blindly (based on the headers, it indeed needs to be
__strdup).

Thanks,
Florian

Re: [PATCH] getaddrinfo: Always allocate canonical name on the heap

[Adhemerval Zanella]

On 02/06/2017 11:05, Florian Weimer wrote:
> On 06/02/2017 03:56 PM, Adhemerval Zanella wrote:
>>> +static char *
>>> +getcanonname (service_user *nip, struct gaih_addrtuple *at, const char *name)
>>> +{
> 
>>> +  return strdup (name);
>>> +}
> 
>> I think you need to use __strdup here, I am seeing check-local-plt failures on
>> master.
> 
> On which architecture?  I don't get see it on x86-64.
> 
> I can push a fix blindly (based on the headers, it indeed needs to be
> __strdup).
> 
> Thanks,
> Florian
> 

I noticed on x86_64 (gcc 5.4), powerpc64le (gcc 5.4), aarch64 (gcc 4.9.2),
and sparc64 (gcc 6.3.1).  My guess is newer GCC versions are inline it.

Re: [PATCH] getaddrinfo: Always allocate canonical name on the heap

[Florian Weimer]

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

On 06/02/2017 04:10 PM, Adhemerval Zanella wrote:
> I noticed on x86_64 (gcc 5.4), powerpc64le (gcc 5.4), aarch64 (gcc 4.9.2),
> and sparc64 (gcc 6.3.1).  My guess is newer GCC versions are inline it.

Fair enough.  What about the attached patch?  I spotted one missing heap
allocation (which is not visible as a bug because the code doesn't try
to pass the pointer to free), and included that as well.

Thanks,
Florian

[-- Attachment #2: getaddrinfo-localplt.patch --]
[-- Type: text/x-patch, Size: 1347 bytes --]

getaddrinfo: Fix localplt failure involving strdup

2017-06-02  Florian Weimer  <fweimer@redhat.com>

	* sysdeps/posix/getaddrinfo.c (gethosts): Eliminate another
	strdupa.
	(getcanonname): Use __strdup instead of strdup.

diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
index d92db70..a8b5bb5 100644
--- a/sysdeps/posix/getaddrinfo.c
+++ b/sysdeps/posix/getaddrinfo.c
@@ -286,9 +286,16 @@ convert_hostent_to_gaih_addrtuple (const struct addrinfo *req,
 	}								      \
       *pat = addrmem;							      \
 									      \
-      if (localcanon !=	NULL && canon == NULL)				      \
-	canon = strdupa (localcanon);					      \
-									      \
+      if (localcanon != NULL && canon == NULL)				      \
+	{								      \
+	  canonbuf = __strdup (localcanon);				      \
+	  if (canonbuf == NULL)						      \
+	    {								      \
+	      result = -EAI_SYSTEM;					      \
+	      goto free_and_return;					      \
+	    }								      \
+	  canon = canonbuf;						      \
+	}								      \
       if (_family == AF_INET6 && *pat != NULL)				      \
 	got_ipv6 = true;						      \
     }									      \
@@ -330,7 +337,7 @@ getcanonname (service_user *nip, struct gaih_addrtuple *at, const char *name)
 	   string.  */
 	s = (char *) name;
     }
-  return strdup (name);
+  return __strdup (name);
 }
 
 static int

Re: [PATCH] getaddrinfo: Always allocate canonical name on the heap

[Adhemerval Zanella]

On 02/06/2017 11:37, Florian Weimer wrote:
> On 06/02/2017 04:10 PM, Adhemerval Zanella wrote:
>> I noticed on x86_64 (gcc 5.4), powerpc64le (gcc 5.4), aarch64 (gcc 4.9.2),
>> and sparc64 (gcc 6.3.1).  My guess is newer GCC versions are inline it.
> 
> Fair enough.  What about the attached patch?  I spotted one missing heap
> allocation (which is not visible as a bug because the code doesn't try
> to pass the pointer to free), and included that as well.
> 
> Thanks,
> Florian
> 

LGTM, thanks.

Re: [PATCH] getaddrinfo: Always allocate canonical name on the heap

[Joseph Myers]

I'm still seeing a localplt failure for strdup even after commit 
6257fcfd58479f6b7ae0fdde045b9ff144d543da.

https://sourceware.org/ml/libc-testresults/2017-q2/msg00276.html

(The strdup@plt reference is from gaih_inet.constprop.6, at least on 
x86_64.  It seems there is one plain strdup reference in 
sysdeps/posix/getaddrinfo.c.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] getaddrinfo: Always allocate canonical name on the heap

[Florian Weimer]

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

On 06/02/2017 11:26 PM, Joseph Myers wrote:
> I'm still seeing a localplt failure for strdup even after commit 
> 6257fcfd58479f6b7ae0fdde045b9ff144d543da.
> 
> https://sourceware.org/ml/libc-testresults/2017-q2/msg00276.html
> 
> (The strdup@plt reference is from gaih_inet.constprop.6, at least on 
> x86_64.  It seems there is one plain strdup reference in 
> sysdeps/posix/getaddrinfo.c.)

Right.  I figured out why I wasn't seeing this (localplt doesn't check
for local function relocation relocations in general, only PLT entries,
so -z blinds it), and verified that the attached patch fixes the
remaining failure.  Committed.

Thanks,
Florian

[-- Attachment #2: getaddrinfo-localplt-2.patch --]
[-- Type: text/x-patch, Size: 642 bytes --]

getaddrinfo: Eliminate another strdup call

2017-06-03  Florian Weimer  <fweimer@redhat.com>

	* sysdeps/posix/getaddrinfo.c (gaih_inet): Call __strdup instead
	of strdup.

diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
index a8b5bb5..a8bdd9a 100644
--- a/sysdeps/posix/getaddrinfo.c
+++ b/sysdeps/posix/getaddrinfo.c
@@ -732,7 +732,7 @@ gaih_inet (const char *name, const struct gaih_service *service,
 			(*pat)->name = NULL;
 		      else if (canonbuf == NULL)
 			{
-			  canonbuf = strdup (air->canon);
+			  canonbuf = __strdup (air->canon);
 			  if (canonbuf == NULL)
 			    {
 			      result = -EAI_MEMORY;