This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH][BZ #16892] Check value of futex before updating in __lll_timedlock
- From: Roland McGrath <roland at hack dot frob dot com>
- To: Bernard Ogden <bernie dot ogden at linaro dot org>
- Cc: "GNU C. Library" <libc-alpha at sourceware dot org>
- Date: Wed, 6 Aug 2014 14:27:42 -0700 (PDT)
- Subject: Re: [PATCH][BZ #16892] Check value of futex before updating in __lll_timedlock
- Authentication-results: sourceware.org; auth=none
- References: <1D01BCB7-8B02-415F-9244-58C15296B799 at linaro dot org>
> This bug previously affected arm, aarch64, m68k and sh/sh4. Since mips
> switched to the generic lowlevellock.h, it is also affected. Applying
> this patch will fix arm, aarch64 and mips. m68k and sh would need to
> switch to the generic header to get the fix.
m68k uses the generic code now.
> Change is pretty simple, but has been built and tested on arm.
> 2014-08-05 Bernard Ogden <bernie.ogden@linaro.org>
>
> [BZ #16892]
> * sysdeps/nptl/lowlevellock.h: Use
> atomic_compare_and_exchange_bool_acq rather than atomic_exchange_acq
> in __lll_timedlock.
This should look like:
[BZ #16892]
* sysdeps/nptl/lowlevellock.h (__lll_timedlock): Use
atomic_compare_and_exchange_bool_acq rather than atomic_exchange_acq.
1. Use 1 tab to indent, not 8 spaces.
2. Put the name of the affected macro/function/variable in parens after the
file name, not just in regular English.
> --- a/sysdeps/nptl/lowlevellock.h
> +++ b/sysdeps/nptl/lowlevellock.h
> @@ -93,7 +93,8 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *,
> int *__futex = (futex); \
> int __val = 0; \
> \
> - if (__glibc_unlikely (atomic_exchange_acq (__futex, 1))) \
> + if (__glibc_unlikely \
> + (atomic_compare_and_exchange_bool_acq (__futex, 1, 0))) \
> __val = __lll_timedlock_wait (__futex, abstime, private); \
> __val; \
> })
Please add a comment explaining what the code is doing. The rest of the
file needs more comments like this and I'm not asking you to add all those
(unless you want to!). But where you're touching it, and especially
replacing one magic atomic operation with a different magic atomic
operation to fix a bug, a comment is really warranted. If there had been a
good comment on the original code, it probably would have prevented us from
letting the bug go by in the first place.
Thanks,
Roland