fix Issue 14439 - aa's keys, values, byKey, byValue not usable in @sa…

[WalterBright]

…fe context

The mechanics of the aa's are trustable. This leaves the effect of whether postblit is safe or not intact.

[dlang-bot]

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close	Bugzilla	Description
✓	14439	aa's keys, values, byKey, byValue not usable in @safe context

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

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

[schuetzm]

Seeing @trusted followed by : makes me nervous... IMO it's better to add it to each function signature individually.

[JackStouffer]

I agree; besides the safety issue, I have run into over applied attribute issues in other PRs.

[WalterBright]

Given that the functions it applies to are all right there and one liners, this isn't an issue, and implementing it would add clutter.

[aG0aep6G]

To be fair, this is also applies to std.range.front for normal arrays when compiling in release mode without bounds checking, and that's marked as safe.

But that's -boundscheck=off's fault. It flat-out breaks @safe. An @safe/@trusted function may be unsafe with the switch, but it must be safe without it.

[dnadlinger]

Just going to leave this here:

struct Foo {
  int[int] aa;
  auto opCast() pure nothrow @nogc { *cast(uint*)0xdeadbeef = 0xcafebabe; return null; }
  alias aa this;
}

int main() @safe {
    Foo f;
    return !f.byKey.empty;
}

I hope it is slowly getting obvious that the @trusted guidelines I must have been going on about for several years by now aren't just my personal preference, but actually help avoid common pitfalls. I've certainly been able to find safety issues in dlang PRs fairly consistently – including several of yours – by just mechanically applying them and having a closer look at code that violates them.

[andralex]

@klickverbot is that code sample submitted as a bug? Seems to me opCast is not checked, and it should.

[WalterBright]

I've certainly been able to find safety issues in dlang PRs fairly consistently – including several of yours – by just mechanically applying them and having a closer look at code that violates them.

That's great. Keep 'em coming! Fixed.

[codecov-io]

Current coverage is 74.12% (diff: 89.65%)

Merging #1644 into master will increase coverage by 0.12%

@@             master      #1644   diff @@
==========================================
  Files           133        133          
  Lines         17037      17134    +97   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          12607      12701    +94   
- Misses         4430       4433     +3   
  Partials          0          0          

Powered by Codecov. Last update 0d0ee2c...992f0b7

[dnadlinger]

@andralex: Your question actually raises an interesting point – the code was invalid for alias this types with an opCast compatible with void* before. This is somewhat tangential to what I'm pointing out here, though – it was just the easiest way for me to point out that @trusting T can lead to executing arbitrarily unsafe code; there would be a memory safety issue anyway.

@WalterBright:

Fixed.

Arbitrarily disallowing AliasThis breaks perfectly valid code, e.g. this:

struct Container {
    int[int] aa;
    alias aa this;
}

void main() {
    Container c;
    // …
    foreach (a; c.byKey) {
        // …
    }
}

The trick is not to disallow AliasThis'ed types altogether, but to force conversion outside the scope of @trusted.

[dnadlinger]

[…] force conversion outside the scope of @trusted.

To elaborate on this a bit, you could for example make the signatures look more like this:

auto byValue(K, V)(const(V[K]) aa) { … }

Alternatively, you can also assign aa to a V[K] type inside the implementation, and remove the function-level @trusted altogether – which has no business being on a generic template anyway.

---

Speaking of @trusted annotations, why do you insist of keeping the incorrect ones on the extern(C) function declarations? Granted, the issue is no longer user-visible after you made them private, but we now still need to audit all of object.d for incorrect uses of the functions. If you just used @trusted the way it is supposed to be – as a local escape for non-checkable code –, we wouldn't have this issue in the first place, at no additional cost.

[WalterBright]

disallowing AliasThis breaks perfectly valid code

I understand, but the trouble is, alias this can be used to override various behaviors (not just opCast) that the implementation relies on - i.e. the cast to void* that is a type known to the implementation could get corrupted. And even if the user supplied opCast was safe, that doesn't mean it left the AA in a state recognizable by the implementation.

