[PATCH] nscd: Drop local address tuple variable [BZ #29607]

[PATCH] nscd: Drop local address tuple variable [BZ #29607]

[Siddhesh Poyarekar]

When a request needs to be resent (e.g. due to insufficient buffer
space), the references to subsequent tuples in the local variable are
stale and should not be used.  This used to work by accident before, but
since 1d495912a it no longer does.  Instead of trying to reset it, just
let gethostbyname4_r write into TUMPBUF6 for us, thus maintaining a
consistent state at all times.  This is now consistent with what is done
in gaih_inet for getaddrinfo.

Resolves: BZ #29607
Reported-by: Holger Hoffstätte <holger@applied-asynchrony.com>
---

Tested on x86_64 with Fedora and nscd enabled.  Testing with other
distributions would be really appreciated!

Thanks,
Sid


 nscd/aicache.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/nscd/aicache.c b/nscd/aicache.c
index 51e793199f..e0baed170b 100644
--- a/nscd/aicache.c
+++ b/nscd/aicache.c
@@ -110,11 +110,10 @@ addhstaiX (struct database_dyn *db, int fd, request_header *req,
 							  "gethostbyname4_r");
       if (fct4 != NULL)
 	{
-	  struct gaih_addrtuple atmem;
 	  struct gaih_addrtuple *at;
 	  while (1)
 	    {
-	      at = &atmem;
+	      at = NULL;
 	      rc6 = 0;
 	      herrno = 0;
 	      status[1] = DL_CALL_FCT (fct4, (key, &at,
@@ -137,7 +136,7 @@ addhstaiX (struct database_dyn *db, int fd, request_header *req,
 	    goto next_nip;
 
 	  /* We found the data.  Count the addresses and the size.  */
-	  for (const struct gaih_addrtuple *at2 = at = &atmem; at2 != NULL;
+	  for (const struct gaih_addrtuple *at2 = at; at2 != NULL;
 	       at2 = at2->next)
 	    {
 	      ++naddrs;
-- 
2.37.2

Re: [PATCH] nscd: Drop local address tuple variable [BZ #29607]

[Holger Hoffstätte]

On 2022-10-04 02:06, Siddhesh Poyarekar wrote:
> When a request needs to be resent (e.g. due to insufficient buffer
> space), the references to subsequent tuples in the local variable are
> stale and should not be used.  This used to work by accident before, but
> since 1d495912a it no longer does.  Instead of trying to reset it, just
> let gethostbyname4_r write into TUMPBUF6 for us, thus maintaining a
> consistent state at all times.  This is now consistent with what is done
> in gaih_inet for getaddrinfo.
> 
> Resolves: BZ #29607
> Reported-by: Holger Hoffstätte <holger@applied-asynchrony.com>
> ---
> 
> Tested on x86_64 with Fedora and nscd enabled.  Testing with other
> distributions would be really appreciated!
> 
> Thanks,
> Sid
> 
> 
>   nscd/aicache.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/nscd/aicache.c b/nscd/aicache.c
> index 51e793199f..e0baed170b 100644
> --- a/nscd/aicache.c
> +++ b/nscd/aicache.c
> @@ -110,11 +110,10 @@ addhstaiX (struct database_dyn *db, int fd, request_header *req,
>   							  "gethostbyname4_r");
>         if (fct4 != NULL)
>   	{
> -	  struct gaih_addrtuple atmem;
>   	  struct gaih_addrtuple *at;
>   	  while (1)
>   	    {
> -	      at = &atmem;
> +	      at = NULL;
>   	      rc6 = 0;
>   	      herrno = 0;
>   	      status[1] = DL_CALL_FCT (fct4, (key, &at,
> @@ -137,7 +136,7 @@ addhstaiX (struct database_dyn *db, int fd, request_header *req,
>   	    goto next_nip;
>   
>   	  /* We found the data.  Count the addresses and the size.  */
> -	  for (const struct gaih_addrtuple *at2 = at = &atmem; at2 != NULL;
> +	  for (const struct gaih_addrtuple *at2 = at; at2 != NULL;
>   	       at2 = at2->next)
>   	    {
>   	      ++naddrs;
> 

Hello Siddesh,

I am happy to report that this seems to be working fine and fixes the reported bug;
there haven't been any nscd crashes for a few hours now on any of my machines.
Thanks!

Holger

Re: [PATCH] nscd: Drop local address tuple variable [BZ #29607]

[Siddhesh Poyarekar]

On 2022-10-04 02:49, Holger Hoffstätte wrote:
> I am happy to report that this seems to be working fine and fixes the 
> reported bug;
> there haven't been any nscd crashes for a few hours now on any of my 
> machines.
> Thanks!

Thank you for confirming Holger!

Tested-by: Holger Hoffstätte <holger@applied-asynchrony.com>

Re: [PATCH] nscd: Drop local address tuple variable [BZ #29607]

[Carlos O'Donell]

On Mon, Oct 03, 2022 at 08:06:57PM -0400, Siddhesh Poyarekar via Libc-alpha wrote:
> When a request needs to be resent (e.g. due to insufficient buffer
> space), the references to subsequent tuples in the local variable are
> stale and should not be used.  This used to work by accident before, but
> since 1d495912a it no longer does.  Instead of trying to reset it, just
> let gethostbyname4_r write into TUMPBUF6 for us, thus maintaining a
> consistent state at all times.  This is now consistent with what is done
> in gaih_inet for getaddrinfo.
 
LGTM.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> Resolves: BZ #29607
> Reported-by: Holger Hoffstätte <holger@applied-asynchrony.com>
> ---
> 
> Tested on x86_64 with Fedora and nscd enabled.  Testing with other
> distributions would be really appreciated!
> 
> Thanks,
> Sid
> 
> 
>  nscd/aicache.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/nscd/aicache.c b/nscd/aicache.c
> index 51e793199f..e0baed170b 100644
> --- a/nscd/aicache.c
> +++ b/nscd/aicache.c
> @@ -110,11 +110,10 @@ addhstaiX (struct database_dyn *db, int fd, request_header *req,
>  							  "gethostbyname4_r");
>        if (fct4 != NULL)
>  	{
> -	  struct gaih_addrtuple atmem;

OK. Remove the single static struct (no longer used).

>  	  struct gaih_addrtuple *at;
>  	  while (1)
>  	    {
> -	      at = &atmem;
> +	      at = NULL;

OK. Start with at NULL.

>  	      rc6 = 0;
>  	      herrno = 0;
>  	      status[1] = DL_CALL_FCT (fct4, (key, &at,
> @@ -137,7 +136,7 @@ addhstaiX (struct database_dyn *db, int fd, request_header *req,
>  	    goto next_nip;
>  
>  	  /* We found the data.  Count the addresses and the size.  */
> -	  for (const struct gaih_addrtuple *at2 = at = &atmem; at2 != NULL;
> +	  for (const struct gaih_addrtuple *at2 = at; at2 != NULL;

OK. Remove &atmem. We set at2 to at.

In all of these cases the memory is created by alloc_create_buffer() and
the memory for the gaih_addrtuple comes from that buffer. We don't want
to reset to &atmem sine this is no longer correct. The allocation is
done at the lowest level in fct4.

>  	       at2 = at2->next)
>  	    {
>  	      ++naddrs;
> -- 
> 2.37.2
>