Skip to content

Commit

Permalink
SlotMap.compute can lead to deadlock in ThreadSafeSlotMapContainer (#…
Browse files Browse the repository at this point in the history 
…1746)

Refactor code to prevent a deadlock that can arise when Rhino is run with the "thread safe objects" feature turned on.

Co-authored-by: duncan.macgregor <duncan.macgregor@servicenow.com>
  • Loading branch information
@nabacg @aardvark179
nabacg and aardvark179 authored Dec 11, 2024
1 parent 2fcc422 commit 80933c5
Show file tree
Hide file tree
Showing 9 changed files with 336 additions and 86 deletions.
48 changes: 48 additions & 0 deletions rhino/src/main/java/org/mozilla/javascript/AccessorSlot.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,10 +124,34 @@ Function getGetterFunction(String name, Scriptable scope) {
return getter.asGetterFunction(name, scope);
}

@Override
boolean isSameGetterFunction(Object function) {
if (function == Scriptable.NOT_FOUND) {
return true;
}
if (getter == null) {
return ScriptRuntime.shallowEq(Undefined.instance, function);
}
return getter.isSameGetterFunction(function);
}

@Override
boolean isSameSetterFunction(Object function) {
if (function == Scriptable.NOT_FOUND) {
return true;
}
if (setter == null) {
return ScriptRuntime.shallowEq(Undefined.instance, function);
}
return setter.isSameSetterFunction(function);
}

interface Getter {
Object getValue(Scriptable start);

Function asGetterFunction(final String name, final Scriptable scope);

boolean isSameGetterFunction(Object getter);
}

/** This is a Getter that delegates to a Java function via a MemberBox. */
Expand All @@ -150,6 +174,11 @@ public Object getValue(Scriptable start) {
public Function asGetterFunction(String name, Scriptable scope) {
return member.asGetterFunction(name, scope);
}

@Override
public boolean isSameGetterFunction(Object function) {
return member.isSameGetterFunction(function);
}
}

/** This is a getter that delegates to a JavaScript function. */
Expand All @@ -175,12 +204,20 @@ public Object getValue(Scriptable start) {
public Function asGetterFunction(String name, Scriptable scope) {
return target instanceof Function ? (Function) target : null;
}

@Override
public boolean isSameGetterFunction(Object function) {
return ScriptRuntime.shallowEq(
target instanceof Function ? (Function) target : Undefined.instance, function);
}
}

interface Setter {
boolean setValue(Object value, Scriptable owner, Scriptable start);

Function asSetterFunction(final String name, final Scriptable scope);

boolean isSameSetterFunction(Object getter);
}

/** Invoke the setter on this slot via reflection using MemberBox. */
Expand Down Expand Up @@ -213,6 +250,11 @@ public boolean setValue(Object value, Scriptable owner, Scriptable start) {
public Function asSetterFunction(String name, Scriptable scope) {
return member.asSetterFunction(name, scope);
}

@Override
public boolean isSameSetterFunction(Object function) {
return member.isSameSetterFunction(function);
}
}