The correct fix to this is to create a fully templated implementation of the AAs. We've aspired to do this, but have repeatedly failed. Anyhow, doing that is way beyond the scope of this PR. As long as the implementation has a non-templated void* interface, the only pragmatic way forward for now is to disallow alias this usage.

make the signatures look more like this

Doesn't work because then the values will all be const.

you can also assign aa to a V[K] type inside the implementation

That was my first try. Unfortunately, that does not work (again, const issues, the test suite failed miserably).

we now still need to audit all of object.d for incorrect uses of the functions.

I'm not terribly concerned about that. There are just a couple uses, and there aren't unsafe template arguments mucking it up. I don't even like it being in object.d at all, it should be encapsulated elsewhere.

If you just used @trusted the way it is supposed to be – as a local escape for non-checkable code –, we wouldn't have this issue in the first place, at no additional cost.

It wouldn't have solved the alias this problem, and it would have looked awful because it is pretty much every line of code. The buildin AAs do not follow the usual typing rules in D, this is a known issue, and can only be solved by redoing the whole thing with templates. Until then I think we're stuck with this admittedly hackish approach - but at least it solves the particular bug report problem.

[dnadlinger]

The correct fix to this is to create a fully templated implementation of the AAs.

This is only tangentially related. Of course leaving everything up to inference makes things easier, but that doesn't change the correctness a fix to the current implementation. The hybrid system we have is messy, but can still be equally type-safe.

As long as the implementation has a non-templated void* interface, the only pragmatic way forward for now is to disallow alias this usage.

I suspect you make this statement just because you don't have a good handle on how AliasThis interacts with @safety and with how inout merging works with templates and AAs. Then again, nobody does, so that's an understandable mistake to make.

How about this as a starting point?

diff --git a/src/object.d b/src/object.d
index a956e18..7b49c04 100644
--- a/src/object.d
+++ b/src/object.d
@@ -1863,7 +1863,13 @@ extern (C)
     // alias _dg2_t = extern(D) int delegate(void*, void*);
     // int _aaApply2(void* aa, size_t keysize, _dg2_t dg);

-    private struct AARange { void* impl; size_t idx; }
+    private struct AARange
+    {
+    private:
+        this(void* impl, size_t idx) { this.impl = impl; this.idx = idx; }
+        void* impl;
+        size_t idx;
+    }
     AARange _aaRange(void* aa) pure nothrow @nogc;
     bool _aaRangeEmpty(AARange r) pure nothrow @nogc;
     void* _aaRangeFrontKey(AARange r) pure nothrow @nogc;
@@ -1964,7 +1970,7 @@ V[K] dup(T : V[K], K, V)(T* aa)
     return (*aa).dup;
 }

-auto byKey(T : V[K], K, V)(T aa) pure nothrow @nogc
+auto byKey(K, V)(const(V[K]) aa) pure nothrow @nogc
 {
     import core.internal.traits : substInout;

@@ -1972,22 +1978,22 @@ auto byKey(T : V[K], K, V)(T aa) pure nothrow @nogc
     {
         AARange r;

-    pure nothrow @nogc:
+    pure nothrow @nogc @trusted:
         @property bool empty() { return _aaRangeEmpty(r); }
         @property ref front() { return *cast(substInout!K*)_aaRangeFrontKey(r); }
         void popFront() { _aaRangePopFront(r); }
         @property Result save() { return this; }
     }

-    return Result(_aaRange(cast(void*)aa));
+    return Result(((typeof(aa) aa) @trusted => _aaRange(cast(void*)aa))(aa));
 }

-auto byKey(T : V[K], K, V)(T* aa) pure nothrow @nogc
+auto byKey(T : V[K], K, V)(T* aa)
 {
     return (*aa).byKey();
 }

