-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Soundness violation with self-written futures #49345
Comments
@eernstg in labeling this with |
That was what I was thinking, but it wouldn't be dead ( |
@mraleph just clarified a bunch of things. Here's a simpler example: class F implements Future<int> {
@override
dynamic noSuchMethod(i) async {
i.positionalArguments[0]('Hello, int!');
}
}
void main() async {
var x = await F();
print('x: $x, of type: ${x.runtimeType}, isEven: ${x.isEven}');
// Prints 'x: Hello, int!, of type: int, isEven: true'.
} The underlying issue seems to be that |
We have looked at this briefly with @eernstg and discovered that the underlying soundness issue is actually related to VM (or dart2js) implementation of the async rather than CFE - VM passes a very loosely typed closure to import 'dart:async';
void foo(Future<String> f) async {
final String result = await f;
print(result.runtimeType);
}
class F<T> implements Future<T> {
Future<R> then<R>(FutureOr<R> Function(T) onValue, {Function? onError}) {
return Future.value((onValue as FutureOr<R> Function(dynamic))(10));
}
@override
dynamic noSuchMethod(i) => throw 'Unimplimented';
}
void main() async {
foo(F<String>());
} Here we would pass It seems that /cc @alexmarkov |
[Edit: This is just a joke, please ignore. ;-] So the canonical arbitrary number is 7 or 87, the canonical string is |
Future<Never>
It seems like in order to close this loophole we should either
=>
=>
where implementation of await can use The first approach is simple and can be done in the front-end to share across implementations, but may incur significant performance and code size overheads. The second approach probably has better performance and code size, but passing the expected static type to I think this problem is a good example of unnecessary complexity and additional overheads caused by user-defined Future implementations, which are rarely used. We should consider deprecating user-defined futures to avoid that. /cc @lrhn |
@alexmarkov I think there is a third approach where we do something like this pseudo-code inside awaitImpl(Future x) {
if (x is _Future) {
// Old case: value produced by [_Future.then] is guaranteed to be a subtype of
// static type of a variable which contained this future. No need to use
// tightly typed callback.
x.then(_genericThenCallback);
} else if (x is Future<var T>) {
// Enforce types for custom future implementations.
x.then((T value) => _genericThenCallback(value));
}
} The idea here is that we don't really need static type at the await side. Instead we could enforce a very strong constraint that an instance of This has no code size implications at call-site, but unfortunately requires allocating closures. I think there is a possibility to avoid that as well, by using continuation approach: we stash T and suspend PC in a separate place in the suspend state and then set suspend PC to point to a stub which type-checks the value and then redirects to actual suspend PC. |
A If we have a combination of a Alternatively, we can say that it's unspecified behavior what happens if a future does not follow the |
@mraleph Deriving the expected type of value from future instance is a great idea! Note that user-defined future class might implement Future, not extend it, so we cannot query type from the type arguments vector. We would probably need to make a runtime call to search Future recursively among base classes and implemented interfaces in order to get the expected type. On top of the runtime call allocating a new closure is not that heavy. Maybe we should prefer creating new closures instead of increasing size of SuspendState objects (memory usage) and adding more complexity to the stubs. This approach should not add any overhead to the common case of built-in futures, and only make awaiting user-defined futures slower. I'll create a CL for that. @lrhn Yes, that's what we'll probably end up doing in the implementation. Note that providing a callback which type is specialized for every await means the implementation cannot reuse the same callback for all awaits. |
You could reuse the same callback and inline the type check at the point of the await instead, a place which should have access to the expected type. As long as it throws a some time before the |
This approach is mentioned in #49345 (comment) but it has bad code size implications. |
What's the percentage of The main stumbling block here would probably be that an overridden method could change this: So we probably just can't know this sufficiently frequently to make it useful. |
Fix for the VM: https://dart-review.googlesource.com/c/sdk/+/250222. @sigmundch Both dart2js and DDC fail on the regression test (derived from #49345 (comment)) so they probably have this bug too. I'm going to approve the failures when landing the VM fix. |
TEST=language/async/await_user_defined_future_soundness_test Issue: #49345 Change-Id: Ieaaa9baace13dad242c770a710d4d459e135af81 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/250222 Reviewed-by: Slava Egorov <vegorov@google.com> Commit-Queue: Alexander Markov <alexmarkov@google.com>
Dart VM fix landed (abedfaf). |
Thanks @alexmarkov! FYI @rakudrama @nshahan |
…_soundness_test This change is a follow-up to https://dart-review.googlesource.com/c/sdk/+/250222. TEST=language/async/await_user_defined_future_soundness_test Issue: #49345 Change-Id: I9e486a0a90fbe6df74398bd11a2be805e6d1c0a4 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/250404 Reviewed-by: Lasse Nielsen <lrn@google.com> Auto-Submit: Alexander Markov <alexmarkov@google.com> Commit-Queue: Lasse Nielsen <lrn@google.com>
Thanks to Sergejs for detecting this soundness violation!
Consider the following program:
This program prints 'hello' and then throws a
NoSuchMethodError
because it attempts to invokeisEven
on the null object, using dart 2.17.0-266.5.beta, but this is a soundness violation because it implies thatawait guard
evaluated to null in spite of the fact that the expression has static typeNever
. The execution does not use unsound null safety, so an expression of typeNever
should not yield an object.Interestingly, the kernel transformation obtained by running the following command:
where the example program is stored in
n011.dart
anddart2kernel
is the following script:shows that
foo
is translated into the following kernel code:I do not understand how the evaluation of the
let
expression can complete normally. Also, it is surprising thatawait guard
can complete normally in the first place, because that expression has static typeNever
.Note that we do get the expected reachability error if the example is executed with
--no-sound-null-safety
.The text was updated successfully, but these errors were encountered: