http://d.puremagic.com/issues/show_bug.cgi?id=4624

           Summary: std.stdio.File and std.typecons.Unique not GC-heap
                    safe
           Product: D
           Version: D2
          Platform: Other
        OS/Version: All
            Status: NEW
          Severity: normal
          Priority: P2
         Component: Phobos
        AssignedTo: nobody@puremagic.com
        ReportedBy: michel.fortin@michelf.com


--- Comment #0 from Michel Fortin <michel.fortin@michelf.com> 2010-08-11 15:28:53 EDT ---
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\n", 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;
    }

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=4624



--- Comment #1 from Michel Fortin <michel.fortin@michelf.com> 2010-08-12 12:29:25 EDT ---
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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=4624



--- Comment #2 from github-bugzilla@puremagic.com 2012-10-04 09:51:28 PDT ---
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

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=4624



--- Comment #3 from Michel Fortin <michel.fortin@michelf.com> 2012-10-04 14:13:23 EDT ---
(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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------