Re: [PATCH] #13594: Avoid race in nscd

[Andreas Jaeger <aj at suse dot com>]

On 05/14/2012 10:17 PM, Roland McGrath wrote:
>> +/* Copyright (C) 1998-2012  Free Software Foundation, Inc.
> 
> Excess space here.
fixed


>>    @@ -100,9 +99,27 @@ libc_freeres_fn (hst_map_free)
> 
> Looks like a pasting problem in the patch.

That's the git diff output.

>> +  while (__builtin_expect
>> +	 (atomic_compare_and_exchange_val_acq (&__hst_map_handle.lock,
>> +					       1, 0) != 0, 0))
>> +    {
>> +      /* XXX Best number of rounds?  */
>> +      if (__builtin_expect (++cnt>  5, 0))
>> +	return 0;
> 
> Why limit the rounds?  Is it even kosher to return zero here?
> What effect does it have?

This is the same code as used in nscd/nscd_helper.c.  The existing
code already returns 0 in other cases, so returning it is fine, the
single caller can handle it.

>> -    return 0;
>> +    retval =  0;
> 
> Excess space here.

Fixed.

Ok to commit with the above fixes (below again)?

thanks for the review,
Andreas

diff --git a/nscd/nscd_gethst_r.c b/nscd/nscd_gethst_r.c
index c1661f8..956ebce 100644
--- a/nscd/nscd_gethst_r.c
+++ b/nscd/nscd_gethst_r.c
@@ -1,5 +1,4 @@
-/* Copyright (C) 1998-2005, 2006, 2007, 2008, 2009, 2011
-   Free Software Foundation, Inc.
+/* Copyright (C) 1998-2012 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
    Contributed by Ulrich Drepper <drepper@cygnus.com>, 1998.
 
@@ -100,9 +99,27 @@ libc_freeres_fn (hst_map_free)
 uint32_t
 __nscd_get_nl_timestamp (void)
 {
+  uint32_t retval;
   if (__nss_not_use_nscd_hosts != 0)
     return 0;
 
+  int cnt = 0;
+  /* __nscd_get_mapping can change hst_map_handle.mapped to NO_MAPPING.
+   However, __nscd_get_mapping assumes the prior value was not NO_MAPPING.
+   Thus we have to acquire the lock to prevent this thread from changing
+   hst_map_handle.mapped to NO_MAPPING while another thread is inside
+    __nscd_get_mapping.  */
+  while (__builtin_expect
+	 (atomic_compare_and_exchange_val_acq (&__hst_map_handle.lock,
+					       1, 0) != 0, 0))
+    {
+      /* XXX Best number of rounds?  */
+      if (__builtin_expect (++cnt > 5, 0))
+	return 0;
+
+      atomic_delay ();
+    }
+
   struct mapped_database *map = __hst_map_handle.mapped;
 
   if (map == NULL
@@ -112,9 +129,14 @@ __nscd_get_nl_timestamp (void)
     map = __nscd_get_mapping (GETFDHST, "hosts", &__hst_map_handle.mapped);
 
   if (map == NO_MAPPING)
-    return 0;
+    retval = 0;
+  else
+    retval =  map->head->extra_data[NSCD_HST_IDX_CONF_TIMESTAMP];
 
-  return map->head->extra_data[NSCD_HST_IDX_CONF_TIMESTAMP];
+  /* Release the lock.  */
+  __hst_map_handle.lock = 0;
+
+  return retval;
 }
 
 

-- 
 Andreas Jaeger aj@{suse.com,opensuse.org} Twitter/Identica: jaegerandi
  SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
   GF: Jeff Hawn,Jennifer Guild,Felix Imendörffer,HRB16746 (AG Nürnberg)
    GPG fingerprint = 93A3 365E CE47 B889 DF7F  FED1 389A 563C C272 A126