-auto byValue(T : V[K], K, V)(T aa) pure nothrow @nogc
+auto byValue(K, V)(const(V[K]) aa) pure nothrow @nogc
 {
     import core.internal.traits : substInout;

@@ -1995,22 +2001,22 @@ auto byValue(T : V[K], K, V)(T aa) pure nothrow @nogc
     {
         AARange r;

-    pure nothrow @nogc:
+    pure nothrow @nogc @trusted:
         @property bool empty() { return _aaRangeEmpty(r); }
-        @property ref front() { return *cast(substInout!V*)_aaRangeFrontValue(r); }
+        @property ref front() { return *cast(const(V)*)_aaRangeFrontValue(r); }
         void popFront() { _aaRangePopFront(r); }
         @property Result save() { return this; }
     }

-    return Result(_aaRange(cast(void*)aa));
+    return Result(((typeof(aa) aa) @trusted => _aaRange(cast(void*)aa))(aa));
 }

-auto byValue(T : V[K], K, V)(T* aa) pure nothrow @nogc
+auto byValue(T : V[K], K, V)(T* aa)
 {
     return (*aa).byValue();
 }

-auto byKeyValue(T : V[K], K, V)(T aa) pure nothrow @nogc
+auto byKeyValue(K, V)(const(V[K]) aa) pure nothrow @nogc
 {
     import core.internal.traits : substInout;

@@ -2018,7 +2024,7 @@ auto byKeyValue(T : V[K], K, V)(T aa) pure nothrow @nogc
     {
         AARange r;

-    pure nothrow @nogc:
+    pure nothrow @nogc @trusted:
         @property bool empty() { return _aaRangeEmpty(r); }
         @property auto front() @trusted
         {
@@ -2039,7 +2045,7 @@ auto byKeyValue(T : V[K], K, V)(T aa) pure nothrow @nogc
         @property Result save() { return this; }
     }

-    return Result(_aaRange(cast(void*)aa));
+    return Result(((typeof(aa) aa) @trusted => _aaRange(cast(void*)aa))(aa));
 }

 auto byKeyValue(T : V[K], K, V)(T* aa) pure nothrow @nogc
@@ -2047,10 +2053,12 @@ auto byKeyValue(T : V[K], K, V)(T* aa) pure nothrow @nogc
     return (*aa).byKeyValue();
 }

-Key[] keys(T : Value[Key], Value, Key)(T aa) @property
+Key[] keys(Value, Key)(Value[Key] aa) @property
 {
-    auto a = cast(void[])_aaKeys(cast(inout(void)*)aa, Key.sizeof, typeid(Key[]));
-    auto res = *cast(Key[]*)&a;
+    auto res = () @trusted {
+        auto voidRes = _aaKeys(cast(void*)aa, Key.sizeof, typeid(Key[]));
+        return *cast(Key[]*)&voidRes;
+    }();
     _doPostblit(res);
     return res;
 }
@@ -2060,10 +2068,12 @@ Key[] keys(T : Value[Key], Value, Key)(T *aa) @property
     return (*aa).keys;
 }

-Value[] values(T : Value[Key], Value, Key)(T aa) @property
+Value[] values(Value, Key)(Value[Key] aa) @property
 {
-    auto a = cast(void[])_aaValues(cast(inout(void)*)aa, Key.sizeof, Value.sizeof, typeid(Value[]));
-    auto res = *cast(Value[]*)&a;
+    auto res = () @trusted {
+        auto voidRes = _aaValues(cast(void*)aa, Key.sizeof, Value.sizeof, typeid(Key[]));
+        return *cast(Value[]*)&voidRes;
+    }();
     _doPostblit(res);
     return res;
 }
@@ -2100,6 +2110,20 @@ unittest
     assert(T.count == 2);
 }

