Fix Issue 15768 - std.stdio.trustedStdout accesses __gshared data wit… #6382

JackStouffer · 2018-03-29T19:24:48Z

Running the following code

import std.algorithm;
import std.parallelism;
import std.range;
import std.stdio;

void main()
{
    stderr = File("/dev/null", "w");
    
    foreach(t; 1_000_000.iota.parallel)
    {
        stderr.write("aaa");
    }
}

dies 100% of the time with v2.079.0, but with this fix it works every time I've tried.

I don't think there can be a test in the test suite for this, so please review carefully.

dlang-bot · 2018-03-29T19:24:50Z

Thanks for your pull request, @JackStouffer!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	15768	critical	std.stdio.File does not support __gshared semantics of stdout/err/in

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

Regression or critical bug fixes should always target the stable branch. Learn more about rebasing to stable or the D release process.

To target stable perform these two steps:

Rebase your branch to upstream/stable:

git rebase --onto upstream/stable upstream/master

Change the base branch of your PR to stable

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + phobos#6382"

n8sh · 2018-03-29T19:29:35Z

    private void initImpl(FILE* handle, string name, uint refs = 1, bool isPopened = false)
    {
        assert(_p);
        _p.handle = handle;
        _p.refs = refs;

Needs atomicStore here.

    private void resetFile(string name, in char[] stdioOpenmode, bool isPopened) @trusted
    {
        import core.stdc.stdlib : malloc;
        import std.exception : enforce;
        import std.conv : text;
        import std.exception : errnoEnforce;

        if (_p is null)
        {
            _p = cast(Impl*) enforce(malloc(Impl.sizeof), "Out of memory");
        }
        else if (_p.refs == 1)

Needs atomicLoad here.

    void detach() @safe
    {
        if (!_p) return;
        if (_p.refs == 1)

Needs atomicLoad here.

n8sh · 2018-03-29T19:45:00Z

std/stdio.d

@@ -843,12 +844,12 @@ Throws: $(D ErrnoException) on failure if closing the file.
    void detach() @safe
    {
        if (!_p) return;
-        if (_p.refs == 1)
+        if (atomicLoad(_p.refs) == 1)


This is a potential resource leak. Example:

Two threads have references. Both check count, and see that it is 2. Both skip this branch and so the file is not closed. What needs to instead happen is unconditional atomic!"-=" and check the result of that.

So in the new example:
Two threads have references. Both atomically decrement. One sees 0 as the new value and the other sees 1 as the new value. Whichever sees 0 then calls close.

n8sh · 2018-03-29T19:51:42Z

std/stdio.d

@@ -513,13 +514,13 @@ Throws: $(D ErrnoException) in case of error.
        {
            _p = cast(Impl*) enforce(malloc(Impl.sizeof), "Out of memory");
        }
-        else if (_p.refs == 1)
+        else if (atomicLoad(_p.refs) == 1)


Same concurrency story here as in detatch. Instead of "close if it's 1, otherwise decrement" you need to "decrement, then close if the result was 0."

n8sh · 2018-03-29T20:10:08Z

std/stdio.d

@@ -843,14 +846,11 @@ Throws: $(D ErrnoException) on failure if closing the file.
    void detach() @safe
    {
        if (!_p) return;
-        if (_p.refs == 1)
+        atomicOp!"-="(_p.refs, 1);
+        if (atomicLoad(_p.refs) == 0)


You can save an atomic operation: if (atomicOp!"-="(_p.refs, 1) == 0)

n8sh · 2018-03-29T20:11:42Z

std/stdio.d

-            _p.refs--;
-            _p = cast(Impl*) enforce(malloc(Impl.sizeof), "Out of memory");
+            atomicOp!"-="(_p.refs, 1);
+            if (atomicLoad(_p.refs) == 0)


You can save an atomic operation: if (atomicOp!"-="(_p.refs, 1) == 0)

andralex · 2018-03-29T20:17:46Z

std/stdio.d

@@ -887,8 +881,7 @@ Throws: $(D ErrnoException) on error.
        if (!_p) return; // succeed vacuously
        scope(exit)
        {
-            assert(_p.refs);
-            if (!--_p.refs)
+            if (atomicLoad(_p.refs) == 0)


This is different from the existing code - it doesn't decrement anymore.

It looks like it's been changed to balance out a change in detatch. The problem is that this is a public function that can be called from places other than detatch.

I don't see how I can fix this without running into the issue you pointed out earlier.

You could refactor the code so detatch doesn't call close (perhaps both of them call a third, private function) or in the branch of detatch where _p.refs is 0 you could change it back to 1 before calling close.

close already calls closeHandles so you could change detatch to this:

void detach() @safe { if (!_p) return; scope(exit) _p = null; if (atomicOp!"-="(_p.refs, 1) == 0) { scope(exit) free(_p); closeHandles(); } }

Then you could change close so it decrements again.

EDIT: put free(_p) in another scope(exit)

jercaianu · 2018-03-30T19:48:16Z

@JackStouffer
For testing, can't we redirect stdout to a file, write something in there from a couple of threads and then restore stdout?

wilzbach · 2018-04-01T07:56:38Z

For testing, can't we redirect stdout to a file, write something in there from a couple of threads and then restore stdout?

Well, the test that Jack posted above allows to perfectly reproduce this bug. It just takes ~0.3s on my system. Maybe we can have a special folder (e.g. stresstests), s.t. these tests aren't run by default for the unittest target?

JackStouffer · 2018-04-02T12:25:59Z

Added test under a new version branch.

wilzbach · 2018-04-02T12:35:55Z

std/stdio.d

+        auto deleteme = testFilename();
+        stderr = File(deleteme, "w");
+
+        foreach(t; 1_000_000.iota.parallel)


std/stdio.d:3439: foreach(t; 1_000_000.iota.parallel)
posix.mak:544: recipe for target 'style_lint' failed

JackStouffer · 2018-04-02T13:03:05Z

Can anyone that uses Windows confirm or deny that 13727 is fixed by this?

wilzbach · 2018-04-02T13:22:46Z

Maybe add the test case from dlang/dmd#5747?

JackStouffer · 2018-04-02T13:34:22Z

I'd need to have the test be run on windows on the auto tester. And I'd assume we don't want to run that for every PR.

wilzbach · 2018-04-02T13:36:50Z

Well, we can definitely run it once here for validation?
(also the test case that Walter wanted to add it in dlang/dmd#5747 only uses 1000 runs.)

anon17 · 2018-04-03T10:37:10Z

13727 has a C test, it doesn't depend on phobos.

anon17 · 2018-04-03T12:09:17Z

Also don't remove comments about unsafe reassignment. You only fixed that reading them is thread-safe after single publishing, but concurrent writing is still subject to race condition.

…hout synchronization

JackStouffer · 2018-04-03T15:47:14Z

How so? Writing is done with LockingTextWriter, which uses OS file locks.

schveiguy · 2018-04-03T15:55:38Z

I think he means writing the handle itself, not writing data to the handle.

But the warning needs to be reworded -- the handle is subject to race conditions after it's initialized, for reading OR writing. It has nothing to do with the issue this PR is fixing.

JackStouffer · 2018-04-03T16:04:46Z

What specific race conditions are you talking about?

schveiguy · 2018-04-03T16:07:58Z

Let me back up a second here -- this PR is fixing an issue, but it doesn't appear to be the issue identified by the bug report's description. It appears that the test case given in the bug report has nothing to do with a race condition on modifying stdout, but improperly implemented reference counting inside File.

My recommendation: split the bug into another report, as there are really 2 bugs here -- one is the File refcount implementation, and the other is the race condition you could have from trustedStdout returning stdout without locking.

I'm not sure we even want to add a mutex lock to stdout. Most of the time, it doesn't change ever, or if it does, you are doing it early on when only one thread exists. Maybe just updating the documentation to reflect the rules is the proper fix.

schveiguy · 2018-04-03T16:12:10Z

What specific race conditions are you talking about?

Classic race condition with shared data. 2 or more threads writing the stdout handle without locking, or 1 thread writing the stdout handle while another thread is calling trustedStdout to read it without locking. In order to properly provide a way to modify the stdout handle, we would need locking.

The other option is to document that you can only change the handles from a single thread, or you get race/undefined behavior.

JackStouffer · 2018-04-03T16:31:58Z

The test case shows the assignment outside of the threaded code because the bug in the bug report is specifically about the ref counting in File not working in a multi-threaded environment, and how that's an issue for stdout/in/err. It could possibly be an issue that a thread could re-assign stdout to another file instance in multi-threaded code, if that's what you're referring to.

In that case, the nature of stdout/in/err being global due to the nature of C libraries should be documented, and the fact that calls to write/ln auto lock the file. I really wouldn't call writing to stdout a "race condition" in the normal usage of the phrase because there are locks around writing, and just like in any well designed multi-threaded program you'll need some queue equivalent to make sure data is written to the file in the correct order. Once a chunk of data starts to be written to the file, that entire chunk will successfully write.

schveiguy · 2018-04-03T16:59:52Z

It could possibly be an issue that a thread could re-assign stdout to another file instance in multi-threaded code, if that's what you're referring to.

Yes.

I really wouldn't call writing to stdout a "race condition" in the normal usage of the phrase because there are locks around writing

Again, I'm not talking about writing data to stdout, but reassigning stdout itself to a different handle (e.g. stdout = File("output.txt");). There are no protections around this.

The original warning was worded poorly anyway -- it said reassigning stdout was dangerous due to bug 15768, but really any use of any of the standard handles (regardless of assignment) was dangerous.

In any case, your new information is good, so I think it's solved.

andralex

Thanks!

schveiguy · 2018-04-05T18:41:03Z

Not sure if issue 15768 should be reworded to reflect what this is actually fixing or this fix should target another bug? See my previous comment.

Edit: Well, I guess we could consider the documentation "fixing" the race condition -- just don't do it, RTFM ;)

JackStouffer · 2018-04-05T18:45:29Z

15768 specifically mentions the ref count being the heart of the issue. The reassignment race conditions are really not part of this.

schveiguy · 2018-04-05T19:16:04Z

The title of the bug report is the problem (it's talking about trustedStdout which simply returns stdout). I'll just change it, so it doesn't confuse people.

anon17 · 2018-04-06T12:46:05Z

$(REF stdin,core,stdout) you probably meant $(REF stdin,core,stdio). Same for stderr.

JackStouffer requested review from andralex and wilzbach March 29, 2018 19:24

JackStouffer requested review from CyberShadow and schveiguy as code owners March 29, 2018 19:24

dlang-bot added the Severity:Bug Fix label Mar 29, 2018

JackStouffer mentioned this pull request Mar 29, 2018

Fix Issue 18024 - checkedint.Warn should be @safe #5928

Closed

JackStouffer force-pushed the safe-file branch from 0f6203a to e09fc69 Compare March 29, 2018 19:32

n8sh reviewed Mar 29, 2018

View reviewed changes

andralex reviewed Mar 29, 2018

View reviewed changes

wilzbach approved these changes Apr 1, 2018

View reviewed changes

wilzbach added this to the 2.080.0 milestone Apr 1, 2018

JackStouffer force-pushed the safe-file branch 2 times, most recently from 7b18d4c to 7d4037e Compare April 2, 2018 12:25

wilzbach reviewed Apr 2, 2018

View reviewed changes

JackStouffer force-pushed the safe-file branch from 7d4037e to 2c2dec3 Compare April 2, 2018 12:42

Fix Issue 15768 - std.stdio.trustedStdout accesses __gshared data wit…

9410ed0

…hout synchronization

JackStouffer force-pushed the safe-file branch from 2c2dec3 to 9410ed0 Compare April 3, 2018 13:01

JackStouffer added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Apr 3, 2018

wilzbach force-pushed the safe-file branch from 5ad6641 to 32736b5 Compare April 5, 2018 05:26

andralex approved these changes Apr 5, 2018

View reviewed changes

Added note about thread safety

db11ea1

JackStouffer force-pushed the safe-file branch from 32736b5 to db11ea1 Compare April 6, 2018 12:50

JackStouffer added Merge:auto-merge and removed Merge:72h no objection -> merge The PR will be merged if there are no objections raised. labels Apr 6, 2018

dlang-bot merged commit 7762418 into dlang:master Apr 6, 2018

JackStouffer deleted the safe-file branch April 6, 2018 15:18

wilzbach mentioned this pull request Apr 9, 2018

Fix Issue 18024 - checkedint.Warn should be @safe (alternative) #6369

Closed

dkgroot mentioned this pull request Apr 29, 2018

Fix stdio File.open / resetFile / detach / refcount #6484

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Issue 15768 - std.stdio.trustedStdout accesses __gshared data wit… #6382

Fix Issue 15768 - std.stdio.trustedStdout accesses __gshared data wit… #6382

JackStouffer commented Mar 29, 2018 •

edited

Loading

dlang-bot commented Mar 29, 2018 •

edited

Loading

n8sh commented Mar 29, 2018

n8sh Mar 29, 2018

n8sh Mar 29, 2018

n8sh Mar 29, 2018

n8sh Mar 29, 2018

andralex Mar 29, 2018

n8sh Mar 29, 2018

JackStouffer Mar 29, 2018

n8sh Mar 29, 2018

n8sh Mar 29, 2018 •

edited

Loading

jercaianu commented Mar 30, 2018

wilzbach commented Apr 1, 2018

JackStouffer commented Apr 2, 2018

wilzbach Apr 2, 2018

JackStouffer commented Apr 2, 2018

wilzbach commented Apr 2, 2018

JackStouffer commented Apr 2, 2018

wilzbach commented Apr 2, 2018

anon17 commented Apr 3, 2018

anon17 commented Apr 3, 2018

JackStouffer commented Apr 3, 2018

schveiguy commented Apr 3, 2018

JackStouffer commented Apr 3, 2018

schveiguy commented Apr 3, 2018

schveiguy commented Apr 3, 2018

JackStouffer commented Apr 3, 2018

schveiguy commented Apr 3, 2018

andralex left a comment

schveiguy commented Apr 5, 2018 •

edited

Loading

JackStouffer commented Apr 5, 2018

schveiguy commented Apr 5, 2018 •

edited

Loading

anon17 commented Apr 6, 2018

Fix Issue 15768 - std.stdio.trustedStdout accesses __gshared data wit… #6382

Fix Issue 15768 - std.stdio.trustedStdout accesses __gshared data wit… #6382

Conversation

JackStouffer commented Mar 29, 2018 • edited Loading

dlang-bot commented Mar 29, 2018 • edited Loading

Bugzilla references

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

Testing this PR locally

n8sh commented Mar 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n8sh Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

jercaianu commented Mar 30, 2018

wilzbach commented Apr 1, 2018

JackStouffer commented Apr 2, 2018

Choose a reason for hiding this comment

JackStouffer commented Apr 2, 2018

wilzbach commented Apr 2, 2018

JackStouffer commented Apr 2, 2018

wilzbach commented Apr 2, 2018

anon17 commented Apr 3, 2018

anon17 commented Apr 3, 2018

JackStouffer commented Apr 3, 2018

schveiguy commented Apr 3, 2018

JackStouffer commented Apr 3, 2018

schveiguy commented Apr 3, 2018

schveiguy commented Apr 3, 2018

JackStouffer commented Apr 3, 2018

schveiguy commented Apr 3, 2018

andralex left a comment

Choose a reason for hiding this comment

schveiguy commented Apr 5, 2018 • edited Loading

JackStouffer commented Apr 5, 2018

schveiguy commented Apr 5, 2018 • edited Loading

anon17 commented Apr 6, 2018

JackStouffer commented Mar 29, 2018 •

edited

Loading

dlang-bot commented Mar 29, 2018 •

edited

Loading

n8sh Mar 29, 2018 •

edited

Loading

schveiguy commented Apr 5, 2018 •

edited

Loading

schveiguy commented Apr 5, 2018 •

edited

Loading