fix issue 17108 Associative array byKeyValue is unsafe by somzzz · Pull Request #1944 · dlang/druntime

somzzz · 2017-10-17T15:55:24Z

No description provided.

dlang-bot · 2017-10-17T16:01:31Z

Thanks for your pull request, @somzzz! 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
✓	17108	Associative array byKeyValue is unsafe

MetaLang · 2017-10-17T16:48:34Z

src/object.d

 {
    import core.internal.traits : substInout;

+    static auto safeAARangeEmpty(AARange rr) @safe { return () @trusted { return _aaRangeEmpty(rr); } (); }


I'm not sure it's necessary to have these functions as they're only used once. I think an @trusted block should suffice.

MetaLang · 2017-10-17T16:52:32Z

src/object.d

    }

-    return Result(_aaRange(cast(void*)aa));
+    return () @trusted {return Result(_aaRange(cast(void*)aa)); } ();


Space between { and return.

wilzbach · 2017-10-17T21:51:54Z

src/object.d

    return (*aa).get(key, defaultValue);
 }

+unittest


Why don't you simply make the unittest @safe?
__compiles is an anti-pattern and should only be used if absolutely necessary.

wilzbach · 2017-10-17T21:54:13Z

src/object.d

    }

-    return Result(_aaRange(cast(void*)aa));
+    return () @trusted { return Result(_aaRange(cast(void*)aa)); } ();


The constructor of Result should be safe, so you could try to move the lamda inside.

wilzbach · 2017-10-17T21:56:50Z

src/object.d


    pure nothrow @nogc:
-        @property bool empty() { return _aaRangeEmpty(r); }
+        @property bool empty() @safe { return () @trusted { return _aaRangeEmpty(r); } (); }


For DMD there is an overhead in these lamdas and in this case there is no need for the lamda if you replace @safe with @trusted

I thought Kenji fixed the compiler so when the compiler detects this pattern it always inlines the lambda.

This should simply be marked @safe, and the implementation of _aaRangeEmpty marked @safe. We should avoid @trusted wherever we can.

schveiguy

I think you can also instrument byKey and byValue while you are at it, as all of them do pretty much the same thing as byKeyValue.

See also: https://issues.dlang.org/show_bug.cgi?id=17507

schveiguy · 2017-10-18T00:12:14Z

src/object.d

+                @property auto value() inout
+                {
+                    return () @trusted { return *cast(substInout!V*)valp; } ();
+                };


There is a problem here in that the AA can rehash the buckets, making any existing range point at invalid elements. I think in order to make this @safe we need to verify the range is still pointing at a valid element.

In particular the fetching of the value returns the pointer + key.sizeof, which even if the element pointer is null is going to be sketchy.

I still prefer to see this not being a @trusted escape, but instead mark the implementation as @safe and have the implementation use an escape.

We also have to be careful here of any kind of postblit in K or V. If you trust the whole thing, then it is going to inadvertently trust that part too.

@schveiguy the "abandoned" elements will be still valid memory. All we need to make sure is they are cleared properly (by means of move in all likelihood). That would make old ranges iterate the wrong elements but not unsafe.

The abandoned elements themselves are valid, and are set to null.

However, the code for fetching the value looks like this (from here: https://github.com/somzzz/druntime/blob/84af68f87d077d9632e7dc58da04670316d4219c/src/rt/aaA.d#L705):

void* _aaRangeFrontValue(Range r) { return r.buckets[r.idx].entry + r.valoff; }

Here, r.buckets is really r.impl.buckets (impl points at the actual AA instance, so we are OK there). The r.idx is the range's local copy of which bucket it is currently looking at.

entry is possibly null (shouldn't be on a valid AA range, but if a rehash happens, it could be). So we are adding an arbitrary amount to a null pointer, and dereferencing that without considering it (unsafe).

Another consideration that I had figured out in the past but forgot to mention it here, it's very possible r.idx >= r.impl.buckets.length if the rehash has shrunk the array. Now we are definitely in unsafe territory.

All this can be fixed by returning null if r.idx >= r.buckets.length or the appropriate bucket pointer is null (just don't add the offset). And these aren't bounds-checked, since druntime is compiled in release mode.

Looking at this more closely, it's really the generation of the Pair struct that is where the safety issues may lie.

schveiguy · 2017-10-18T00:18:05Z

src/object.d

+            }
+        }
+    ));
+}


I'd like to also make sure that the following doesn't compile:

