This is the mail archive of the
libc-help@sourceware.org
mailing list for the glibc project.
Re: thread safety level of fwide
- From: Torvald Riegel <triegel at redhat dot com>
- To: Alexandre Oliva <aoliva at redhat dot com>
- Cc: MaShimiao <mashimiao dot fnst at cn dot fujitsu dot com>, "Carlos O'Donell" <carlos at redhat dot com>, libc-help at sourceware dot org, GLIBC Devel <libc-alpha at sourceware dot org>
- Date: Thu, 20 Nov 2014 13:59:07 +0100
- Subject: Re: thread safety level of fwide
- Authentication-results: sourceware.org; auth=none
- References: <546C4388 dot 1090209 at cn dot fujitsu dot com> <or61eborsv dot fsf at free dot home>
On Wed, 2014-11-19 at 18:32 -0200, Alexandre Oliva wrote:
> On Nov 19, 2014, MaShimiao <mashimiao.fnst@cn.fujitsu.com> wrote:
>
> > After reading it's source code, I think it should be marked with race:stream.
> > The reasoning is fwide() uses fp several times inside without holding the lock over all of them.
>
> > How do you think about that?
>
> The uses of fp, although technically racy, are well-defined, because of
> the way stream orientation works: once it's decided, it never changes,
> so it is safe to test, before taking a lock, whether it is already
> decided and use the settled value if so.
We have to consider two things here: data races on the value itself, and
data races on memory locations that have are related to the value.
When we change the accesses to fp->_mode to memory_order_relaxed
atomics, then there's no race on _mode anymore (and we should do that at
least). However, if a certain _mode value logically relates to anything
of what _IO_fwide does internally while holding the lock, then there's
likely a data race on that.
I've just glanced at a few pieces of code, but this may be a case of an
incorrect double-checked locking implementation. Whether that is the
case depends on whether there is an invariant between x and mode in the
example below (this uses C11-like atomic operations, and assumes the C11
memory model):
// Fast path
m = atomic_load_relaxed(&mode);
if (m != 0) return m;
else {
// Slow path
lock();
m = atomic_load_relaxed(&mode);
if (m != 0) return m;
x = 23;
atomic_store_relaxed(&mode, 1);
unlock();
}
If a return of != 0 relies on that x == 23, then this is not ensured by
concurrent invocations of this piece of code. This is because the fast
path's load does not ensure that x = 23 happens before it; it just uses
relaxed memory order, and it doesn't acquire the lock in the fast path.
If x == 23 never matters, then it's correct however.
A correct implementation of this would be:
m = atomic_load_acquire(&mode);
if (m != 0) return m;
else {
// Slow path
lock();
m = atomic_load_acquire(&mode);
if (m != 0) return m;
x = 23;
atomic_store_release(&mode, 1);
unlock();
}
Here, whenever a mode load reads-from the mode store, there is an
release/acquire pair, which results in synchronizes-with and thus
happens-before between the the x=23 store and the caller of this piece
of code.
nptl/pthread_once.c does something similar.
> It is only if it is not
> decided yet (when _mode is zero) and we want to set a value (when mode
> is nonzero) that we have to resort to mutual exclusion to serialize mode
> settings.
It seems -1 can be a possible value too, after _mode has been non-zero.
What are the invariants related to that?
> Torvald, this is another of those âtechnically a race, but arguably
> safeâ cases that you've been looking for in glibc. This idiom of using
> âundecidedâ initial values tested before taking locks is quite common in
> IOstreams, in TLS infrastructure, and in various __have_*
> feature-testing caches throughout glibc.
Hmm. So I guess we need to review them too unless there's just the
value that matters, but nothing else is logically related to it (IOW,
there's no invariant attached to a "decided" value).
We probably should go through all of them, convert to atomics, and check
whether relaxed memory order is fine or not. It is fine in several
cases, but often not.