+@safe pure unittest
+{
+    string[string] saa = ["a" : "1", "b" : "2"];
+    string s = saa["a"];
+    saa["c"] = "3";
+    if ("c" in saa) {}
+    size_t l = saa.length;
+    foreach(k; saa.keys) {}
+    foreach(k; saa.byKey) {}
+    foreach(v; saa.values) {}
+    foreach(v; saa.byValue) {}
+    foreach(k, v; saa) {}
+}
+
 inout(V) get(K, V)(inout(V[K]) aa, K key, lazy inout(V) defaultValue)
 {
     auto p = key in aa;

It still passes the test suite, but I didn't spend much time thinking about the details, so please only take it as an idea for further discussion. The patch notably also doesn't address @trustedness of the returned ranges yet. This is because …

---

… your current PR, overly restrictive as it is, still has a safety issue:

int main() @safe {
  alias Result = typeof(["a" : 3].byValue());
  alias AARange = typeof(Result.init.tupleof[0]);
  return Result(AARange(new int, 3)).front;
}

This is the first time I encounter this particular kind of issue myself, so I suspect there might be similar bugs hidden throughout the some of the various Phobos ranges.

[WalterBright]

How about this as a starting point?

Won't work. The trouble is that once data is cast to void*, ALL type information is lost, including any alias this information. Making it a trusted cast won't help. The purpose of alias this is to override certain behaviors, and even the layout, of the underlying type. This will never work safely with an implementation that assumes exact layout and behavior coming from a void*.

[WalterBright]

still has a safety issue:

Interesting. Will work on it.

[WalterBright]

Hole fixed. Next? :-)

[dnadlinger]

Won't work. The trouble is that once data is cast to void*, ALL type information is lost, including any alias this information.

Note that in my example the parameter is typed as an AA, so what you are pointing out does not apply. The AliasThis is evaluated at the function call site, which also side-steps @safety issues.

[dnadlinger]

Next? :-)

-release is not supposed to disable bounds-checking in @safe code, so you might want to use version (D_NoBoundsChecks) instead of asserts. It's probably a bit of a murky area, though.

[andralex]

@klickverbot will leave approval of this to you following consensus with @WalterBright. Let me add we're very appreciative of your work!

[WalterBright]

Note that in my example the parameter is typed as an AA, so what you are pointing out does not apply. The AliasThis is evaluated at the function call site, which also side-steps @safety issues.

It does apply, otherwise the cast override in your first example would not have happened. Secondly, even if did not apply, it still is a hole. Let me reiterate that the purpose of wrapping is to override behaviors. Overriding behaviors means the void* interface can no longer rely on having had complete control over the data structure. For example, a field could be added before the aa, and then boom, the supposedly trustworthy void* cast points to that field rather than the aa.

-release is not supposed to disable bounds-checking in @safe code, so you might want to use version (D_NoBoundsChecks) instead of asserts. It's probably a bit of a murky area, though.

The bounds checking is for arrays, not associative arrays. Even so, to add this requirement implies reconsidering the pervasive use of asserts in ranges, starting with:

https://github.com/dlang/phobos/blob/master/std/range/primitives.d#L2267

which is a whole 'nother discussion and way outside the scope of this PR.

[dnadlinger]

For example, a field could be added before the aa, and then boom, the supposedly trustworthy void* cast points to that field rather than the aa.

That's… not how AliasThis works. I'm not fond of repeating myself, but:

Note that in my example the parameter is typed as an AA, so [t]he AliasThis is evaluated at the function call site

T : V[K] matches T as the original struct type. In contrast, typing the parameter as V[K] evaluates the AliasThis at the call site to pass the AA value to the function.

---

I also updated the patch above. The const qualifiers still need a closer look, but it passes the test suite and should serve as a good starting point towards a comprehensive solution.

[WalterBright]

At this point, it would seem more efficient if you wrote and submitted another PR rather than trying to fold in all those diffs, as you've essentially rewritten it completely anyway and have it ready to go. You deserved the credit for it.

[dnadlinger]

Oh, I'm not particularly interested in working on this issue myself at all. I just happened to come across this PR, which was first introducing a @safety hole and currently breaks backwards compatibility. Both these issues seem to stem from a slightly inaccurate mental model of alias this and template argument deduction, so I threw together a quick sketch to hopefully convey why I consider the resulting pessimistic outlook ("the only pragmatic way forward for now is to disallow alias this usage") to be unfounded.

And many thanks for your concerns, but I'm not worried about credit or appreciation. Knowing that I've had a hand in making sure D doesn't become less than it could be by preventing a few absent-minded slip-ups here and there (or helping to clarify some concepts, cf. purity and safety in general) is certainly enough for me to be content with. If you want to give me credit, do it by starting from the assumption that I have a sound point to make when I go through the effort to entertain a discussion. There will of course be cases where I'm wrong in a plain and simple way, but be assured I am trying to avoid having such arguments. They are an idle waste of time much better spent elsewhere.

