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

Undefined Behavior in Value impl #114

Open
xq-tec opened this issue Jun 7, 2021 · 2 comments
Open

Undefined Behavior in Value impl #114

xq-tec opened this issue Jun 7, 2021 · 2 comments
Labels

Comments

@xq-tec
Copy link

xq-tec commented Jun 7, 2021

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...

@pravic pravic added the bug label Jun 7, 2021
@xq-tec
Copy link
Author

xq-tec commented Jun 8, 2021

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().

@pravic
Copy link
Member

pravic commented Jun 8, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

1 participant