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: Bernard Ogden <bernie dot ogden at linaro dot org>
- To: Roland McGrath <roland at hack dot frob dot com>
- Cc: "GNU C. Library" <libc-alpha at sourceware dot org>
- Date: Thu, 07 Aug 2014 13:11:03 +0100
- 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> <20140806212742 dot 1092C2C3981 at topped-with-meat dot com>
Changelog fixed (provided my mail client is now behaving).
I'm not sure how much detail to put into in the comment - is this better?
I may attempt some comments in other part of the file when I've got a little time.
2014-08-07 Bernard Ogden <bernie.ogden@linaro.org>
[BZ #16892]
* sysdeps/nptl/lowlevellock.h (__lll_timedlock): Use
atomic_compare_and_exchange_bool_acq rather than atomic_exchange_acq.
---
diff --git a/sysdeps/nptl/lowlevellock.h b/sysdeps/nptl/lowlevellock.h
index 548a9c8..28f4ba3 100644
--- a/sysdeps/nptl/lowlevellock.h
+++ b/sysdeps/nptl/lowlevellock.h
@@ -88,12 +88,15 @@ extern int __lll_timedlock_wait (int *futex, const struct
timespec *,
extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *,
int private) attribute_hidden;
+/* Take futex if it is untaken.
+ Otherwise block until either we get the futex or we time out. */
#define __lll_timedlock(futex, abstime, private) \
({ \
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; \
})
On 06/08/14 22:27, Roland McGrath wrote:
>> 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
>