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

["Carlos O'Donell" <carlos at systemhalted dot org>]

On Mon, May 14, 2012 at 4:11 PM, Andreas Jaeger <aj@suse.de> wrote:
>
> Jeff Law says in bugzilla:
> "Based on just reading the code, I wonder if a one thread is mucking up
> hst_map_handle.mapped behind the back of nscd_get_mapping.
>
> nscd_get_nl_timestamp doesn't bother to grab the hst_map_handle lock and
> calls
> into nscd_get_mapping which could potentially change hst_map_handle.mapped
> to
> NO_MAPPING.
>
> If this occurs after another thread had passed the NO_MAPPING check in
> nscd_get_map_ref, but hasn't yet hit the atomic_decrement_val in
> nscd_get_mapping then it could cause the failure mode reported in this
> report
> (and several others across various distros, upstream kde and possibly
> elsewhere)."
>
> I agree with his analysis and cleaned up the patch submission (added
> copyright header and ChangeLog entry).
>
> This patch has been tested in openSUSE and Fedora.
>
> Ok to commit? Andreas
>
> 2012-05-14 ?Jeff Law ?<law@redhat.com>
>
> ? ? ? ?[BZ #13594]
> ? ? ? ?* nscd/nscd_gethst_r.c (__nscd_get_nl_timestamp): Add locking to
> ? ? ? ?code changing __hst_map_handle.map.
>
> diff --git a/nscd/nscd_gethst_r.c b/nscd/nscd_gethst_r.c
> index c1661f8..36c4bb3 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 ();
> + ? ?}
> +

Roland has already OK'd the patch for checkin so my questions are
purely academic for now.

Why are we rolling a bespoke pthread_spin_lock()?

If we have threads, why aren't we using the pthreads interface?

> ? 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;

Is this sufficient to guarantee atomicity? Is it because we use the
*_acq sematic atomic that it works?

> +
> + ?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