search

denoland / rusty_v8

Rust bindings for the V8 JavaScript engine
https://crates.io/crates/v8
MIT License
3.13k stars 300 forks source link

fix: ExternalOneByteStringResource is not guaranteed to be valid UTF-8 #1532

Closed aapoalas closed 1 month ago

aapoalas commented 1 month ago

A subtle unsoundness / undefined behaviour made its way into the fairly recently added ExternalOneByteStringResource object: The as_str API is not sound as the data inside may be be Latin-1, not ASCII.

As the API was not used anywhere in deno or deno_core, I opted to simply remove it and replace it with an as_bytes API. I also modified the test to showcase the Latin-1 string case and added copious notes and explanations around the code to make sure this doesn't accidentally happen again. The likely reason why the API originally slipped in is because the OneByteConst has this API where it is safe because the OneByteConst creation checks the data for ASCII-ness.

I also tried to add an API to extract an Option<&'static OneByteConst> from an &ExternalOneByteStringResource but run into https://github.com/rust-lang/rust/issues/119618 ie. OneByteConst is actually duplicating the vtables... which is not great.

Closes #1531

bartlomieju commented 1 month ago

Closes https://github.com/denoland/rusty_v8/issues/1531?

aapoalas commented 1 month ago

Closes #1531?

Oh, yes

aapoalas commented 1 month ago

LGTM provided that the answer to #1532 (comment) is "yes"

I couldn't give a full "yes" so I fixed the code to check for length being zero. If length is zero, then the pointer could also theoretically be null.