This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] [BZ #17977] Fix potential hanging of gethostbyaddr_r/gethostbyname_r


On Fri, Jun 19, 2015 at 03:49:07AM +0300, Dmitry V. Levin wrote:
> [BZ #17977]
> * resolv/res_hconf.c (_res_hconf_reorder_addrs): Fix unlocking
> when initializing interface list.
> * resolv/tst-res_hconf_reorder.c: New test.
> * resolv/Makefile [$(have-thread-library) = yes] (tests): Add
> tst-res_hconf_reorder.
> ($(objpfx)tst-res_hconf_reorder): Depend on $(libdl)
> and $(shared-thread-library).
> (tst-res_hconf_reorder-ENV): New variable.

Can you please describe the problem and the proposed change in detail?
Also, it looks like the patch was proposed by Eric Newton and the test
case proposed by you.  It would be nice to mention that in the
ChangeLog snippet or at least in the email.

> ---
>  ChangeLog                      | 12 ++++++
>  NEWS                           | 20 ++++-----
>  resolv/res_hconf.c             |  6 +--
>  resolv/tst-res_hconf_reorder.c | 95 ++++++++++++++++++++++++++++++++++++++++++
>  resolv/Makefile                |  4 ++
>  5 files changed, 124 insertions(+), 13 deletions(-)
>  create mode 100644 resolv/tst-res_hconf_reorder.c
> 
> diff --git a/resolv/res_hconf.c b/resolv/res_hconf.c
> index b9c229d..0d4f3f4 100644
> --- a/resolv/res_hconf.c
> +++ b/resolv/res_hconf.c
> @@ -421,7 +421,7 @@ _res_hconf_reorder_addrs (struct hostent *hp)
>        /* Get lock.  */
>        __libc_lock_lock (lock);
>  
> -      /* Recheck, somebody else might have done the work by done.  */
> +      /* Recheck, somebody else might have done the work by now.  */

Trivial comment change, could go in as a separate obvious commit.

>        if (num_ifs <= 0)
>  	{
>  	  int new_num_ifs = 0;
> @@ -473,10 +473,10 @@ _res_hconf_reorder_addrs (struct hostent *hp)
>  	  errno = save;
>  
>  	  num_ifs = new_num_ifs;
> -
> -	  __libc_lock_unlock (lock);
>  	}
>  
> +      __libc_lock_unlock (lock);
> +
>        __close (sd);
>      }
>  
> diff --git a/resolv/tst-res_hconf_reorder.c b/resolv/tst-res_hconf_reorder.c
> new file mode 100644
> index 0000000..e874d94
> --- /dev/null
> +++ b/resolv/tst-res_hconf_reorder.c
> @@ -0,0 +1,95 @@
> +/* BZ #17977 _res_hconf_reorder_addrs test.
> +
> +   Copyright (C) 2015 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <errno.h>
> +#include <error.h>
> +#include <time.h>
> +#include <dlfcn.h>
> +#include <pthread.h>
> +#include <netdb.h>
> +#include <netinet/in.h>
> +#include <sys/socket.h>
> +
> +static struct timespec ts;
> +
> +void *
> +realloc (void *ptr, size_t len)
> +{
> +  static void *(*fun) (void *, size_t);
> +
> +  if (!fun)
> +    fun = dlsym (RTLD_NEXT, "realloc");
> +
> +  if (ts.tv_nsec)
> +    nanosleep (&ts, NULL);
> +
> +  return (*fun) (ptr, len);
> +}

Please add a comment to describe why this is necessary.

> +
> +static void *
> +resolve (void *arg)
> +{
> +  struct in_addr addr;
> +  struct hostent ent;
> +  struct hostent *result;
> +  int err;
> +  char buf[1024];
> +
> +  addr.s_addr = htonl (INADDR_LOOPBACK);
> +  (void) gethostbyaddr_r ((void *) &addr, sizeof (addr), AF_INET,
> +		          &ent, buf, sizeof (buf), &result, &err);
> +  return arg;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  #define N 3
> +  pthread_t thr[N];
> +  unsigned int i;
> +  int result = 0;
> +
> +  ts.tv_nsec = 100000000;
> +
> +  for (i = 0; i < N; ++i)
> +    {
> +      int rc = pthread_create (&thr[i], NULL, resolve, NULL);
> +
> +      if (rc)
> +	error (1, rc, "pthread_create");

Use printf since error() will print to stderr and we don't want that.
We want the errors to be captured in the .out file.

> +    }
> +
> +  for (i = 0; i < N; ++i)
> +    {
> +      void *retval;
> +      int rc = pthread_join (thr[i], &retval);
> +
> +      if (rc)
> +	error (1, rc, "pthread_join");
> +      if (retval)
> +	result = 1;
> +    }
> +
> +  ts.tv_nsec = 0;

Why is this needed?

> +
> +  return result;
> +}
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> diff --git a/resolv/Makefile b/resolv/Makefile
> index f62eea4..3509d98 100644
> --- a/resolv/Makefile
> +++ b/resolv/Makefile
> @@ -39,6 +39,7 @@ extra-libs := libresolv libnss_dns
>  ifeq ($(have-thread-library),yes)
>  extra-libs += libanl
>  routines += gai_sigqueue
> +tests += tst-res_hconf_reorder
>  endif
>  extra-libs-others = $(extra-libs)
>  libresolv-routines := gethnamaddr res_comp res_debug	\
> @@ -98,6 +99,9 @@ $(objpfx)libanl.so: $(shared-thread-library)
>  
>  $(objpfx)ga_test: $(objpfx)libanl.so $(shared-thread-library)
>  
> +$(objpfx)tst-res_hconf_reorder: $(libdl) $(shared-thread-library)
> +tst-res_hconf_reorder-ENV = RESOLV_REORDER=on
> +
>  $(objpfx)tst-leaks: $(objpfx)libresolv.so
>  tst-leaks-ENV = MALLOC_TRACE=$(objpfx)tst-leaks.mtrace
>  $(objpfx)mtrace-tst-leaks.out: $(objpfx)tst-leaks.out
> 
> -- 
> ldv


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]