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

Mitigate unsoundness in InternalState #387

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion boa/src/builtins/console/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,12 @@ use crate::{
},
exec::Interpreter,
};
use gc::{Finalize, Trace};
use rustc_hash::FxHashMap;
use std::time::SystemTime;

/// This is the internal console object state.
#[derive(Debug, Default)]
#[derive(Debug, Default, Trace, Finalize)]
pub struct ConsoleState {
count_map: FxHashMap<String, u32>,
timer_map: FxHashMap<String, u128>,
Expand Down
6 changes: 5 additions & 1 deletion boa/src/builtins/function/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,16 @@ pub enum ConstructorKind {
/// Defines how this references are interpreted within the formal parameters and code body of the function.
///
/// Arrow functions don't define a `this` and thus are lexical, `function`s do define a this and thus are NonLexical
#[derive(Trace, Finalize, Debug, Clone)]
#[derive(Finalize, Copy, Debug, Clone)]
pub enum ThisMode {
Lexical,
NonLexical,
}

unsafe impl Trace for ThisMode {
unsafe_empty_trace!();
}

/// FunctionBody is specific to this interpreter, it will either be Rust code or JavaScript code (AST Node)
#[derive(Clone, Finalize)]
pub enum FunctionBody {
Expand Down
38 changes: 15 additions & 23 deletions boa/src/builtins/object/internal_state.rs
Original file line number Diff line number Diff line change
@@ -1,19 +1,18 @@
//! Implementations for storing normal rust structs inside any object as internal state.

use gc::{unsafe_empty_trace, Finalize, Trace};
use std::{
any::Any,
fmt::{self, Debug},
ops::{Deref, DerefMut},
ops::Deref,
rc::Rc,
};

use gc::{unsafe_empty_trace, Finalize, Trace};

/// Wrapper around `Rc` to implement `Trace` and `Finalize`.
#[derive(Clone)]
pub struct InternalStateCell {
/// The internal state.
state: Rc<dyn Any>,
state: Rc<dyn InternalState + 'static>,
}

impl Finalize for InternalStateCell {}
Expand All @@ -22,19 +21,6 @@ unsafe impl Trace for InternalStateCell {
unsafe_empty_trace!();
}

impl Deref for InternalStateCell {
type Target = dyn Any;
fn deref(&self) -> &Self::Target {
Deref::deref(&self.state)
}
}

impl DerefMut for InternalStateCell {
fn deref_mut(&mut self) -> &mut Self::Target {
Rc::get_mut(&mut self.state).expect("failed to get mutable")
}
}

/// The derived version would print 'InternalStateCell { state: ... }', this custom implementation
/// only prints the actual internal state.
impl Debug for InternalStateCell {
Expand All @@ -45,20 +31,26 @@ impl Debug for InternalStateCell {

impl InternalStateCell {
/// Create new `InternalStateCell` from a value.
pub fn new<T: Any + InternalState>(value: T) -> Self {
pub fn new<T: 'static + InternalState>(value: T) -> Self {
Self {
state: Rc::new(value),
}
}

/// Get a reference to the stored value and cast it to `T`.
pub fn downcast_ref<T: Any + InternalState>(&self) -> Option<&T> {
self.deref().downcast_ref::<T>()
pub fn downcast_ref<T: InternalState>(&self) -> Option<&T> {
let state = Deref::deref(&mut self.state);

(state as &dyn Any).downcast_ref::<T>()
}

/// Get a mutable reference to the stored value and cast it to `T`.
pub fn downcast_mut<T: Any + InternalState>(&mut self) -> Option<&mut T> {
self.deref_mut().downcast_mut::<T>()
pub fn downcast_mut<T: InternalState>(&mut self) -> Option<&mut T> {
let state = Rc::get_mut(&mut self.state).expect("failed to get mutable");

(state as &mut dyn Any).downcast_mut::<T>()
}
}

/// This trait must be implemented by all structs used for internal state.
pub trait InternalState: Debug {}
pub trait InternalState: Debug + Trace + Any {}
Copy link
Member Author

Choose a reason for hiding this comment

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

It seems that this is not enough to be able to call downcast_mut() on self.state. It gives the following error:

method not found in `&(dyn builtins::object::internal_state::InternalState + 'static)`

Copy link
Member Author

Choose a reason for hiding this comment

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

After some refactoring, I'm getting this error:

error[E0605]: non-primitive cast: `&dyn builtins::object::internal_state::InternalState` as `&(dyn std::any::Any + 'static)`
  --> boa/src/builtins/object/internal_state.rs:44:9
   |
44 |         (state as &dyn Any).downcast_ref::<T>()
   |         ^^^^^^^^^^^^^^^^^^^
   |
   = note: an `as` expression can only be used to convert between primitive types. Consider using the `From` trait

error[E0605]: non-primitive cast: `&mut (dyn builtins::object::internal_state::InternalState + 'static)` as `&mut (dyn std::any::Any + 'static)`
  --> boa/src/builtins/object/internal_state.rs:51:9
   |
51 |         (state as &mut dyn Any).downcast_mut::<T>()
   |         ^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: an `as` expression can only be used to convert between primitive types. Consider using the `From` trait

15 changes: 10 additions & 5 deletions boa/src/builtins/regexp/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,6 @@
//! [spec]: https://tc39.es/ecma262/#sec-regexp-constructor
//! [mdn]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp

use std::ops::Deref;

use regex::Regex;

use crate::{
builtins::{
object::{InternalState, Object, ObjectInternalMethods, ObjectKind, PROTOTYPE},
Expand All @@ -21,12 +17,15 @@ use crate::{
},
exec::Interpreter,
};
use gc::{unsafe_empty_trace, Finalize, Trace};
use regex::Regex;
use std::ops::Deref;

#[cfg(test)]
mod tests;

/// The internal representation on a `RegExp` object.
#[derive(Debug)]
#[derive(Debug, Finalize)]
struct RegExp {
/// Regex matcher.
matcher: Regex,
Expand Down Expand Up @@ -56,6 +55,12 @@ struct RegExp {
unicode: bool,
}

// FIXME: Maybe `Regex` could at some point implement `Trace`, and we need to take that into
// account.
unsafe impl Trace for RegExp {
unsafe_empty_trace!();
}

impl InternalState for RegExp {}

/// Create a new `RegExp`
Expand Down
5 changes: 1 addition & 4 deletions boa/src/builtins/value/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -487,10 +487,7 @@ impl ValueData {
///
/// This will panic if this value doesn't have an internal state or if the internal state doesn't
/// have the concrete type `S`.
pub fn with_internal_state_ref<S: Any + InternalState, R, F: FnOnce(&S) -> R>(
&self,
f: F,
) -> R {
pub fn with_internal_state_ref<S: InternalState, R, F: FnOnce(&S) -> R>(&self, f: F) -> R {
if let Self::Object(ref obj) = *self {
let o = obj.borrow();
let state = o
Expand Down