Fix Issue 18163 - std.container.array should be usable in @safe by n8sh · Pull Request #6001 · dlang/phobos

n8sh · 2018-01-05T17:59:58Z

I think that standard library Array should be just as @safe as built-in dynamic arrays. Thanks to @wilzbach for opening this issue. This patch requires marking some code @trusted but in each case I believe it is defensible. Some changes were required in RefCounted because it is used in Array.

dlang-bot · 2018-01-05T18:00:00Z

Thanks for your pull request, @n8sh! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Auto-close	Bugzilla	Description
✓	18163	std.container.array should be usable in @safe

JackStouffer · 2018-01-05T18:17:38Z

Marking RefCounted as trusted is very unsafe. See http://forum.dlang.org/post/n0nnu0$1tth$1@digitalmars.com

n8sh · 2018-01-05T18:30:17Z

Thanks for the link @JackStouffer. I'm not making the entirety of RefCounted trusted, just certain things in initialize, move, ~this, and opAssign. I think these changes are sound, but I'll go through them one-by-one.

n8sh · 2018-01-05T18:33:40Z

std/typecons.d

            import std.conv : emplace;

-            _store = cast(Impl*) pureMalloc(Impl.sizeof);
+            _store = (() @trusted => cast(Impl*) pureMalloc(Impl.sizeof))();


@trusted for the cast from void* to Impl*. Could only be unsafe if _store were dereferenced before being initialized, which cannot happen since we next call emplace.

These comments should be put in the source code, s.t. they are easy to catch by future readers ;-)

n8sh · 2018-01-05T18:34:59Z

std/typecons.d

                onOutOfMemoryError();
            static if (hasIndirections!T)
-                pureGcAddRange(&_store._payload, T.sizeof);
+                (() @trusted => pureGcAddRange(&_store._payload, T.sizeof))();


@trusted for the pureGcAddRange. Okay because this cannot cannot conceivably cause memory corruption.

n8sh · 2018-01-05T18:36:02Z

std/typecons.d

            import core.stdc.string : memcpy, memset;

-            _store = cast(Impl*) pureMalloc(Impl.sizeof);
+            _store = (() @trusted => cast(Impl*) pureMalloc(Impl.sizeof))();


@trusted for the cast from void* to Impl*. Could only be unsafe if _store were dereferenced before being initialized, which cannot happen since we next either initialize with memcpy or with a blit.

n8sh · 2018-01-05T18:36:53Z

std/typecons.d

                onOutOfMemoryError();
            static if (hasIndirections!T)
-                pureGcAddRange(&_store._payload, T.sizeof);
+                (() @trusted => pureGcAddRange(&_store._payload, T.sizeof))();


@trusted for the pureGcAddRange. Okay because this cannot cannot conceivably cause memory corruption (pointer is non-null and size is correct).

n8sh · 2018-01-05T18:38:49Z

std/typecons.d

            // Can avoid destructing result.
            static if (hasElaborateAssign!T || !isAssignable!T)
-                memcpy(&_store._payload, &source, T.sizeof);
+                (() @trusted => memcpy(&_store._payload, &source, T.sizeof))();


@trusted for the memcpy. Okay because this cannot cannot cause memory corruption barring unsafe previous operations. By specification it is not safe for any structure to contain a pointer to itself, so the change in address is allowed; _store is non-null; and _store has the correct size.

n8sh · 2018-01-05T18:39:22Z

std/typecons.d

                auto init = typeid(T).initializer();
                if (init.ptr is null) // null ptr means initialize to 0s
-                    memset(&source, 0, sz);
+                    (() @trusted => memset(&source, 0, sz))();


@trusted for memset. Address is valid and size is valid.

n8sh · 2018-01-05T18:40:36Z

std/typecons.d

+                    (() @trusted => memset(&source, 0, sz))();
                else
-                    memcpy(&source, init.ptr, sz);
+                    (() @trusted => memcpy(&source, init.ptr, sz))();


@trusted for memcpy. Destination is non-null, source is valid if there has not already been memory corruption, and size does not overrun.

n8sh · 2018-01-05T18:43:58Z

