[PATCH v2 4/4] libio: Fix race access to _IO_FLAGS2_NEED_LOCK

[Adhemerval Zanella]

On 10/05/2021 14:46, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> The flag is set on:
>>
>>   1. the stream creation (for memstream and wmemstream).
>>   2. by pthread_create when it detects the process become multi-thread.
>>   3. by flockfile.
>>
>> The 1. and 2. cases do not generate a race condition since for 1. the
>> stream reference is not accessible by other threads and for 2. the
>> process is still single-threaded.
>>
>> However, for 3. the functions feof, ferror, fputc, getc, getchar,
>> ungetc, and putc access it without locking the stream to check whether
>> to use the optimized _unlocked variants.  So to avoid a racy condition,
>> both the check and set are done using atomic operations.
> 
> Sorry, I don't think this works, purely for performance reasons.
> 
> Here's why:
> 
>> diff --git a/libio/feof.c b/libio/feof.c
>> index 8275321788..823b40637a 100644
>> --- a/libio/feof.c
>> +++ b/libio/feof.c
>> @@ -32,7 +32,7 @@ _IO_feof (FILE *fp)
>>  {
>>    int result;
>>    CHECK_FILE (fp, EOF);
>> -  if (!_IO_need_lock (fp))
>> +  if (!io_need_lock (fp))
>>      return _IO_feof_unlocked (fp);
>>    _IO_flockfile (fp);
>>    result = _IO_feof_unlocked (fp);
> 
> io_need_lock is used on the fast path here.
> 
>> diff --git a/libio/libio.h b/libio/libio.h
>> index 511b39457f..6b569511ec 100644
>> --- a/libio/libio.h
>> +++ b/libio/libio.h
> 
>> +/* The _IO_FLAGS2_NEED_LOCK flag is set on:
>> +
>> +   1. the stream creation (for memstream and wmemstream).
>> +   2. by pthread_create when it detects the process become multi-thread.
>> +   3. by flockfile.
>> +
>> +   The 1. and 2. cases do not generate a race condition since for 1.
>> +   the stream reference is not accessible by other threads and for 2.
>> +   the process is still single-threaded.
>> +
>> +   However, for 3. the functions feof, ferror, fputc, getc, getchar, ungetc,
>> +   and putc access it without locking the stream to check whether to use the
>> +   optimized _unlocked variants.  So to avoid a racy condition, both the
>> +   check and set are done using atomic operations.  */
>> +
>> +static inline bool
>> +io_need_lock (FILE *fp)
>> +{
>> +  return atomic_load_acquire (&fp->_flags2) & _IO_FLAGS2_NEED_LOCK;
>> +}
> 
> And this means we have an acquire load on the fast path.  And I think
> that's … not good on the architecture for which the optimization was
> written.
> 
>> diff --git a/stdio-common/flockfile.c b/stdio-common/flockfile.c
>> index a66e0a731e..3b42fe891d 100644
>> --- a/stdio-common/flockfile.c
>> +++ b/stdio-common/flockfile.c
>> @@ -22,7 +22,7 @@
>>  void
>>  __flockfile (FILE *stream)
>>  {
>> -  stream->_flags2 |= _IO_FLAGS2_NEED_LOCK;
>> +  io_set_need_lock (stream);
>>    _IO_lock_lock (*stream->_lock);
>>  }
>>  weak_alias (__flockfile, flockfile);
> 
> My impression is that _IO_FLAGS2_NEED_LOCK is set here for consistency,
> not for concurrency control (i.e., so that other internal locking
> operations produce the desired result).

Indeed thinking again, the _IO_FLAGS2_NEED_LOCK is similar to
__libc_single_threaded: once the process goes multithread 
_IO_FLAGS2_NEED_LOCK is set and all stream function should take
the lock.  And currently there is no support to reset to default
(which is also tricky because open_memstream should always have it
set).

> 
> As you pointed out, pthread_create automatically sets
> _IO_FLAGS2_NEED_LOCK in those cases where it is not set by default.  So
> the _flags2 update only matters in the single-threaded case.  But the
> lock does not matter, so we can do this instead:
> 
>   void
>   __flockfile (FILE *stream)
>   {
>     _IO_lock_lock (*stream->_lock);
>     stream->_flags2 |= _IO_FLAGS2_NEED_LOCK;
>   }
> 
> Since the _flags2 update happens after the lock operation, the data race
> is gone.
> 
> Ideally this should have a comment, but I'm not sure what to write
> there.

I see that my first assumption of moving the _IO_FLAGS2_NEED_LOCK after
the lock should be suffice.  The data race is not really related to 
_IO_FLAGS2_NEED_LOCK set in fact, but rather the small window where 
the _flags2 read and update with _IO_FLAGS2_NEED_LOCK might lost some
bits update.

Maybe the comment:

/* The _IO_FLAGS2_NEED_LOCK is required here to enable the non-atomic
   fast path on function that call _IO_need_lock.  However it requires
   to be set with stream locked to avoid a potential data race.  */

> 
> With more careful review, maybe we could eliminate the _flags2 update
> altogether.  What we cannot do is make the flockfile operation itself
> conditional on _IO_FLAGS2_NEED_LOCK because of observable impact
> vis-a-vis funlockfile.  If we want to optimize flockfile, I think we'd
> have to change _IO_lock_lock and add an atomics-free fast path for
> SINGLE_THREAD_P (that still performs the lock update).  With that, the
> entire _IO_FLAGS2_NEED_LOCK business could be removed again, I think.

I am not sure we could eliminate since we need a way to inform the
function that call _IO_need_lock that they can elide _IO_flockfile.
The single-thread optimization won't really work: um multithread 
process the idea exactly to make a set of stream operation to avoid
taking the lock.

And _IO_FILE::_flag does not see to have space (high-order word is
_IO_MAGIC and all flags seems to be used).