Fix race condition in concurrent printf and fopen.

Fix race condition in concurrent printf and fopen.

Fixes a race condition reported by ThreadSanitizer that happens when fopen() and
printf() are invoked concurrently.  fopen() would traverse the list of all open
files and check their _flags for non-zero (in __sfp()), while printf would
attempt to modify the flags for _stdout via ORIENT(fp, -1) in vfprintf.c:679.

BUG=none, but related to http://code.google.com/p/nativeclient/issues/detail?id=870
TEST=./scons irt=0 platform=x86-64 run_newlib_stdio_test --verbose buildbot=tsan
     with patch: http://codereview.chromium.org/3298014
     (the fix does not remove the race reports completely, there is a remaining
     race on errno)

Committed: http://git.chromium.org/gitweb?p=native_client/nacl-newlib.git;a=commit;h=f5185a5

[Roland McGrath, 2011-08-09 18:28:39]

I think that's actually a false race report.  Simultaneous access to the word
will never make it momentarily zero.

[pasko-google - do not use, 2011-08-09 19:06:18]

On 2011/08/09 18:28:39, Roland McGrath wrote:
> I think that's actually a false race report.  Simultaneous access to the word
> will never make it momentarily zero.

I agree that in contemporary practice this code would hardly cause any harm, but
strictly speaking it is not guaranteed.  In the absence of  proper
synchronization a compiler is allowed to set intermediate values to memory,
assuming the execution is single-threaded, including a zero value.

It hardly hurts performance to add an extra brief lock in file opening, so I
would rather go with better correctness across architectures and compilers.

[pasko-google - do not use, 2011-08-09 20:46:48]

On 2011/08/09 19:06:18, pasko wrote:
> On 2011/08/09 18:28:39, Roland McGrath wrote:
> > I think that's actually a false race report.  Simultaneous access to the
word
> > will never make it momentarily zero.
> 
> I agree that in contemporary practice this code would hardly cause any harm,
but
> strictly speaking it is not guaranteed.  In the absence of  proper
> synchronization a compiler is allowed to set intermediate values to memory,
> assuming the execution is single-threaded, including a zero value.
> 
> It hardly hurts performance to add an extra brief lock in file opening, so I
> would rather go with better correctness across architectures and compilers.

Yet another argument to add more ordering here: enabling more tests to run under
ThreadSanitizer we would need either to add the lock or to annotate this type of
synchronization.  The latter would hurt readability more.

[Roland McGrath, 2011-08-09 20:57:12]

Adding unnecessary synchronization slows things down.
But I don't feel strongly enough to keep arguing.
LGTM

[pasko-google - do not use, 2011-08-10 13:41:25]

On Wed, Aug 10, 2011 at 12:57 AM,  <mcgrathr@chromium.org> wrote:
> Adding unnecessary synchronization slows things down.
> But I don't feel strongly enough to keep arguing.
> LGTM

thank you for review!


-- 
Egor Pasko

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.