Re: thread safety level of fwide

[Torvald Riegel <triegel at redhat dot com>]

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.