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

Disallow memory slices as inputs to async ops #16738

Merged

Conversation

andreubotella
Copy link
Contributor

In Rust, it is UB if a slice is mutated while borrowed except through the slice itself, and it is also UB if a mutable slice is read while borrowed. The op macro allows borrowing an ArrayBuffer{,View} as a memory slice for the duration of an op, but this is not sound for async ops, since the ArrayBuffer could be accessed from JS during the await points. This PR therefore disallows such automatic borrowing only for async ops.

In Rust, it is UB if a slice is mutated while borrowed except through
the slice itself, and it is also UB if a mutable slice is read while
borrowed. The op macro allows borrowing an `ArrayBuffer{,View}` as a
memory slice for the duration of an op, but this is not sound for
async ops, since the `ArrayBuffer` could be accessed from JS during
the await points. This PR therefore disallows such automatic borrowing
only for async ops.
@andreubotella
Copy link
Contributor Author

@littledivy PTAL

@crowlKats
Copy link
Member

@andreubotella a test is failing; could you take a look?

@andreubotella
Copy link
Contributor Author

@andreubotella a test is failing; could you take a look?

The failing test is simply an ops optimizer test added in #16579 that passes a memory slice to a fast async op. Since the point of that PR seems to be to allow op_read to use fast calls, disallowing memory slices might limit the usefulness of that optimization, even though implementing op_read like this would make UB inevitable.

cc @littledivy

@littledivy littledivy self-assigned this Jan 15, 2023
@littledivy littledivy enabled auto-merge (squash) January 15, 2023 06:10
Copy link
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

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

LGTM. I'm in favour of disabling slices in async ops. IIRC there is no fast async op using it anyways

@littledivy littledivy merged commit 90c0381 into denoland:main Jan 15, 2023
@andreubotella andreubotella deleted the disallow-memslices-in-async-ops branch January 15, 2023 22:23
bartlomieju pushed a commit that referenced this pull request Jan 16, 2023
In Rust, it is UB if a slice is mutated while borrowed except through
the slice itself, and it is also UB if a mutable slice is read while
borrowed. The op macro allows borrowing an `ArrayBuffer{,View}` as a
memory slice for the duration of an op, but this is not sound for async
ops, since the `ArrayBuffer` could be accessed from JS during the await
points. This PR therefore disallows such automatic borrowing only for
async ops.

Co-authored-by: Divy Srivastava <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants