Skip to content

Commit

Permalink
[vm] Fix soundness issue when awaiting a user-defined Future
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
alexmarkov authored and Commit Bot committed Jun 30, 2022
1 parent 4308eb3 commit abedfaf
Show file tree
Hide file tree
Showing 5 changed files with 171 additions and 3 deletions.
40 changes: 40 additions & 0 deletions runtime/lib/async.cc
Expand Up @@ -22,4 +22,44 @@ DEFINE_NATIVE_ENTRY(AsyncStarMoveNext_debuggerStepCheck, 0, 1) {
return Object::null();
}

// Instantiate generic [closure] using the type argument T
// corresponding to Future<T> in the given [future] instance
// (which may extend or implement Future).
DEFINE_NATIVE_ENTRY(SuspendState_instantiateClosureWithFutureTypeArgument,
0,
2) {
GET_NON_NULL_NATIVE_ARGUMENT(Closure, closure, arguments->NativeArgAt(0));
GET_NON_NULL_NATIVE_ARGUMENT(Instance, future, arguments->NativeArgAt(1));
IsolateGroup* isolate_group = thread->isolate_group();

const auto& future_class =
Class::Handle(zone, isolate_group->object_store()->future_class());
ASSERT(future_class.NumTypeArguments() == 1);

const auto& cls = Class::Handle(zone, future.clazz());
auto& type =
AbstractType::Handle(zone, cls.GetInstantiationOf(zone, future_class));
ASSERT(!type.IsNull());
if (!type.IsInstantiated()) {
const auto& instance_type_args =
TypeArguments::Handle(zone, future.GetTypeArguments());
type =
type.InstantiateFrom(instance_type_args, Object::null_type_arguments(),
kNoneFree, Heap::kNew);
}
auto& type_args = TypeArguments::Handle(zone, type.arguments());
if (type_args.Length() != 1) {
// Create a new TypeArguments vector of length 1.
type = type_args.TypeAtNullSafe(0);
type_args = TypeArguments::New(1);
type_args.SetTypeAt(0, type);
}
type_args = type_args.Canonicalize(thread, nullptr);

ASSERT(closure.delayed_type_arguments() ==
Object::empty_type_arguments().ptr());
closure.set_delayed_type_arguments(type_args);
return closure.ptr();
}

} // namespace dart
1 change: 1 addition & 0 deletions runtime/vm/bootstrap_natives.h
Expand Up @@ -66,6 +66,7 @@ namespace dart {
V(SendPortImpl_sendInternal_, 2) \
V(Smi_bitNegate, 1) \
V(Smi_bitLength, 1) \
V(SuspendState_instantiateClosureWithFutureTypeArgument, 2) \
V(Mint_bitNegate, 1) \
V(Mint_bitLength, 1) \
V(Developer_debugger, 2) \
Expand Down
29 changes: 26 additions & 3 deletions sdk/lib/_internal/vm/lib/async_patch.dart
Expand Up @@ -419,6 +419,26 @@ class _SuspendState {
zone.scheduleMicrotask(run);
}

@pragma("vm:invisible")
@pragma("vm:prefer-inline")
void _awaitUserDefinedFuture(Future future) {
// Create a generic callback closure and instantiate it
// using the type argument of Future.
// This is needed to avoid unsoundness which may happen if user-defined
// Future.then casts callback to Function(dynamic) passes a value of
// incorrect type.
@pragma("vm:invisible")
dynamic typedCallback<T>(T value) {
return unsafeCast<dynamic Function(dynamic)>(_thenCallback)(value);
}

future.then(
unsafeCast<dynamic Function(dynamic)>(
_instantiateClosureWithFutureTypeArgument(typedCallback, future)),
onError:
unsafeCast<dynamic Function(Object, StackTrace)>(_errorCallback));
}

@pragma("vm:entry-point", "call")
@pragma("vm:invisible")
Object? _await(Object? object) {
Expand All @@ -437,9 +457,7 @@ class _SuspendState {
} else if (object is! Future) {
_awaitNotFuture(object);
} else {
object.then(unsafeCast<dynamic Function(dynamic)>(_thenCallback),
onError:
unsafeCast<dynamic Function(Object, StackTrace)>(_errorCallback));
_awaitUserDefinedFuture(object);
}
return _functionData;
}
Expand Down Expand Up @@ -612,6 +630,11 @@ class _SuspendState {
@pragma("vm:recognized", "other")
@pragma("vm:prefer-inline")
external _SuspendState _clone();

@pragma("vm:external-name",
"SuspendState_instantiateClosureWithFutureTypeArgument")
external static Function _instantiateClosureWithFutureTypeArgument(
dynamic Function<T>(T) closure, Future future);
}

class _SyncStarIterable<T> extends Iterable<T> {
Expand Down
51 changes: 51 additions & 0 deletions tests/language/async/await_user_defined_future_soundness_test.dart
@@ -0,0 +1,51 @@
// Copyright (c) 2022, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

// Verifies that user-define Future cannot provide a value of incorrect

This comment has been minimized.

Copy link
@lrhn

lrhn Jul 1, 2022

Member

user-define -> a user-defined

// type by casting 'onValue' callback.
// Regression test for https://github.com/dart-lang/sdk/issues/49345.

import 'dart:async';

import "package:expect/expect.dart";

import 'dart:async';

bool checkpoint1 = false;
bool checkpoint2 = false;
bool checkpoint3 = false;
bool checkpoint4 = false;

Future<void> foo(Future<String> f) async {
checkpoint1 = true;
final String result = await f;
checkpoint3 = true;
print(result.runtimeType);
}

class F implements Future<String> {
Future<R> then<R>(FutureOr<R> Function(String) onValue, {Function? onError}) {
checkpoint2 = true;
final result = (onValue as FutureOr<R> Function(dynamic))(10);

This comment has been minimized.

Copy link
@lrhn

lrhn Jul 1, 2022

Member

I'd insert a delay before calling onValue.

Calling the onValue synchronously in the then call is itself unsupported, and could be the reason for a state failure instead of the type error.

The checkpoints should not be necessary, just use Expect.throws.

Consider doing:

import "package:async_helper/async_helper.dart";
// ...
  Future<R> then<R>(FutureOr<R> Function(String) onValue, {Function? onError}) {
    scheduleMicrotask(() {
      Expect.throws(() { 
         (onValue as FutureOr<R> Function(dynamic))(10);
      });
      asyncEnd();
   });
 });
 
// ...
void main() {
 asyncStart(); 
 foo(F());
}

Then the async test will only succeed if asyncEnd() is called, which only happens if Expect.throws succeeds.

checkpoint4 = true;
return Future.value(result);
}

@override
dynamic noSuchMethod(i) => throw 'Unimplimented';

This comment has been minimized.

Copy link
@lrhn

lrhn Jul 1, 2022

Member

"Unimplimented" -> "Unimplemented"

Or better: UnimplementedError().

}

void main() {
bool seenError = false;
runZoned(() {
foo(F());
}, onError: (e, st) {
seenError = true;
});
Expect.isTrue(checkpoint1);
Expect.isTrue(checkpoint2);
Expect.isFalse(checkpoint3);
Expect.isFalse(checkpoint4);
Expect.isTrue(seenError);
}
@@ -0,0 +1,53 @@
// Copyright (c) 2022, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

// @dart = 2.9

// Verifies that user-define Future cannot provide a value of incorrect
// type by casting 'onValue' callback.
// Regression test for https://github.com/dart-lang/sdk/issues/49345.

import 'dart:async';

import "package:expect/expect.dart";

import 'dart:async';

bool checkpoint1 = false;
bool checkpoint2 = false;
bool checkpoint3 = false;
bool checkpoint4 = false;

Future<void> foo(Future<String> f) async {
checkpoint1 = true;
final String result = await f;
checkpoint3 = true;
print(result.runtimeType);
}

class F implements Future<String> {
Future<R> then<R>(FutureOr<R> Function(String) onValue, {Function onError}) {
checkpoint2 = true;
final result = (onValue as FutureOr<R> Function(dynamic))(10);
checkpoint4 = true;
return Future.value(result);
}

@override
dynamic noSuchMethod(i) => throw 'Unimplimented';
}

void main() {
bool seenError = false;
runZoned(() {
foo(F());
}, onError: (e, st) {
seenError = true;
});
Expect.isTrue(checkpoint1);
Expect.isTrue(checkpoint2);
Expect.isFalse(checkpoint3);
Expect.isFalse(checkpoint4);
Expect.isTrue(seenError);
}

0 comments on commit abedfaf

Please sign in to comment.