Undefined Behavior in Value impl
Opened this issue · 2 comments
Deleted user commented
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...
Deleted user commented
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:
- 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 reimplementValueNthElementValue()
, 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. - Delete the
Index
andIndexMut
implementations and replace their uses withget()
,set()
,get_item()
, andset_item()
.