Issue 15768 - std.stdio.File does not support __gshared semantics of stdout/err/in
Summary: std.stdio.File does not support __gshared semantics of stdout/err/in
Status: RESOLVED FIXED
Alias: None
Product: D
Classification: Unclassified
Component: phobos (show other issues)
Version: D2
Hardware: All All
: P1 critical
Assignee: No Owner
URL: http://forum.dlang.org/post/vrchiulms...
Keywords: safe
: 13443 (view as issue list)
Depends on:
Blocks:
 
Reported: 2016-03-06 01:20 UTC by Marco Leise
Modified: 2018-04-06 15:11 UTC (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description Marco Leise 2016-03-06 01:20:46 UTC
std.stdio.trustedStdout returns a copy of stdout which invokes the postblit of File. This is done without internal synchronization and so the reference count increment/decrement is prone to race conditions if stdout has been assigned an ordinary file. The following snippet is thus likely to close stdout too early, resulting - for example - in segmentation faults inside Glibc:

	stdout = File("/dev/null", "w");
	foreach(t; 1000.iota.parallel)
		writeln("Oops");

When Phobos is compiled with assertions, the bug is generally caught within the File struct itself.

The compiler did warn that accessing the global data `stdout` would be unsafe (because of potential race conditions). A wrapper `trustedStdout` was written to make stdout usable in @safe code, but it bears no warning as to threading issues.

Compare to: https://issues.dlang.org/show_bug.cgi?id=15645 where @trusted was added to silence legitimate compiler warnings about safety, resulting in a Phobos bug.

Ultimately I believe that stdout must be a shared resource with a shared postblit and dtor that decrements the ref count in an atomic way or stdout must not be reassignable at all. See also: The situation with thread-safety of std.logger's global stdlog.
Comment 1 github-bugzilla 2016-06-04 15:04:29 UTC
Commits pushed to stable at https://github.com/dlang/phobos

https://github.com/dlang/phobos/commit/92eed4f45ed01a5d1f975fb23859ea63473e6f5e
Warn about Issue 15768

https://github.com/dlang/phobos/commit/02d1aedf6d3d001989ee342eae6d9fc6db61d399
Merge pull request #4401 from JackStouffer/patch-9

Warn about Issue 15768
Comment 2 Jack Stouffer 2016-06-09 21:14:21 UTC
In order for stdout, stdin, and stderr to be shared(File), I think File needs huge rewrites that will end up being backwards incompatible. Either that, or you need to write a second implementation of File that supports shared and has a lot less functionality.
Comment 3 anonymous4 2016-06-10 15:13:32 UTC
I'd say follow TLS-cached singleton pattern, FILE* can be shared, but the reference counter must be separate.
Comment 4 Walter Bright 2016-06-25 05:31:37 UTC
*** Issue 13443 has been marked as a duplicate of this issue. ***
Comment 6 anonymous4 2016-07-21 12:59:50 UTC
(In reply to Jack Stouffer from comment #2)
> In order for stdout, stdin, and stderr to be shared(File)

1. shared(File) stdio will break code that consumes it as unshared
2. always doing interlocked counting for shared(File) is still slow, consider this optimization: https://dpaste.dzfl.pl/d8dab5b7aa1f
Comment 7 Jack Stouffer 2016-07-28 14:22:40 UTC
(In reply to Sobirari Muhomori from comment #6)
> (In reply to Jack Stouffer from comment #2)
> > In order for stdout, stdin, and stderr to be shared(File)
> 
> 1. shared(File) stdio will break code that consumes it as unshared

Yup, and I don't see a way around that. Apparently when std.stdio was designed, shared still had bugs, so it wasn't used and __gshared was chosen instead. That was a huge mistake that we are now paying for. shared is the right choice here; this is specifically what shared was designed for.

OT: IMO anytime __gshared shows up in Phobos outside of C bindings, it's a bug. I don't care if it's using locks, or the code is slightly slower, we need to dog food shared if shared going to get any good.

> 2. always doing interlocked counting for shared(File) is still slow,
> consider this optimization: https://dpaste.dzfl.pl/d8dab5b7aa1f

You can further optimize with a new implementation by removing the ref count from a non shared File instance.
Comment 8 John Colvin 2017-07-12 10:58:16 UTC
Is this resolved now? I see a fair amount of synchronisation in makeGlobal, which is what std{in,out,err} are aliased to now.
Comment 9 Jack Stouffer 2017-07-12 14:04:48 UTC
(In reply to John Colvin from comment #8)
> Is this resolved now? I see a fair amount of synchronisation in makeGlobal,
> which is what std{in,out,err} are aliased to now.

Nope. I was able to reproduce this with Phobos master.
Comment 10 anonymous4 2017-08-16 12:20:53 UTC
https://dpaste.dzfl.pl/183e6dae9867 - a draft of shared reference counter, relies on GC a bit
Comment 11 anonymous4 2018-02-12 16:15:07 UTC
https://github.com/dlang/dmd/pull/7566 some people think you don't even need shared destructor :)
Comment 12 github-bugzilla 2018-04-06 15:11:56 UTC
Commits pushed to master at https://github.com/dlang/phobos

https://github.com/dlang/phobos/commit/9410ed02d3cf402fe991fb1ad1ca2a3d89f8f9b8
Fix Issue 15768 - std.stdio.trustedStdout accesses __gshared data without synchronization

https://github.com/dlang/phobos/commit/77624187be34ce6455b08a6d565082d36fcc5e8a
Merge pull request #6382 from JackStouffer/safe-file

Fix Issue 15768 - std.stdio.trustedStdout accesses __gshared data wit…
merged-on-behalf-of: Jack Stouffer <jack@jackstouffer.com>