std/typecons.d

        static if (hasIndirections!T)
        {
-            pureGcRemoveRange(&_refCounted._store._payload);
+            (() @trusted => pureGcRemoveRange(&_refCounted._store._payload))();


@trusted for pureGcRemoveRange. Cannot possibly cause memory corruption unless the payload has pointers to garbage-collected memory which is referred to by pointers elsewhere not in a location known to the garbage collector, and there is no safe way to obtain such pointers.

quickfur · 2018-01-05T18:45:27Z

I would feel slightly less uncomfortable at the number of @trusted escapes introduced, if there was at least one unittest that ensures that instantiating RefCounted with an unsafe payload actually causes the compiler to infer @system for the resulting type / method calls. I.e., something along the lines of:

@system unittest
{
    struct UnsafePayload { ... /* do something unsafe here */ }
    auto x = RefCounted!UnsafePayload(...);
    void canary(T)(T t) @safe
    {
        ... /* call various RefCounted methods here */
    }
    static assert(!__traits(compiles, canary(x));
}

The point being, ensure that the new @trusted additions aren't introducing unintended safety loopholes for payloads that are obviously unsafe.

n8sh · 2018-01-05T18:46:14Z

std/typecons.d

        }

-        pureFree(_refCounted._store);
+        (() @trusted => pureFree(_refCounted._store))();


@trusted for pureFree. The pointer is known not to be null due to isInitialized. Is this a bit that could cause problems if an "alias" to the stored object is captured elsewhere as in http://forum.dlang.org/post/n0nnu0$1tth$1@digitalmars.com?

Yeah, this is where it breaks down. Because you can return a raw reference to the payload, you could potentially have RefCounted free the memory from underneath you.

If the change were made in Array instead, the RefCounted payload is private to Array so a reference to the payload itself couldn't escape, but we're still left with the problem of an escaping reference to one of the items contained in the Array whose space will be freed when the reference count hits zero. Is there a way forward that doesn't rely on DIP-1000?

One approach that comes to mind is making Array's opIndex return by value instead of by reference. To avoid breaking existing code or giving unpleasant surprises we'd have to limit this to types that are reasonably small and don't have a complex destructor and don't have a complex copy constructor. (Of course for those types there would need to be an opIndexAssign.) Then we know that no references to the to-be-freed memory could have escaped without unsafe shenanigans.

If there was a way to say "in safe mode, only return values, not references", that would be ideal. I'm unaware if there is such a thing.

One thought that occurs to me is to use std.traits.isSafe with a suitably-crafted lambda / function in a template sig constraint, and thereby alter the function's refness depending on where it's called from. Unfortunately, since we can't overload on @safe/@system, I'm not sure if this is achievable.

What I am thinking of now is adding a Flag!"returnByValue" parameter to the Array template that defaults to !hasElaborateDestructor!T && !hasElaborateCopyConstructor!T && T.sizeof <= (uint[]).sizeof but can be manually specified if desired.

EDIT: Defaulting to anything but return-by-reference seems to be infeasible because it would break anything that had been relying on hasSwappableElements.

n8sh · 2018-01-05T18:50:13Z

std/typecons.d

 Assignment operators
 */
-    void opAssign(typeof(this) rhs)
+    void opAssign(typeof(this) rhs) @trusted


@trusted for std.algorithm.swap(_refCounted._store, rhs._refCounted._store). I believe this is safe due to the prohibition against structs holding pointers to themselves and due to the lack of any safe way for anyone else to have the address of either _store (meaning there is no need to account for this).

n8sh · 2018-01-05T18:51:17Z

The point being, ensure that the new @trusted additions aren't introducing unintended safety loopholes for payloads that are obviously unsafe.

Okay, will add a test in a few hours.

EDIT: On hold while seeing if fundamental problems can be addressed.

n8sh · 2018-01-05T18:57:15Z

BTW, I acknowledge that there are a lot of @trusted escapes introduced, but I think the standard library is the correct place to bear this pain. Not putting them here where they can be checked once for correctness means that instead every single user of std.container.Array must clutter his own code with similar escapes -- possibly making a mistake -- or eschew safety altogether.

schveiguy

~~I think the trusted escapes are OK~~, but see comment on the swap.

Edit: I forgot about the fact that RefCounted can return a reference to its payload, which means you could have a dangling pointer after the last RefCounted got destroyed. I think this likely isn't viable as a way to make Array safe, you would have to add all the appropriate escapes inside there.

schveiguy · 2018-01-05T18:55:51Z

std/typecons.d

 Assignment operators
 */
-    void opAssign(typeof(this) rhs)
+    void opAssign(typeof(this) rhs) @trusted


This should not need marking, you sure it needs it? swap should be safe for swapping 2 pointers (which is what I think this is doing).

Oddly it does. Just verified.

Weird. I tested locally, and swap with 2 pointers works in @safe. I would have expected this to infer that.

In any case, this should be marked @safe, not @trusted I would think.

schveiguy · 2018-01-05T19:28:20Z

ensure that the new @trusted additions aren't introducing unintended safety loopholes for payloads that are obviously unsafe

And, even if they aren't (and I don't think they are), make sure future improvements don't!

wilzbach · 2018-01-05T20:15:26Z

@n8sh thanks a lot for working on this. Just an FYI @edi33416 is working on a new containers library for Phobos (though it still might take some time ...).

Marking RefCounted as trusted is very unsafe. See

Uh oh!

Conversation

n8sh commented Jan 5, 2018

Uh oh!

dlang-bot commented Jan 5, 2018

Bugzilla references

Uh oh!

JackStouffer commented Jan 5, 2018

Uh oh!

n8sh commented Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quickfur commented Jan 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n8sh Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n8sh commented Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

n8sh commented Jan 5, 2018

Uh oh!

schveiguy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schveiguy Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schveiguy commented Jan 5, 2018

Uh oh!

wilzbach commented Jan 5, 2018

Uh oh!

n8sh commented Jan 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

n8sh commented Jan 5, 2018 •

edited

Loading

n8sh Jan 5, 2018 •

edited

Loading

n8sh commented Jan 5, 2018 •

edited

Loading

schveiguy left a comment •

edited

Loading

schveiguy Jan 5, 2018 •

edited

Loading