struct BadValue { int x; this(this) { *cast(ubyte*)(null + 100000) = 5; } // not @safe alias x this; } BadValue[int] aa; auto x = aa.byKeyValue.front; () @safe {auto y = x.value; }

andralex · 2017-10-23T15:13:05Z

src/object.d

-                @property ref value() inout { return *cast(substInout!V*)valp; }
+                @property auto key() inout
+                {
+                    return () @trusted { return *cast(substInout!K*)keyp; } ();


auto p = @trusted { return cast(substInout!K *) keyp; }(); return *p;

andralex · 2017-10-23T15:17:02Z

src/object.d

+                };
+                @property auto value() inout
+                {
+                    return () @trusted { return *cast(substInout!V*)valp; } ();


edi33416 · 2017-10-25T07:57:08Z

Sorry to be nitpicking.

I think that return (() @trusted => *cast(substInout!K *) keyp)(); is more pleasant to the eye than return () @trusted { return *cast(substInout!K*)keyp; } ();
The style checker should complain about wanting a space between the cast(T) and the value
ex. cast(substInout!K*)keyp should be cast(substInout!K*) keyp; at least the phobos style checker does.

Aside from the nits, LGTM.

wilzbach · 2017-10-28T07:15:06Z

The style checker should complain about wanting a space

FYI: atm there is no style checking in place at druntime and DMD :/
(Also Walter prefers different styles for DMD than the ones that are used for Phobos).

somzzz · 2017-11-02T07:30:39Z

@schveiguy @andralex ping

rainers · 2017-11-02T08:25:53Z

src/rt/aaA.d


    void* _aaRangeFrontKey(Range r)
    {
+        if (r.idx >= r.dim)


Marking the function @safe enables bounds checking on the buckets-access, so I think the test here is unnecessary. A range error is probably better than an access violation later.

Interestingly, x64 code elides the bounds check if this condition exists, but keeps both for 32-bit code.

rainers · 2017-11-02T08:36:57Z

src/object.d

-        @property ref front() { return *cast(substInout!K*)_aaRangeFrontKey(r); }
-        void popFront() { _aaRangePopFront(r); }
+        @property bool empty()  @safe { return _aaRangeEmpty(r); }
+        @property auto front()


This changes the return value from a reference to a copy. Is this expected? Can it break code that has been using ref for iteration?
While a mutable reference is not a good idea to begin with, using a const reference might be a bit more conservative.

An intermediate version of this PR didn't allow ref as return type so at that point I changed it to auto.
But yes, it's better to preserve the original. Thanks for pointing it out!

andralex · 2017-11-06T14:15:03Z

@schveiguy I'll merge this since your concern has been addressed, holler if there's anything left

schveiguy · 2017-11-06T14:29:04Z

Thanks, @andralex, I didn't notice the updates. They all look good! Thanks @somzzz

wilzbach · 2017-12-13T08:54:06Z

Ehm just copy/pasting @klickverbot's examples of what shouldn't be allowed from #1644

int main() @safe {
  auto a = ["foo": 1];
  auto r = a.byValue;
  r.popFront;
  return r.front; // oops
}

and another one

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;
}

While 2.077.1 correctly emits warnings that this isn't @safe, this code now segfaults.
-> https://issues.dlang.org/show_bug.cgi?id=18071

schveiguy · 2017-12-13T14:11:16Z

The first one, I think is expected to segfault, as you are dereferencing a null pointer (which is allowed in @safe code). This is equivalent to:

void foo() @safe
{
   int *x = null;
   int y = *x; // oops, but it's ok.
}

The second, I think can be fixed by assigning to a local AA before casting. I'll look at making a PR.

Again IFTI is confusing here, as we really are trying to add a member to the AA, but instead end up adding members to items that aren't really AAs.

dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label Oct 17, 2017

somzzz force-pushed the make_foreach_byKeyValue_safe branch 2 times, most recently from f550561 to 00b147a Compare October 17, 2017 16:09

MetaLang suggested changes Oct 17, 2017

View reviewed changes

somzzz force-pushed the make_foreach_byKeyValue_safe branch from 00b147a to 84af68f Compare October 17, 2017 17:53

wilzbach reviewed Oct 17, 2017

View reviewed changes

schveiguy suggested changes Oct 18, 2017

View reviewed changes

andralex reviewed Oct 23, 2017

View reviewed changes

src/object.d Outdated

};

@property auto value() inout

{

return () @trusted { return *cast(substInout!V*)valp; } ();

Copy link

Member

andralex Oct 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar

somzzz force-pushed the make_foreach_byKeyValue_safe branch from 84af68f to 7afc355 Compare October 23, 2017 17:03

somzzz force-pushed the make_foreach_byKeyValue_safe branch from 7afc355 to 99e701c Compare October 27, 2017 14:12

rainers reviewed Nov 2, 2017

View reviewed changes

fix issue 17108 Associative array byKeyValue is unsafe

51003a3

somzzz force-pushed the make_foreach_byKeyValue_safe branch from 99e701c to 51003a3 Compare November 2, 2017 10:30

andralex merged commit 3bf2559 into dlang:master Nov 6, 2017

somzzz mentioned this pull request Nov 9, 2017

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

Closed

Uh oh!

Comments

Conversation

somzzz commented Oct 17, 2017

Uh oh!

dlang-bot commented Oct 17, 2017

Bugzilla references

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!

schveiguy left a comment

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!

edi33416 commented Oct 25, 2017

Uh oh!

wilzbach commented Oct 28, 2017

Uh oh!

somzzz commented Nov 2, 2017

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!

andralex commented Nov 6, 2017

Uh oh!

schveiguy commented Nov 6, 2017

Uh oh!

wilzbach commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schveiguy commented Dec 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

wilzbach commented Dec 13, 2017 •

edited

Loading