Bug 15768 – std.stdio.File does not support __gshared semantics of stdout/err/in

Status
RESOLVED
Resolution
FIXED
Severity
critical
Priority
P1
Component
phobos
Product
D
Version
D2
Platform
All
OS
All
Creation time
2016-03-06T01:20:46Z
Last change time
2018-04-06T15:11:57Z
Keywords
safe
Assigned to
No Owner
Creator
Marco Leise
See also
https://issues.dlang.org/show_bug.cgi?id=16535, https://issues.dlang.org/show_bug.cgi?id=6804, https://issues.dlang.org/show_bug.cgi?id=18052, https://issues.dlang.org/show_bug.cgi?id=8295

Comments

Comment #0 by Marco.Leise — 2016-03-06T01:20:46Z
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 by github-bugzilla — 2016-06-04T15:04:29Z
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 by jack — 2016-06-09T21:14:21Z
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 by dfj1esp02 — 2016-06-10T15:13:32Z
I'd say follow TLS-cached singleton pattern, FILE* can be shared, but the reference counter must be separate.
Comment #4 by bugzilla — 2016-06-25T05:31:37Z
*** Issue 13443 has been marked as a duplicate of this issue. ***
Comment #5 by github-bugzilla — 2016-07-20T11:52:05Z
Comment #6 by dfj1esp02 — 2016-07-21T12:59:50Z
(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 by jack — 2016-07-28T14:22:40Z
(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 by john.loughran.colvin — 2017-07-12T10:58:16Z
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 by jack — 2017-07-12T14:04:48Z
(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 by dfj1esp02 — 2017-08-16T12:20:53Z
https://dpaste.dzfl.pl/183e6dae9867 - a draft of shared reference counter, relies on GC a bit
Comment #11 by dfj1esp02 — 2018-02-12T16:15:07Z
https://github.com/dlang/dmd/pull/7566 some people think you don't even need shared destructor :)
Comment #12 by github-bugzilla — 2018-04-06T15:11:56Z
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 <[email protected]>