std.stdio.File and std.typecons.Unique not GC-heap safe

[dlangBugzillaToGithub]

michel.fortin reported this on 2010-08-11T12:28:53Z

Transfered from https://issues.dlang.org/show_bug.cgi?id=4624

CC List

• bruno.do.medeiros+deebugz
• jasone
• joseph.wakeling

Description

Just took a look at Phobos for struct destructors. Both std.stdio.File and std.typeconst.Unique seem unsafe to store anywhere in the GC heap (in an array or in a class).

For std.stdio.File, it's because because the destructor assumes the GC-allocated Impl instance can be dereferenced (which is a risky bet during the collection that the GC will deallocate things in the "right" order):

    this(string name, in char[] stdioOpenmode = "rb")
    {
        p = new Impl(errnoEnforce(.fopen(name, stdioOpenmode),
                        "Cannot open file `"~name
                        ~"' in mode `"~stdioOpenmode.idup~"'"),
                1, name);
    }

    ~this()
    {
        if (!p) return;
        // @@@BUG@@@ These lines prematurely close the file
        //printf("Destroying file `%s' with %d refs
", toStringz(p.name), p.refs);
        if (p.refs == 1) close;
        else --p.refs;
    }

In struct std.typecons.Unique(T), unique calls delete on the object it references, but since that object is in the GC heap the same problem arises: it might already have been deallocated:

    ~this()
    {
        writeln("Unique destructor of ", (_p is null)? null: _p);
        delete _p;
        _p = null;
    }

[dlangBugzillaToGithub]

michel.fortin commented on 2010-08-12T09:29:25Z

Additionally, I believe std.containers.Array has a race condition when stored in the GC heap. The Array destructor checks the reference count before deciding whether it should free the array's content or not. This reference counter is non-shared, but the collection cycle can run on any thread so there is a race when accessing the reference counter.

    private struct Data
    {
        uint _refCount = 1;
        size_t _capacity;
        T[] _payload;
        this(T[] p) { _capacity = p.length; _payload = p; }
    }

The reference counter should be made shared so it is only accessed using atomic operations. Other fields don't need to be shared because the only time they're accessed in the destructor is when the reference counter falls to zero and the destructor has the only remaining reference.

This problem is also present in std.stdio.File (in addition to the other problem above) and std.typecons.RefCounted which both use a non-shared reference counter, rendering them unsuitable for being put in the GC heap.

[dlangBugzillaToGithub]

github-bugzilla commented on 2012-10-04T09:51:28Z

Commit pushed to master at https://github.com/D-Programming-Language/phobos

https://github.com/D-Programming-Language/phobos/commit/26b7d3fad37c2cd8e592f4ab4021ba014ff35bd0
Fix std.stdio.File part of Issue 4624

[dlangBugzillaToGithub]

michel.fortin commented on 2012-10-04T11:13:23Z

(In reply to comment #2)
> Commit pushed to master at https://github.com/D-Programming-Language/phobos
> 
> https://github.com/D-Programming-Language/phobos/commit/26b7d3fad37c2cd8e592f4ab4021ba014ff35bd0
> Fix std.stdio.File part of Issue 4624

This only fixes one side of the std.stdio.File part of this issue: there's still a race between the collecting thread and other threads when accessing and decrementing the reference counter in the destructor (see comment #1 on the issue). The reference counter should only be changed using atomic increment/decrement to prevent that race, at least until the GC can guaranty it will collect objects in the same thread they were created in.

[dlangBugzillaToGithub]

joseph.wakeling commented on 2015-07-26T12:18:44Z

Same occurs with Unique:

    static assert(isUniformRNG!(Unique!Random))

... will fail.

[dlangBugzillaToGithub]

joseph.wakeling commented on 2015-07-26T12:19:33Z

(In reply to Joseph Rushton Wakeling from comment #4)
> Same occurs with Unique:
> 
>     static assert(isUniformRNG!(Unique!Random))
> 
> ... will fail.

Ack.  Don't know how this wound up here -- was replying to another bug report. :-(