If you don't mind me going on a bit of a tangent: A while ago, Andrei said something (it might have been in private conversation – in which case, apologies!) to the effect of D having a problem with people just wanting to get a kick out of "winning" discussions and proving the two of you wrong over and over again. As far as I am concerned, the converse applies: If anything, I'd really rather not "win" any discussion in which I don't walk away with a deeper understanding of the subject in hand as well. Unfortunately, reaching for the quick fix, the same pragmatism that can be incredibly useful in getting things done, seems to have a habit of also preventing one from seeing the structure of the problem at hand, even if others might have already recognised it. I would argue that a large portion of programming language design is fundamentally about distilling such patterns into a form that makes them readily available for others to draw upon. In this light, the frequency with which essentially the same discussions have come up again and again recently – such as in the case of @trusted – certainly makes me wonder where the future of D will lie. I've become more cautious about investing effort into the ecosystem as a result, and I know that I'm not alone in this; neither in my attitude towards discussions, nor in worrying about this seeming lack of perspective.

---

As an aside,

https://github.com/dlang/phobos/blob/master/std/range/primitives.d#L2267

you could have hardly picked a worse example to try making your point about @safety and -release. Implicit array bounds checks will be inserted as usual for a[0].

[WalterBright]

If you don't want to make a PR with your file, how about emailing it to me? That would be a lot easier than trying to go line by line with the above diff, and far less error prone.

[dnadlinger]

Oh, the above is a standard unified diff, which you can just paste into patch, git apply or whatever you favourite method of applying patches is. I've attached the full file for your convenience.

object.d.zip

[MartinNowak]

Thanks for the review @klickverbot.
The PR smells a bit, but it would also be nice if we can make those methods safe before the long library AA transition.

The correct fix to this is to create a fully templated implementation of the AAs. We've aspired to do this, but have repeatedly failed.

Just to correct your perception, we had failed twice, a promising third attempt was mostly stuck by lack of feedback, see #1282. Though you didn't seem to fully understand the topic, I'm taking your affirmation #1282 (comment) as a reason to put some more resources into this.
Hope I can get more useful information out of this thread http://forum.dlang.org/post/anhtpsftcedurgskfsgx@forum.dlang.org, otherwise would have to go through a DIP.

[dnadlinger]

The PR smells a bit, but it would also be nice if we can make those methods safe before the long library AA transition.

Agreed! The PR previously introduced a safety hole, so it was unacceptable back then. It still breaks valueWithAliasThisToAA.keys and so on, which should be fixed before it goes in. I posted a 90% solution as a starting point, but the const qualifiers need a closer look before that can go in.

(I still have nightmares from trying to track down how inout and AAs interact last time, where I turned out to hit a type merging bug, so I'd much rather if somebody who is fluent in inout verifies the change. We probably also want to add some more test coverage for qualifiers.)

[WalterBright]

@klickverbot I have posted your version.

[WalterBright]

Fails with:

runnable/interpret.d(1975): Error: template object.keys cannot deduce function from argument types !()(immutable(bool[int])), candidates are:
../../druntime/import/object.d(2058):        object.keys(Value, Key)(Value[Key] aa)
../../druntime/import/object.d(2068):        object.keys(T : Value[Key], Value, Key)(T* aa)
runnable/interpret.d(1979):        called from here: foo101()

[WalterBright]

I changed it back to the version I originally wrote because it passes the test suite. It does have the issue that it cannot be wrapped, but I don't see a way around it at the moment. @klickverbot 's version could be wrapped, but didn't pass the tests, and the reason for that is the same reason the templates are written a little oddly in the original.

[somzzz]

@WalterBright @andralex is this PR still required?
byKey, byValue and byKeyValue were made @safe in #1944

[RazvanN7]

byKey and byValue are @safe now and keys/values are being made @safe in this PR: #3528 . So I'm closing this, but feel free to give feedback to #3528