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.

Sign up for GitHub

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

Jump to bottom

feat(ops): support raw pointer arguments #16826

Merged
merged 4 commits into from
Nov 26, 2022
Merged

feat(ops): support raw pointer arguments #16826

merged 4 commits into from
Nov 26, 2022

Conversation

littledivy
Copy link
Member

@littledivy littledivy commented Nov 26, 2022 •
edited
Loading

See #16814 (comment). Allows nullable buffers in low-level ops like FFI:

fn op_ffi_ptr_of<FP>(
  state: &mut OpState,
  buf: *const u8,
  out: &mut [u32],
) 
where
  FP: FfiPermissions + 'static {
  // ..
}

@littledivy littledivy mentioned this pull request Nov 26, 2022
Merged
@littledivy littledivy marked this pull request as ready for review November 26, 2022 05:53
@aapoalas
Copy link
Collaborator

This will be problematic if/when I get my Fast API void pointer CL merged to V8, as we'll probably want to use pointers to represent that... Except! Yes, this is good. Represent Uint8Array's pointer using *mut u8, u32 for Uint32Array etc. and use *mut c_void for Externals.

aapoalas
aapoalas reviewed Nov 26, 2022
View reviewed changes
ext/ffi/lib.rs
@@ -2065,7 +2065,7 @@ fn op_ffi_call_nonblocking<'scope>(
#[op(fast)]
fn op_ffi_ptr_of<FP>(
state: &mut deno_core::OpState,
buf: &[u8],
buf: *const u8,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider using *mut u8, not because we're going to do mutation but because we want this to be from an ArrayBuffer which are always mut.

Copy link
Member Author

Choose a reason for hiding this comment

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

*const u8 does not suggest that the pointed value is not mutable. You can safely cast it to *mut u8. It's just a hint that we are only dealing with the non-mut reference in the current context.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I'm just thinking that there might be a day when *const u8 is needed to stand in for some V8 API type with Fast API support that offers a pointer without mutable access. Admittedly this is quite an unlikely thing scenario, but still a theoretical possibility.

I guess another way to go about this would use some transparent newtype like to tell the various pointer types apart in the ops macro:

#[repr(transparent)]
struct Uint8ArrayPointer(*const u8)

But yeah, since this is an unlikely scenario it doesn't really matter.

@littledivy
Copy link
Member Author

littledivy commented Nov 26, 2022 •
edited
Loading

*mut c_void for Externals.

@aapoalas IMO i'd be nice to have a magic type Resource<T> for when externals are supported, since 99% of cases will dereference it to T anyways. For FFI, yes we can do *mut c_void

@aapoalas
Copy link
Collaborator

*mut c_void for Externals.

@aapoalas IMO i'd be nice to have a magic type Resource<T> for when externals are supported, since 99% of cases will dereference it to T anyways. For FFI, yes we can do *mut c_void

Yeah. One thing that we will need to be careful about is the "substitution safety" required by V8's external pointer table, see this code comment and Samuel Groß's comment on it: https://chromium-review.googlesource.com/c/v8/v8/+/3957815/18/include/v8-internal.h#387
Essentially, Deno should make sure that either it's safe to call a given API with an External of the wrong type (Resource<T> would accomplish this if each call verifies its Resource type, though FFI spoils that since then there are unknown pointers floating around as well) or makes sure that it's impossible to call the raw APIs directly. ops.op_some_resource_call's being exposed makes this impossible as well...

It's a bit of a complicated situation that probably cannot be resolved easily but should be taken up as a future goal. The reason for aiming for this is that pointer swapping enables memory corruption attacks, which may at worst lead to exploits and sandbox escapes, as opposed to just segfaulting.

bartlomieju
bartlomieju approved these changes Nov 26, 2022
View reviewed changes
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM

@littledivy littledivy merged commit fcdcc8c into denoland:main Nov 26, 2022
@andreubotella andreubotella mentioned this pull request Nov 26, 2022
Closed
kt3k pushed a commit that referenced this pull request Dec 1, 2022
@littledivy @kt3k
feat(ops): support raw pointer arguments (#16826)
bcbcc72 
See #16814 (comment).
Allows nullable buffers in low-level ops like FFI:

```rust
fn op_ffi_ptr_of<FP>(
  state: &mut OpState,
  buf: *const u8,
  out: &mut [u32],
) 
where
  FP: FfiPermissions + 'static {
  // ..
}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aapoalas aapoalas aapoalas left review comments

@bartlomieju bartlomieju bartlomieju approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@littledivy @aapoalas @bartlomieju