Skip to content

Bun.deepEquals: fix four false-positive equality classes #32872

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.

Sign up for GitHub

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

Open
robobun wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Bun.deepEquals: fix four false-positive equality classes #32872

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 4 additions & 4 deletions src/js/internal/sql/shared.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,10 @@ class SQLResultArray<T> extends PublicArray<T> {
// match postgres's result array, in this way for in will not list the
// properties and .map will not return undefined command and count
Object.defineProperties(this, {
count: { value: null, writable: true },
command: { value: null, writable: true },
lastInsertRowid: { value: null, writable: true },
affectedRows: { value: null, writable: true },
count: { value: null, writable: true, enumerable: false, configurable: true },
command: { value: null, writable: true, enumerable: false, configurable: true },
lastInsertRowid: { value: null, writable: true, enumerable: false, configurable: true },
affectedRows: { value: null, writable: true, enumerable: false, configurable: true },
});
}

Expand Down
135 changes: 95 additions & 40 deletions src/jsc/bindings/bindings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -734,6 +734,7 @@ bool Bun__deepEquals(JSC::JSGlobalObject* globalObject, JSValue v1, JSValue v2,
RETURN_IF_EXCEPTION(scope, false);
if (isSpecialEqual.has_value()) return WTF::move(*isSpecialEqual);
isSpecialEqual = specialObjectsDequal<isStrict, enableAsymmetricMatchers>(globalObject, gcBuffer, stack, scope, c2, c1);
RETURN_IF_EXCEPTION(scope, false);
if (isSpecialEqual.has_value()) return WTF::move(*isSpecialEqual);
JSObject* o1 = v1.getObject();
JSObject* o2 = v2.getObject();
Expand Down Expand Up @@ -796,51 +797,83 @@ bool Bun__deepEquals(JSC::JSGlobalObject* globalObject, JSValue v1, JSValue v2,
return false;
}

JSC::PropertyNameArrayBuilder a1(vm, PropertyNameMode::Symbols, PrivateSymbolMode::Exclude);
JSC::PropertyNameArrayBuilder a2(vm, PropertyNameMode::Symbols, PrivateSymbolMode::Exclude);
JSObject::getOwnPropertyNames(o1, globalObject, a1, DontEnumPropertiesMode::Exclude);
JSC::PropertyNameArrayBuilder a1(vm, PropertyNameMode::StringsAndSymbols, PrivateSymbolMode::Exclude);
JSC::PropertyNameArrayBuilder a2(vm, PropertyNameMode::StringsAndSymbols, PrivateSymbolMode::Exclude);
o1->getOwnNonIndexPropertyNames(globalObject, a1, DontEnumPropertiesMode::Exclude);
RETURN_IF_EXCEPTION(scope, false);
JSObject::getOwnPropertyNames(o2, globalObject, a2, DontEnumPropertiesMode::Exclude);
o2->getOwnNonIndexPropertyNames(globalObject, a2, DontEnumPropertiesMode::Exclude);
RETURN_IF_EXCEPTION(scope, false);

size_t propertyLength = a1.size();
const size_t propertyArrayLength1 = a1.size();
const size_t propertyArrayLength2 = a2.size();
if constexpr (isStrict) {
if (propertyLength != a2.size()) {
if (propertyArrayLength1 != propertyArrayLength2) {
return false;
}
}

UncheckedKeyHashSet<UniquedStringImpl*> a2Names;
a2Names.reserveInitialCapacity(propertyArrayLength2);
for (size_t p = 0; p < propertyArrayLength2; p++) {
a2Names.add(a2[p].impl());
}

auto getOwn = [&](JSObject* obj, const PropertyName& name) -> JSValue {
PropertySlot slot(obj, PropertySlot::InternalMethodType::GetOwnProperty, nullptr);
bool has = obj->methodTable()->getOwnPropertySlot(obj, globalObject, name, slot);
if (scope.exception()) [[unlikely]]
return {};
if (!has) return jsUndefined();
return slot.getValue(globalObject, name);
};

UncheckedKeyHashSet<UniquedStringImpl*> a1Names;
a1Names.reserveInitialCapacity(propertyArrayLength1);

// take a property name from one, try to get it from both
for (size_t i = 0; i < propertyLength; i++) {
Identifier i1 = a1[i];
for (size_t p = 0; p < propertyArrayLength1; p++) {
Identifier i1 = a1[p];
a1Names.add(i1.impl());
PropertyName propertyName1 = PropertyName(i1);

JSValue prop1 = o1->get(globalObject, propertyName1);
JSValue prop1 = getOwn(o1, propertyName1);
RETURN_IF_EXCEPTION(scope, false);

if (!prop1) [[unlikely]] {
if (!a2Names.contains(i1.impl())) {
if constexpr (!isStrict) {
if (prop1.isUndefined()) {
continue;
}
}
return false;
}

JSValue prop2 = o2->getIfPropertyExists(globalObject, propertyName1);
JSValue prop2 = getOwn(o2, propertyName1);
RETURN_IF_EXCEPTION(scope, false);

if constexpr (!isStrict) {
if (prop1.isUndefined() && prop2.isEmpty()) {
continue;
}
auto eql = Bun__deepEquals<isStrict, enableAsymmetricMatchers>(globalObject, prop1, prop2, gcBuffer, stack, scope, true);
RETURN_IF_EXCEPTION(scope, false);
if (!eql) return false;
}

// for enumerable own properties only on the second array, make sure they are undefined
for (size_t p = 0; p < propertyArrayLength2; p++) {
Identifier i2 = a2[p];
if (a1Names.contains(i2.impl())) {
continue;
}
Comment thread
robobun marked this conversation as resolved.

if (!prop2) {
if constexpr (isStrict) {
return false;
}

auto eql = Bun__deepEquals<isStrict, enableAsymmetricMatchers>(globalObject, prop1, prop2, gcBuffer, stack, scope, true);
JSValue prop2 = getOwn(o2, PropertyName(i2));
RETURN_IF_EXCEPTION(scope, false);
if (!eql) return false;
}

RETURN_IF_EXCEPTION(scope, false);
if (!prop2.isUndefined()) {
return false;
}
}

return true;
}
Expand All @@ -849,6 +882,9 @@ bool Bun__deepEquals(JSC::JSGlobalObject* globalObject, JSValue v1, JSValue v2,
if (!equal(JSObject::calculatedClassName(o1), JSObject::calculatedClassName(o2))) {
return false;
}
if (o1->getPrototypeDirect().isNull() != o2->getPrototypeDirect().isNull()) {
return false;
}
}

JSC::Structure* o1Structure = o1->structure();
Expand Down Expand Up @@ -981,10 +1017,13 @@ bool Bun__deepEquals(JSC::JSGlobalObject* globalObject, JSValue v1, JSValue v2,
}
}

UncheckedKeyHashSet<UniquedStringImpl*> a1Names;
a1Names.reserveInitialCapacity(propertyArrayLength1);

// take a property name from one, try to get it from both
size_t i;
for (i = 0; i < propertyArrayLength1; i++) {
for (size_t i = 0; i < propertyArrayLength1; i++) {
Identifier i1 = a1[i];
a1Names.add(i1.impl());
PropertyName propertyName1 = PropertyName(i1);

JSValue prop1 = o1->get(globalObject, propertyName1);
Expand Down Expand Up @@ -1012,12 +1051,14 @@ bool Bun__deepEquals(JSC::JSGlobalObject* globalObject, JSValue v1, JSValue v2,
if (!eql) return false;
}

// for the remaining properties in the other object, make sure they are undefined
for (; i < propertyArrayLength2; i++) {
// for properties only on the second object, make sure they are undefined
for (size_t i = 0; i < propertyArrayLength2; i++) {
Identifier i2 = a2[i];
PropertyName propertyName2 = PropertyName(i2);
if (a1Names.contains(i2.impl())) {
continue;
}

JSValue prop2 = o2->getIfPropertyExists(globalObject, propertyName2);
JSValue prop2 = o2->getIfPropertyExists(globalObject, PropertyName(i2));
RETURN_IF_EXCEPTION(scope, false);

if (!prop2.isUndefined()) {
Expand Down Expand Up @@ -1281,10 +1322,13 @@ std::optional<bool> specialObjectsDequal(JSC::JSGlobalObject* globalObject, Mark
}
}

UncheckedKeyHashSet<UniquedStringImpl*> a1Names;
a1Names.reserveInitialCapacity(propertyArrayLength1);

// take a property name from one, try to get it from both
size_t i;
for (i = 0; i < propertyArrayLength1; i++) {
for (size_t i = 0; i < propertyArrayLength1; i++) {
Identifier i1 = a1[i];
a1Names.add(i1.impl());
if (i1 == vm.propertyNames->stack) continue;
PropertyName propertyName1 = PropertyName(i1);

Expand Down Expand Up @@ -1312,9 +1356,10 @@ std::optional<bool> specialObjectsDequal(JSC::JSGlobalObject* globalObject, Mark
}
}

// for the remaining properties in the other object, make sure they are undefined
for (; i < propertyArrayLength2; i++) {
// for properties only on the second error, make sure they are undefined
for (size_t i = 0; i < propertyArrayLength2; i++) {
Identifier i2 = a2[i];
if (a1Names.contains(i2.impl())) continue;
if (i2 == vm.propertyNames->stack) continue;
PropertyName propertyName2 = PropertyName(i2);

Expand Down Expand Up @@ -1375,16 +1420,24 @@ std::optional<bool> specialObjectsDequal(JSC::JSGlobalObject* globalObject, Mark
if (vector == rightVector) [[unlikely]]
return true;

// For Float32Array and Float64Array, when not in strict mode, we need to
// handle +0 and -0 as equal, and NaN as not equal to itself.
// jest's toEqual compares float elements with Object.is (NaN==NaN, -0!=+0);
// node's assert.deepEqual uses ==, which says the opposite for both. The strict
// path below memcmps, matching both toStrictEqual and isDeepStrictEqual.
if (!isStrict && (c1Type == Float16ArrayType || c1Type == Float32ArrayType || c1Type == Float64ArrayType)) {
auto elementsEqual = [](double a, double b) -> bool {
if constexpr (enableAsymmetricMatchers) {
if (std::isnan(a)) return std::isnan(b);
if (a == 0.0 && b == 0.0) return std::signbit(a) == std::signbit(b);
}
return a == b;
};
if (c1Type == Float16ArrayType) {
auto* leftFloat = static_cast<const WTF::Float16*>(vector);
auto* rightFloat = static_cast<const WTF::Float16*>(rightVector);
size_t numElements = byteLength / sizeof(WTF::Float16);

for (size_t i = 0; i < numElements; i++) {
if (leftFloat[i] != rightFloat[i]) {
if (!elementsEqual(static_cast<double>(leftFloat[i]), static_cast<double>(rightFloat[i]))) {
return false;
}
}
Expand All @@ -1395,7 +1448,7 @@ std::optional<bool> specialObjectsDequal(JSC::JSGlobalObject* globalObject, Mark
size_t numElements = byteLength / sizeof(float);

for (size_t i = 0; i < numElements; i++) {
if (leftFloat[i] != rightFloat[i]) {
if (!elementsEqual(leftFloat[i], rightFloat[i])) {
return false;
}
}
Expand All @@ -1406,7 +1459,7 @@ std::optional<bool> specialObjectsDequal(JSC::JSGlobalObject* globalObject, Mark
size_t numElements = byteLength / sizeof(double);

for (size_t i = 0; i < numElements; i++) {
if (leftDouble[i] != rightDouble[i]) {
if (!elementsEqual(leftDouble[i], rightDouble[i])) {
return false;
}
}
Expand All @@ -1425,14 +1478,16 @@ std::optional<bool> specialObjectsDequal(JSC::JSGlobalObject* globalObject, Mark
return false;
}

JSString* s1 = c1->toStringInline(globalObject);
RETURN_IF_EXCEPTION(scope, {});
JSString* s2 = c2->toStringInline(globalObject);
RETURN_IF_EXCEPTION(scope, {});
// Read the boxed [[StringData]] directly rather than toString(), which
// would invoke a user-defined toString/Symbol.toPrimitive.
JSString* s1 = uncheckedDowncast<StringObject>(c1)->internalValue();
JSString* s2 = uncheckedDowncast<StringObject>(c2)->internalValue();

bool stringsEqual = s1->equal(globalObject, s2);
RETURN_IF_EXCEPTION(scope, {});
return stringsEqual;
if (!stringsEqual) return false;
// Fall through to check own properties
break;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment thread
claude[bot] marked this conversation as resolved.
}
case JSFunctionType: {
return false;
Expand Down
Loading
Loading