Skip to content
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

[vm/ffi] Disallow attaching NativeFinalizers to deeply immutable objects #55067

Closed
dcharkes opened this issue Feb 29, 2024 · 2 comments
Closed
Assignees
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. library-ffi

Comments

@dcharkes
Copy link
Contributor

=> We may want to disallow attaching native finalizers (with run-eagerly-on-isolate-shutdown semantics) to sharable objects.

This is somewhat of a separate issue, but may be worthwhile fixing.

Already today one can create deeply immutable objects (dart constants, but also deeply immutable typed data arrays). Attaching finalizers to them seems not a good idea. @dcharkes could you make a CL to disallow that?

Originally posted by @mkustermann in #55050 (comment)

@dcharkes dcharkes added area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-ffi labels Feb 29, 2024
@dcharkes dcharkes self-assigned this Feb 29, 2024
@dcharkes
Copy link
Contributor Author

dcharkes commented Mar 4, 2024

CheckWritableInstr is specialized for TypedData currently.

DEFINE_RUNTIME_ENTRY(WriteError, 0) {
Exceptions::ThrowUnsupportedError("Cannot modify an unmodifiable list");
}

We could parameterize WriteEntryInstr with en enum for what error to throw, and duplicate the runtime entry.

We could generalize the error message to "The object is immutable.", but that makes the error messages poor (both for trying to modify an immutable typed data and for trying to attach a native finalizer to an immutable object).

It might not be needed to go the slow-path + runtime entry route for NativeFinalizer.attach (it does a bunch of other checks as well). So we could also introduce a new IsImmutableInstr that just returns a bool and do the throwing in Dart code instead. On the other hand, it might not hurt to optimize NativeFinalizer.attach to do the throwing in a stub.

@rmacnak-google Any preferences?

@rmacnak-google
Copy link
Contributor

I definitely prefer separate error messages over a vague general error message.

Parameterizing WriteEntryInstr or a new IsImmutableInstr both seem fine to me, so whatever you find more convenient.

copybara-service bot pushed a commit that referenced this issue Mar 28, 2024
Note this changes the error in AOT to tell what is the typed data that
is not writable.

This CL eases the follow up CL which starts reusing the
`CheckWritableInstr` and accompanying slow path and runtime entry for
other messages.

TEST=tests/lib/typed_data/polymorphic_unmodifiable_typed_data_test.dart

Bug: #55067
Change-Id: Icf26d119501059e2ea4e885897539ebcb1063da0
Cq-Include-Trybots: dart-internal/g3.dart-internal.try:g3-cbuild-try
Cq-Include-Trybots: dart/try:vm-aot-android-release-arm64c-try,vm-aot-android-release-arm_x64-try,vm-aot-linux-debug-simarm_x64-try,vm-aot-linux-debug-simriscv64-try,vm-aot-linux-debug-x64-try,vm-aot-linux-debug-x64c-try,vm-aot-mac-release-arm64-try,vm-aot-mac-release-x64-try,vm-aot-obfuscate-linux-release-x64-try,vm-aot-optimization-level-linux-release-x64-try,vm-aot-win-debug-arm64-try,vm-aot-win-debug-x64c-try,vm-aot-win-release-x64-try,vm-appjit-linux-debug-x64-try,vm-asan-linux-release-x64-try,vm-checked-mac-release-arm64-try,vm-eager-optimization-linux-release-ia32-try,vm-eager-optimization-linux-release-x64-try,vm-linux-debug-ia32-try,vm-linux-debug-simriscv64-try,vm-linux-debug-x64-try,vm-linux-debug-x64c-try,vm-linux-release-simarm-try,vm-mac-debug-arm64-try,vm-mac-debug-x64-try,vm-msan-linux-release-x64-try,vm-reload-linux-debug-x64-try,vm-reload-rollback-linux-debug-x64-try,vm-ubsan-linux-release-x64-try,vm-win-debug-arm64-try,vm-win-debug-x64-try,vm-win-debug-x64c-try,vm-win-release-ia32-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/359063
Reviewed-by: Ilya Yanok <yanok@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. library-ffi
Projects
None yet
Development

No branches or pull requests

2 participants