sciter-sdk/rust-sciter

Undefined Behavior in Value impl

Opened this issue · 2 comments

There's a serious bug in the sciter::value::Value implementation related to std::ops::Index:

let mut v = sciter::value::Value::array(2);
v.set(0, 5);
v.set(1, 6);
let v_0 = &v[0];
let v_1 = &v[1];
dbg!(v_0);
dbg!(v_1);

This will print something like this, which is obviously incorrect:

[src/main.rs:176] v_0 = int:6
[src/main.rs:177] v_1 = int:6

The culprit is this code, which reuses the tmp field in a &self method:

#[allow(clippy::mut_from_ref)]
fn ensure_tmp_mut(&self) -> &mut Value {
    let cp = self as *const Value;
    let mp = cp as *mut Value;
    let me = unsafe { &mut *mp };
    return me.ensure_tmp();
}

fn ensure_tmp(&mut self) -> &mut Value {
    if self.tmp.is_null() {
        let tmp = Box::new(Value::new());
        self.tmp = Box::into_raw(tmp);
    }
    return unsafe { &mut *self.tmp };
}

This isn't just a bug, it's also straight up Undefined Behavior. The Clippy mut_from_ref lint is there for a reason...

So, to provide a little more context, Rust's Index and IndexMut traits are specifically intended for container types, in which the result of the index operation already exists, so that you can return a reference to it. In your implementation of Index however, the result is created on the fly, so you can't return a reference to it. There are two solutions:

  1. Since the indexed value already exists in memory, you could in theory return a reference to it. ValueNthElementValue() computes this reference, but it copies the referenced value to another location instead of returning a pointer. You could reimplement ValueNthElementValue(), but that would violate the Sciter source code license agreement, and the reimplementation would be susceptible to changes in the underlying data structure. So that's a no-go.
  2. Delete the Index and IndexMut implementations and replace their uses with get(), set(), get_item(), and set_item().

Yeah, I know that Rust doesn't support that and introducing the indexing via a hack was a bad idea. It's mentioned in #27.