-
-
Notifications
You must be signed in to change notification settings - Fork 727
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Safe ref counted #8368
Safe ref counted #8368
Conversation
Thanks for your pull request and interest in making D better, @dukc! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + phobos#8368" |
956d20f
to
90d4336
Compare
import std.stdio;
import std.typecons;
struct Container
{
ubyte[] data;
}
struct Blob
{
ubyte[64] data;
}
void main ()
{
auto ptr = getPtr();
writeln(ptr);
const save = ptr.dup;
auto other = getPtr();
assert(save == ptr);
}
ubyte[] getPtr ()
{
Blob blob;
RefCounted!Container rc;
escape(rc.refCountedPayload(), blob);
assert(rc.refCountedPayload().data !is null);
return rc.refCountedPayload().data;
}
void escape (ref Container rc, ref Blob local)
{
rc.data = local.data;
} Compiles, runs, crashes. Command used: $ ./dmd/generated/osx/release/64/dmd -preview=dip1000 -run breakdip1000.d Commits used:
|
Thanks! Yeah, as you suspected it's a compiler bug that can be abused without Phobos at all: @safe unittest //this compiles
{
void escape (ref ubyte[] arr, ref ubyte[64] local)
{
arr = local;
}
@safe ubyte[] getPtr ()
{
ubyte[64] blob;
ubyte[] arr;
escape(arr, blob);
return arr; // Ahh, just like bad old C days!
}
} Gotta report, if I don't find it already reported. |
Didn't find an existing issue, so submitted: https://issues.dlang.org/show_bug.cgi?id=22709 I have another possible vulnerability in mind, but It's late around here so I'll test it another day. I hope I won't forget what I'm having in mind. |
I also looked a bit deeper into the internals of |
Issue 22709 has been fixed: dlang/dmd#13577 |
Tried to implement my plan to hack this implementation, along the lines of this: @safe unittest
{
@safe void assignCrap(RefCounted!(int*) countedRef)
{
int local;
auto anotherRef = countedRef; // I was thinking that anotherRef has shorter lifetime than local...
anotherRef.refCountedPayload() = &local; // ...so that this would compile
}
} The compiler does not fall for this, nor for some similar variations I tried. I THINK it is because the That does not mean that this is safe, just that my poor head is running out of cunning with this. Perhaps by using a struct payload and using some member function to play with the lifetimes of the variables it is still hackable. |
This is the problematic case: @safe unittest
{
auto rc = RefCounted!int(123);
(ref int n) {
destroy(rc);
int oops = n; // use-after-free if allowed
}(rc.refCountedPayload);
} As far as I know, in order to ensure this can't happen in |
Good catch! That one compiles, and also a simpler variant of it (in my opinion) compiles: @safe unittest
{
auto rc = RefCounted!int(123);
int* ptr = &rc.refCountedPayload();
destroy(rc);
int oops = *ptr;
} I think the problem is allowing destruction of Worse, I think this applies to about any pattern @safe void abuse(Container)()
{ auto cont = Container([1,2,3]);
scope ptr = &cont.front;
destroy(cont);
int oops = *ptr;
} |
There is one pattern that can work here, which is a callback-based API: auto ref apply(alias callback, T)(auto ref RefCounted!T this_)
{
// Ensures a reference is held until after `callback` returns
auto hold = this_;
// Explicit `scope` prevents `callback` from escaping a reference
scope ptr = () @trusted { return &this_.refCountedPayload(); }();
return callback(*ptr);
}
// Usage
@safe unittest
{
auto rc = RefCounted!int(123);
rc.apply!((ref n) {
destroy(rc);
int ok = n; // ok - kept alive by `hold`
});
} The downsides of this approach are that (a) the syntax isn't as nice, and (b) it requires an extra ref count increment/decrement at runtime. I think the best we can do, without a real ownership/borrowing system in the language ( |
@pbackus : What if someone calls |
@Geod24 The destructor should reset |
Going to do it like that, when I find time for it. Perhaps there is a way to get the |
A similar idea has been suggested before: https://issues.dlang.org/show_bug.cgi?id=21981 Currently, it would not work, because |
That feature of |
Well, it's entirely possible at this point that the issue with Until and unless we have 100% confirmation that |
Yeah, since DIP1035 can fix most but not all problems of Perhaps a forum topic is in order. On the other hand if DIP1035 gets rejected (I would be disappointed) no discussion is needed IMO: that behaviour is unambiguously a bug. |
Ahh, finally found the "convert to draft" button |
Mostly done in my local tree. Still trying to get automatic inference of I encountered one problem though: consistency. If |
54af44f
to
46a673f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly minor doc/formatting issues.
The removal of RefCounted
's test suite seems premature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the ugliness that is added by checking if the dip1000 switch is set or not. Why don't we just drop support for non-dip1000 phobos?
std/file.d
Outdated
// Must be a template, because the destructor is unsafe or safe depending on | ||
// whether `-preview=dip1000` is in use. Otherwise, linking errors would | ||
// result. | ||
struct DirIterator(bool useDIP1000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, this is a breaking change since DirIterator
is part of the public API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a documented symbol though, and BuildKite passing proves it doesn't break much. But yes it's possible someone still uses it.
Hmm, I wonder if this would avoid the breakage?
struct _DirIterator(bool useDIP1000) {/*...*/}
alias DirIterator = _DirIterator!dip1000Enabled;
Previously this led to a linking error. Alias to a template instance, the compiler thinks the linker can find the function precompiled in the Phobos binary, which it can't because it was compiled with DIP1000 resulting in different ABI. But on the other hand I tested without any template parameters - will using a bool parameter for DIP1000 change the fact? I quess not, but I have to test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tested this. For some reason, doing the alias trick works fine for DirIterator
without causing linking errors for non-dip1000 client code, but not for dirEntries
. With dirEntries
I have to keep the useDIP1000
template parameter there. Luckily it's not so much breaking change there because functions do not require using !()
for instantiation, except maybe if the user wants to take address of the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed the aforementioned change of DirIterator.
The switch is intended for non-dip1000 client code, not for compiling the Phobos binary without dip1000. Or did you mean dropping support for such client code? Way too early. DIP1000 is not even fully enabled by yet, giving only deprecations on violations. Typical codebase being what it is, we'd force well over half of them to migrate right away. I have migrated some code at Symmetry to dip1000 semantics. It's sometimes hard, unless you cop out with |
|
fbe4a5b
to
947ecbc
Compare
947ecbc
to
c8ddafd
Compare
Co-authored-by: Paul Backus <snarwin@gmail.com>
Co-authored-by: Paul Backus <snarwin@gmail.com>
All comments addressed. |
cc @Geod24 @atilaneves . Let's get this in! |
Hello? |
Looks like Dlang Bot is down, so the "72h -> merge" label doesn't do anything. Since it's been a full week, I think we can assume this is good to go. |
🚀🙌 |
Those fixes were done in PR dlang#8509 but PR dlang#8368 accidentally reverted them.
Those fixes were done in PR dlang#8509 but PR dlang#8368 accidentally reverted them.
Those fixes were done in PR dlang#8509 but PR dlang#8368 accidentally reverted them.
This is off Atila's #8101 . I wanted to create the PR against his fork but GitHub didn't allow that.
I tried to reproduce the safety bug @Geod24 found, but could not. It appears that language updates have automatically corrected that. In fact I had to add an additional
@trusted
wrapper to the constructor in question so that the language would not prevent it's use withscope
data totally. I added a test to verify that the safety bug stays gone.I also had the destructor to check that
dip1000
is enabled, otherwise it now stays@system
.