/**
Expand All @@ -239,5 +281,11 @@ public boolean setValue(Object value, Scriptable owner, Scriptable start) {
public Function asSetterFunction(String name, Scriptable scope) {
return target instanceof Function ? (Function) target : null;
}

@Override
public boolean isSameSetterFunction(Object function) {
return ScriptRuntime.shallowEq(
target instanceof Function ? (Function) target : Undefined.instance, function);
}
}
}
82 changes: 63 additions & 19 deletions rhino/src/main/java/org/mozilla/javascript/IdScriptableObject.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -867,8 +867,8 @@ protected void defineOwnProperty(
delete(id); // it will be replaced with a slot
} else {
checkPropertyDefinition(desc);
ScriptableObject current = getOwnPropertyDescriptor(cx, key);
checkPropertyChange(name, current, desc);
var slot = queryOrFakeSlot(cx, key);
checkPropertyChangeForSlot(name, slot, desc);
int attr = (info >>> 16);
Object value = getProperty(desc, "value");
if (value != NOT_FOUND && ((attr & READONLY) == 0 || (attr & PERMANENT) == 0)) {
Expand All @@ -877,7 +877,12 @@ protected void defineOwnProperty(
setInstanceIdValue(id, value);
}
}
attr = applyDescriptorToAttributeBitset(attr, desc);
attr =
applyDescriptorToAttributeBitset(
attr,
getProperty(desc, "enumerable"),
getProperty(desc, "writable"),
getProperty(desc, "configurable"));
setAttributes(name, attr);
return;
}
Expand All @@ -889,8 +894,8 @@ protected void defineOwnProperty(
prototypeValues.delete(id); // it will be replaced with a slot
} else {
checkPropertyDefinition(desc);
ScriptableObject current = getOwnPropertyDescriptor(cx, key);
checkPropertyChange(name, current, desc);
var slot = queryOrFakeSlot(cx, key);
checkPropertyChangeForSlot(name, slot, desc);
int attr = prototypeValues.getAttributes(id);
Object value = getProperty(desc, "value");
if (value != NOT_FOUND && (attr & READONLY) == 0) {
Expand All @@ -900,7 +905,12 @@ protected void defineOwnProperty(
}
}
prototypeValues.setAttributes(
id, applyDescriptorToAttributeBitset(attr, desc));
id,
applyDescriptorToAttributeBitset(
attr,
getProperty(desc, "enumerable"),
getProperty(desc, "writable"),
getProperty(desc, "configurable")));

// Handle the regular slot that was created if this property was previously
// replaced
Expand All @@ -922,62 +932,96 @@ protected ScriptableObject getOwnPropertyDescriptor(Context cx, Object id) {
ScriptableObject desc = super.getOwnPropertyDescriptor(cx, id);
if (desc == null) {
if (id instanceof String) {
return getBuiltInDescriptor((String) id);
return getBuiltInDataDescriptor((String) id);
}

if (ScriptRuntime.isSymbol(id)) {
if (id instanceof SymbolKey) {
return getBuiltInDescriptor((SymbolKey) id);
return getBuiltInDataDescriptor((SymbolKey) id);
}

return getBuiltInDescriptor(((NativeSymbol) id).getKey());
return getBuiltInDataDescriptor(((NativeSymbol) id).getKey());
}
}
return desc;
}

private ScriptableObject getBuiltInDescriptor(String name) {
Object value = null;
int attr = EMPTY;
private Slot queryOrFakeSlot(Context cx, Object id) {
var slot = querySlot(cx, id);
if (slot == null) {
if (id instanceof String) {
return getBuiltInSlot((String) id);
}

if (ScriptRuntime.isSymbol(id)) {
if (id instanceof SymbolKey) {
return getBuiltInSlot((SymbolKey) id);
}

return getBuiltInSlot(((NativeSymbol) id).getKey());
}
}
return slot;
}

private ScriptableObject getBuiltInDataDescriptor(String name) {
Scriptable scope = getParentScope();
if (scope == null) {
scope = this;
}

var slot = getBuiltInSlot(name);
return slot == null ? null : buildDataDescriptor(scope, slot.value, slot.getAttributes());
}

private Slot getBuiltInSlot(String name) {
Object value = null;
int attr = EMPTY;

int info = findInstanceIdInfo(name);
if (info != 0) {
int id = (info & 0xFFFF);
value = getInstanceIdValue(id);
attr = (info >>> 16);
return buildDataDescriptor(scope, value, attr);
var slot = new Slot(name, 0, attr);
slot.value = value;
return slot;
}
if (prototypeValues != null) {
int id = prototypeValues.findId(name);
if (id != 0) {
value = prototypeValues.get(id);
attr = prototypeValues.getAttributes(id);
return buildDataDescriptor(scope, value, attr);
var slot = new Slot(name, 0, attr);
slot.value = value;
return slot;
}
}
return null;
}

private ScriptableObject getBuiltInDescriptor(Symbol key) {
Object value = null;
int attr = EMPTY;

private ScriptableObject getBuiltInDataDescriptor(Symbol key) {
Scriptable scope = getParentScope();
if (scope == null) {
scope = this;
}

var slot = getBuiltInSlot(key);
return slot == null ? null : buildDataDescriptor(scope, slot.value, slot.getAttributes());
}

private Slot getBuiltInSlot(Symbol key) {
Object value = null;
int attr = EMPTY;

if (prototypeValues != null) {
int id = prototypeValues.findId(key);
if (id != 0) {
value = prototypeValues.get(id);
attr = prototypeValues.getAttributes(id);
return buildDataDescriptor(scope, value, attr);
var slot = new Slot(key, 0, attr);
slot.value = value;
return slot;
}
}
return null;
Expand Down
20 changes: 19 additions & 1 deletion rhino/src/main/java/org/mozilla/javascript/LambdaAccessorSlot.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,17 @@ boolean isSetterSlot() {

@Override
ScriptableObject getPropertyDescriptor(Context cx, Scriptable scope) {
ScriptableObject desc = (ScriptableObject) cx.newObject(scope);
return buildPropertyDescriptor(cx);
}

/**
* The method exists avoid changing the getPropertyDescriptor signature and at the same time to
* make it explicit that we don't use Scriptable scope parameter of getPropertyDescriptor, since
* it can be problematic when called from inside ThreadSafeSlotMapContainer::compute lambda
* which can lead to deadlocks.
*/
public ScriptableObject buildPropertyDescriptor(Context cx) {
ScriptableObject desc = new NativeObject();

int attr = getAttributes();
boolean es6 = cx.getLanguageVersion() >= Context.VERSION_ES6;
Expand Down Expand Up @@ -139,4 +149,12 @@ public void setSetter(Scriptable scope, BiConsumer<Scriptable, Object> setter) {
});
}
}

public void replaceWith(LambdaAccessorSlot slot) {
this.getterFunction = slot.getterFunction;
this.getter = slot.getter;
this.setterFunction = slot.setterFunction;
this.setter = slot.setter;
setAttributes(slot.getAttributes());
}
}
10 changes: 10 additions & 0 deletions rhino/src/main/java/org/mozilla/javascript/MemberBox.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,16 @@ public String toString() {
return memberObject.toString();
}

boolean isSameGetterFunction(Object function) {
var f = asGetterFunction == null ? Undefined.instance : asGetterFunction;
return ScriptRuntime.shallowEq(function, f);
}

boolean isSameSetterFunction(Object function) {
var f = asSetterFunction == null ? Undefined.instance : asSetterFunction;
return ScriptRuntime.shallowEq(function, f);
}

/** Function returned by calls to __lookupGetter__ */
Function asGetterFunction(final String name, final Scriptable scope) {
// Note: scope is the scriptable this function is related to; therefore this function
Expand Down
Loading

0 comments on commit 80933c5

Please sign in to comment.