Skip to content

node:fs: convert Stats times by the value, not by the class #47233

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
+270 −83
Open

node:fs: convert Stats times by the value, not by the class #47233

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
99 changes: 44 additions & 55 deletions src/jsc/bindings/NodeFSStatBinding.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include <JavaScriptCore/PropertyNameArray.h>
#include "ZigGlobalObject.h"
#include "ObjectBindings.h"
#include "Bindgen/IDLConvert.h"
#include "JavaScriptCore/DateInstance.h"
#if ENABLE(AOT)
#include <JavaScriptCore/AOTPropertyWalk.h>
Expand Down Expand Up @@ -215,7 +216,15 @@ static const Identifier& identifier(JSC::VM& vm, DateFieldType dateField)
ASSERT_NOT_REACHED();
}

template<DateFieldType field, bool isBigInt>
// Both class structures keep the four `*Ms` fields at the same offsets.
static ALWAYS_INLINE bool hasStatsStructure(Zig::GlobalObject* globalObject, JSC::JSObject* object)
{
JSC::Structure* structure = object->structure();
return structure == globalObject->m_JSStatsClassStructure.getConcurrently()
|| structure == globalObject->m_JSStatsBigIntClassStructure.getConcurrently();
}

template<DateFieldType field>
inline JSC::JSValue getDateField(JSC::JSGlobalObject* globalObject, JSC::EncodedJSValue thisValue, JSC::PropertyName propertyName)
{
auto& vm = globalObject->vm();
Expand All @@ -226,16 +235,23 @@ inline JSC::JSValue getDateField(JSC::JSGlobalObject* globalObject, JSC::Encoded
return JSC::jsUndefined();

JSValue value;
if (thisObject->structureID() == getStructure<isBigInt>(defaultGlobalObject(globalObject))->id()) {
if (hasStatsStructure(defaultGlobalObject(globalObject), thisObject)) {
value = thisObject->getDirect(static_cast<int>(field));
ASSERT(thisObject->getDirectOffset(vm, identifier(vm, field)) == static_cast<int>(field));
} else {
value = thisObject->get(globalObject, identifier(vm, field));
RETURN_IF_EXCEPTION(scope, {});
}

double internalNumber = isBigInt ? value.toBigInt64(globalObject) : value.toNumber(globalObject);
RETURN_IF_EXCEPTION(scope, {});
// Node.js: `Number(ms)` for both classes, then Math.round, which Bun leaves out. https://github.com/nodejs/node/blob/v26.3.0/lib/internal/fs/utils.js#L459-L469
double internalNumber;
if (auto exact = JSBigInt::tryExtractDouble(value)) [[likely]] {
internalNumber = *exact;
} else {
auto ctx = DefaultConversionContext {};
internalNumber = convertIDL<Bindgen::IDLLooseDoubleOrBigInt>(*globalObject, value, ctx);
RETURN_IF_EXCEPTION(scope, {});
}

JSValue result = JSC::DateInstance::create(vm, globalObject->dateStructure(), internalNumber);
Comment thread
robobun marked this conversation as resolved.
Resolved
if (!thisObject->structure()->mayBePrototype()) {
Expand All @@ -247,38 +263,19 @@ inline JSC::JSValue getDateField(JSC::JSGlobalObject* globalObject, JSC::Encoded

JSC_DEFINE_CUSTOM_GETTER(jsStatsPrototypeGetter_atime, (JSGlobalObject * globalObject, JSC::EncodedJSValue thisValue, JSC::PropertyName propertyName))
{
return JSValue::encode(getDateField<DateFieldType::atime, false>(globalObject, thisValue, propertyName));
return JSValue::encode(getDateField<DateFieldType::atime>(globalObject, thisValue, propertyName));
}
JSC_DEFINE_CUSTOM_GETTER(jsStatsPrototypeGetter_mtime, (JSGlobalObject * globalObject, JSC::EncodedJSValue thisValue, JSC::PropertyName propertyName))
{
return JSValue::encode(getDateField<DateFieldType::mtime, false>(globalObject, thisValue, propertyName));
return JSValue::encode(getDateField<DateFieldType::mtime>(globalObject, thisValue, propertyName));
}
JSC_DEFINE_CUSTOM_GETTER(jsStatsPrototypeGetter_ctime, (JSGlobalObject * globalObject, JSC::EncodedJSValue thisValue, JSC::PropertyName propertyName))
{
return JSValue::encode(getDateField<DateFieldType::ctime, false>(globalObject, thisValue, propertyName));
return JSValue::encode(getDateField<DateFieldType::ctime>(globalObject, thisValue, propertyName));
}
JSC_DEFINE_CUSTOM_GETTER(jsStatsPrototypeGetter_birthtime, (JSGlobalObject * globalObject, JSC::EncodedJSValue thisValue, JSC::PropertyName propertyName))
{
return JSValue::encode(getDateField<DateFieldType::birthtime, false>(globalObject, thisValue, propertyName));
}

JSC_DEFINE_CUSTOM_GETTER(jsBigIntStatsPrototypeGetter_birthtime, (JSGlobalObject * globalObject, JSC::EncodedJSValue thisValue, JSC::PropertyName propertyName))
{
return JSValue::encode(getDateField<DateFieldType::birthtime, true>(globalObject, thisValue, propertyName));
}
JSC_DEFINE_CUSTOM_GETTER(jsBigIntStatsPrototypeGetter_ctime, (JSGlobalObject * globalObject, JSC::EncodedJSValue thisValue, JSC::PropertyName propertyName))
{
return JSValue::encode(getDateField<DateFieldType::ctime, true>(globalObject, thisValue, propertyName));
}

JSC_DEFINE_CUSTOM_GETTER(jsBigIntStatsPrototypeGetter_mtime, (JSGlobalObject * globalObject, JSC::EncodedJSValue thisValue, JSC::PropertyName propertyName))
{
return JSValue::encode(getDateField<DateFieldType::mtime, true>(globalObject, thisValue, propertyName));
}

JSC_DEFINE_CUSTOM_GETTER(jsBigIntStatsPrototypeGetter_atime, (JSGlobalObject * globalObject, JSC::EncodedJSValue thisValue, JSC::PropertyName propertyName))
{
return JSValue::encode(getDateField<DateFieldType::atime, true>(globalObject, thisValue, propertyName));
return JSValue::encode(getDateField<DateFieldType::birthtime>(globalObject, thisValue, propertyName));
}

JSC_DEFINE_CUSTOM_SETTER(jsStatsPrototypeFunction_DatePutter, (JSGlobalObject * globalObject, JSC::EncodedJSValue thisValue, JSC::EncodedJSValue encodedValue, JSC::PropertyName propertyName))
Expand Down Expand Up @@ -375,10 +372,10 @@ static const HashTableValue JSBigIntStatsPrototypeTableValues[] = {
{ "isFile"_s, static_cast<unsigned>(JSC::PropertyAttribute::Function), NoIntrinsic, { HashTableValue::NativeFunctionType, jsBigIntStatsPrototypeFunction_isFile, 0 } },
{ "isSocket"_s, static_cast<unsigned>(JSC::PropertyAttribute::Function), NoIntrinsic, { HashTableValue::NativeFunctionType, jsBigIntStatsPrototypeFunction_isSocket, 0 } },
{ "isSymbolicLink"_s, static_cast<unsigned>(JSC::PropertyAttribute::Function), NoIntrinsic, { HashTableValue::NativeFunctionType, jsBigIntStatsPrototypeFunction_isSymbolicLink, 0 } },
{ "atime"_s, static_cast<unsigned>(JSC::PropertyAttribute::CustomAccessor), NoIntrinsic, { HashTableValue::GetterSetterType, jsBigIntStatsPrototypeGetter_atime, jsStatsPrototypeFunction_DatePutter } },
{ "mtime"_s, static_cast<unsigned>(JSC::PropertyAttribute::CustomAccessor), NoIntrinsic, { HashTableValue::GetterSetterType, jsBigIntStatsPrototypeGetter_mtime, jsStatsPrototypeFunction_DatePutter } },
{ "ctime"_s, static_cast<unsigned>(JSC::PropertyAttribute::CustomAccessor), NoIntrinsic, { HashTableValue::GetterSetterType, jsBigIntStatsPrototypeGetter_ctime, jsStatsPrototypeFunction_DatePutter } },
{ "birthtime"_s, static_cast<unsigned>(JSC::PropertyAttribute::CustomAccessor), NoIntrinsic, { HashTableValue::GetterSetterType, jsBigIntStatsPrototypeGetter_birthtime, jsStatsPrototypeFunction_DatePutter } },
{ "atime"_s, static_cast<unsigned>(JSC::PropertyAttribute::CustomAccessor), NoIntrinsic, { HashTableValue::GetterSetterType, jsStatsPrototypeGetter_atime, jsStatsPrototypeFunction_DatePutter } },
{ "mtime"_s, static_cast<unsigned>(JSC::PropertyAttribute::CustomAccessor), NoIntrinsic, { HashTableValue::GetterSetterType, jsStatsPrototypeGetter_mtime, jsStatsPrototypeFunction_DatePutter } },
{ "ctime"_s, static_cast<unsigned>(JSC::PropertyAttribute::CustomAccessor), NoIntrinsic, { HashTableValue::GetterSetterType, jsStatsPrototypeGetter_ctime, jsStatsPrototypeFunction_DatePutter } },
{ "birthtime"_s, static_cast<unsigned>(JSC::PropertyAttribute::CustomAccessor), NoIntrinsic, { HashTableValue::GetterSetterType, jsStatsPrototypeGetter_birthtime, jsStatsPrototypeFunction_DatePutter } },
};

class JSStatsPrototype final : public JSC::JSNonFinalObject {
Expand Down Expand Up @@ -757,6 +754,20 @@ const JSC::ClassInfo JSBigIntStatsPrototype::s_info = { "BigIntStats"_s, &Base::
const JSC::ClassInfo JSStatsConstructor::s_info = { "Stats"_s, &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(JSStatsConstructor) };
const JSC::ClassInfo JSBigIntStatsConstructor::s_info = { "BigIntStats"_s, &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(JSBigIntStatsConstructor) };

// Node.js divides with the JS `/` operator: https://github.com/nodejs/node/blob/v26.3.0/lib/internal/fs/utils.js#L624-L627
static void divideNsIntoMs(JSC::JSGlobalObject* globalObject, JSValue& atime, JSValue& mtime, JSValue& ctime, JSValue& birthtime)
{
auto& vm = globalObject->vm();
auto scope = DECLARE_THROW_SCOPE(vm);

JSValue nsPerMs = JSC::JSBigInt::createFrom(globalObject, kNsPerMs);
RETURN_IF_EXCEPTION(scope, );
for (JSValue* time : { &atime, &mtime, &ctime, &birthtime }) {
*time = jsDiv(globalObject, *time, nsPerMs);
RETURN_IF_EXCEPTION(scope, );
}
}

template<bool isBigInt>
inline JSValue callJSStatsFunction(JSC::JSGlobalObject* globalObject, JSC::CallFrame* callFrame)
{
Expand Down Expand Up @@ -793,18 +804,7 @@ inline JSValue callJSStatsFunction(JSC::JSGlobalObject* globalObject, JSC::CallF
JSValue birthtimeMs = birthtimeNs;

if constexpr (isBigInt) {
// this.atimeMs = atimeNs / kNsPerMsBigInt;
// this.mtimeMs = mtimeNs / kNsPerMsBigInt;
// this.ctimeMs = ctimeNs / kNsPerMsBigInt;
// this.birthtimeMs = birthtimeNs / kNsPerMsBigInt;
const double kNsPerMsBigInt = 1000000;
atimeMs = jsDoubleNumber(atimeNs.toBigInt64(globalObject) / kNsPerMsBigInt);
RETURN_IF_EXCEPTION(scope, {});
mtimeMs = jsDoubleNumber(mtimeNs.toBigInt64(globalObject) / kNsPerMsBigInt);
RETURN_IF_EXCEPTION(scope, {});
ctimeMs = jsDoubleNumber(ctimeNs.toBigInt64(globalObject) / kNsPerMsBigInt);
RETURN_IF_EXCEPTION(scope, {});
birthtimeMs = jsDoubleNumber(birthtimeNs.toBigInt64(globalObject) / kNsPerMsBigInt);
divideNsIntoMs(globalObject, atimeMs, mtimeMs, ctimeMs, birthtimeMs);
RETURN_IF_EXCEPTION(scope, {});
}

Expand Down Expand Up @@ -876,18 +876,7 @@ inline JSValue constructJSStatsObject(JSC::JSGlobalObject* lexicalGlobalObject,
JSValue birthtimeMs = birthtimeNs;

if constexpr (isBigInt) {
// this.atimeMs = atimeNs / kNsPerMsBigInt;
// this.mtimeMs = mtimeNs / kNsPerMsBigInt;
// this.ctimeMs = ctimeNs / kNsPerMsBigInt;
// this.birthtimeMs = birthtimeNs / kNsPerMsBigInt;
const double kNsPerMsBigInt = 1000000;
atimeMs = jsDoubleNumber(atimeNs.toBigInt64(globalObject) / kNsPerMsBigInt);
RETURN_IF_EXCEPTION(scope, {});
mtimeMs = jsDoubleNumber(mtimeNs.toBigInt64(globalObject) / kNsPerMsBigInt);
RETURN_IF_EXCEPTION(scope, {});
ctimeMs = jsDoubleNumber(ctimeNs.toBigInt64(globalObject) / kNsPerMsBigInt);
RETURN_IF_EXCEPTION(scope, {});
birthtimeMs = jsDoubleNumber(birthtimeNs.toBigInt64(globalObject) / kNsPerMsBigInt);
divideNsIntoMs(globalObject, atimeMs, mtimeMs, ctimeMs, birthtimeMs);
RETURN_IF_EXCEPTION(scope, {});
}

Expand Down
202 changes: 202 additions & 0 deletions test/js/node/fs/fs-stats-constructor.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,208 @@ test("Stats instances share Stats.prototype", () => {
expect(bigint instanceof Object.getPrototypeOf(bigint).constructor).toBe(true);
});

// Node.js computes the four `*Ms` fields of BigIntStats in JS: `this.atimeMs = atimeNs / 1_000_000n`.
// That is BigInt division: the result is a BigInt, truncated toward zero, with no 64-bit limit, and
// an operand that does not convert to a BigInt throws a TypeError. The native constructor used to
// divide `BigInt.asIntN(64, atimeNs)` as a double and store a Number, which the date getters rejected.
// https://github.com/nodejs/node/blob/v26.3.0/lib/internal/fs/utils.js#L619-L632
describe("BigIntStats constructor", () => {
const BigIntStats = Object.getPrototypeOf(statSync(import.meta.path, { bigint: true })).constructor;
class Subclass extends BigIntStats {}
// BigIntStats(dev, mode, nlink, uid, gid, rdev, blksize, ino, size, blocks, atimeNs, mtimeNs, ctimeNs, birthtimeNs)
const fields = [0n, 1n, 2n, 3n, 4n, 5n, 6n, 7n, 8n, 9n];
const constructors: [string, (...args: unknown[]) => any][] = [
["new BigIntStats(...)", (...args) => new BigIntStats(...args)],
["new (class extends BigIntStats)(...)", (...args) => new Subclass(...args)],
// Node.js throws for a call without new. Bun accepts it, for Stats too.
["BigIntStats(...) without new (Bun only)", (...args) => BigIntStats(...args)],
];

test.each(constructors)("%s divides the *Ns arguments into BigInt *Ms fields", (_, construct) => {
const stats = construct(...fields, 2_500_000n, 3_999_999n, -2_500_000n, -1n);
expect({ ...stats }).toEqual({
dev: 0n,
mode: 1n,
nlink: 2n,
uid: 3n,
gid: 4n,
rdev: 5n,
blksize: 6n,
ino: 7n,
size: 8n,
blocks: 9n,
atimeMs: 2n,
mtimeMs: 3n,
ctimeMs: -2n,
birthtimeMs: 0n,
atimeNs: 2_500_000n,
mtimeNs: 3_999_999n,
ctimeNs: -2_500_000n,
birthtimeNs: -1n,
});
expect({
atime: stats.atime,
mtime: stats.mtime,
ctime: stats.ctime,
birthtime: stats.birthtime,
}).toEqual({
atime: new Date(2),
mtime: new Date(3),
ctime: new Date(-2),
birthtime: new Date(0),
});
});

test.each(constructors)("%s keeps the precision of *Ns arguments outside the int64 range", (_, construct) => {
const stats = construct(...fields, 2n ** 70n, -(2n ** 70n), 2n ** 63n, -(2n ** 63n) - 1n);
expect({
atimeMs: stats.atimeMs,
mtimeMs: stats.mtimeMs,
ctimeMs: stats.ctimeMs,
birthtimeMs: stats.birthtimeMs,
}).toEqual({
atimeMs: 1_180_591_620_717_411n,
mtimeMs: -1_180_591_620_717_411n,
ctimeMs: 9_223_372_036_854n,
birthtimeMs: -9_223_372_036_854n,
});
expect(stats.atime).toEqual(new Date(1_180_591_620_717_411));

// A *Ms value outside the int64 range is outside the Date range too. The getter must not wrap it into a valid date.
const wide = construct(...fields, 2n ** 64n * 1_000_000n, -(2n ** 64n) * 1_000_000n, 0n, 0n);
expect({ atimeMs: wide.atimeMs, mtimeMs: wide.mtimeMs }).toEqual({ atimeMs: 2n ** 64n, mtimeMs: -(2n ** 64n) });
expect({ atime: wide.atime.getTime(), mtime: wide.mtime.getTime() }).toEqual({ atime: NaN, mtime: NaN });
});

test.each(constructors)("%s throws a TypeError if a *Ns argument does not convert to a BigInt", (_, construct) => {
for (const notBigInt of [2_500_000, "2500000", true, null, undefined, { valueOf: () => 2_500_000 }]) {
for (let i = 0; i < 4; i++) {
const times: unknown[] = [0n, 0n, 0n, 0n];
times[i] = notBigInt;
expect(() => construct(...fields, ...times)).toThrow(TypeError);
}
}
});

test.each(constructors)("%s converts object *Ns arguments with valueOf, once each, in order", (_, construct) => {
const order: string[] = [];
const argument = (name: string, value: unknown) => ({
valueOf() {
order.push(name);
return value;
},
});
const atimeNs = argument("atimeNs", 1_999_999n);
const mtimeNs = argument("mtimeNs", 2_000_000n);
const ctimeNs = argument("ctimeNs", 3_000_000n);
const birthtimeNs = argument("birthtimeNs", 4_000_000n);
const stats = construct(...fields, atimeNs, mtimeNs, ctimeNs, birthtimeNs);
expect(order).toEqual(["atimeNs", "mtimeNs", "ctimeNs", "birthtimeNs"]);
expect({
atimeMs: stats.atimeMs,
mtimeMs: stats.mtimeMs,
ctimeMs: stats.ctimeMs,
birthtimeMs: stats.birthtimeMs,
}).toEqual({ atimeMs: 1n, mtimeMs: 2n, ctimeMs: 3n, birthtimeMs: 4n });
// The *Ns fields hold the arguments as given.
expect(stats.atimeNs).toBe(atimeNs);
expect(stats.mtimeNs).toBe(mtimeNs);
expect(stats.ctimeNs).toBe(ctimeNs);
expect(stats.birthtimeNs).toBe(birthtimeNs);

// The first argument that does not convert to a BigInt ends the conversion.
order.length = 0;
const notBigInt = argument("mtimeNs", 2_000_000);
expect(() => construct(...fields, atimeNs, notBigInt, ctimeNs, birthtimeNs)).toThrow(TypeError);
expect(order).toEqual(["atimeNs", "mtimeNs"]);

// A BigInt wrapper object converts too.
expect(construct(...fields, Object(2_500_000n), 0n, 0n, 0n).atimeMs).toBe(2n);
});
});

// Node.js installs one set of date getters on both prototypes. Each converts `*Ms` with `Number(ms)`,
// so the type of the value decides the conversion and the class of the object does not. The native
// getters used to pick ToNumber for Stats and a ToBigInt cut to 64 bits for BigIntStats.
// https://github.com/nodejs/node/blob/v26.3.0/lib/internal/fs/utils.js#L459-L517
describe("Stats date getters convert *Ms by its type", () => {
const stat = {
Stats: () => statSync(import.meta.path),
BigIntStats: () => statSync(import.meta.path, { bigint: true }),
};
const classes = Object.keys(stat) as (keyof typeof stat)[];

test("a BigInt *Ms outside the int64 range is an Invalid Date, not a wrapped value", () => {
const bigint = stat.BigIntStats();
bigint.atimeMs = 2n ** 64n + 5n;
bigint.mtimeMs = -(2n ** 63n) - 1n;
bigint.ctimeMs = 8_640_000_000_000_000n;
bigint.birthtimeMs = 8_640_000_000_000_001n;
expect([
bigint.atime.getTime(),
bigint.mtime.getTime(),
bigint.ctime.getTime(),
bigint.birthtime.getTime(),
]).toEqual([NaN, NaN, 8.64e15, NaN]);
});

test.each(classes)("%s converts *Ms like Number()", name => {
const values = {
bigint: 5n,
number: 6,
string: "7",
boolean: true,
null: null,
undefined: undefined,
bigintFromValueOf: { valueOf: () => 9n },
numberFromValueOf: { valueOf: () => 10 },
bigintPastDoubleRange: 2n ** 1024n,
};
const times = Object.fromEntries(
Object.entries(values).map(([kind, value]) => {
// The getter caches the Date on the instance, so each value needs its own object.
const stats = stat[name]();
// @ts-expect-error Node.js converts a value of any type
stats.ctimeMs = value;
return [kind, stats.ctime.getTime()];
}),
);
const deleted = stat[name]();
// @ts-expect-error the field is not optional
delete deleted.ctimeMs;
expect({ ...times, deleted: deleted.ctime.getTime() }).toEqual({
bigint: 5,
number: 6,
string: 7,
boolean: 1,
null: 0,
undefined: NaN,
bigintFromValueOf: 9,
numberFromValueOf: 10,
bigintPastDoubleRange: NaN,
deleted: NaN,
});

const symbol = stat[name]();
// @ts-expect-error a Symbol does not convert to a number
symbol.ctimeMs = Symbol("x");
expect(() => symbol.ctime).toThrow(TypeError);
});

test("both classes share one getter and one setter for each date field", () => {
const bigintPrototype = Object.getPrototypeOf(stat.BigIntStats());
for (const field of ["atime", "mtime", "ctime", "birthtime"]) {
const onStats = Object.getOwnPropertyDescriptor(Stats.prototype, field)!;
const onBigIntStats = Object.getOwnPropertyDescriptor(bigintPrototype, field)!;
expect(onBigIntStats.get).toBe(onStats.get!);
expect(onBigIntStats.set).toBe(onStats.set!);
}
// The getter reads `<field>Ms` off any object it is called on.
const ctime = Object.getOwnPropertyDescriptor(bigintPrototype, "ctime")!.get!;
expect([ctime.call({ ctimeMs: 7n }).getTime(), ctime.call({ ctimeMs: 8 }).getTime()]).toEqual([7, 8]);
});
});

// A call resolved through a binding that a closure captures is compiled with the scope object
// itself in the this slot, and native functions see it raw. isFile() and friends used to read
// `mode` out of that scope object (the date getters, once pulled out of their property descriptor,
Expand Down
Loading
Loading