Skip to content

Commit 341f732

Browse files
pjfanningclaudechaokunyang
authored
fix(javascript): improve hps string writing (#4098)
## What does this PR do? Improves how the JavaScript hps string writer handles buffers: - `BinaryWriter` reserves space before using the hps string path. - The `@apache-fory/hps` wrapper validates the destination buffer. - The native writer respects the `Uint8Array` view offset. Adds tests in `test/hps.test.ts`. Note: the hps addon only builds on Node 20, and the JS CI matrix runs Node 18 and 24, so the hps tests are skipped in CI. Verified locally with `npm test` on Node 20 (391 tests passed). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Co-authored-by: chaokunyang <shawn.ck.yang@gmail.com>
1 parent db699c9 commit 341f732

5 files changed

Lines changed: 70 additions & 5 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1178,6 +1178,9 @@ jobs:
11781178
matrix:
11791179
node-version: [18, 24]
11801180
os: [ubuntu-latest, macos-latest, windows-latest]
1181+
include:
1182+
- node-version: 20
1183+
os: ubuntu-latest
11811184
runs-on: ${{ matrix.os }}
11821185
steps:
11831186
- uses: actions/checkout@v7.0.1

‎javascript/packages/core/lib/writer/index.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -296,6 +296,8 @@ export class BinaryWriter {
296296
}
297297

298298
stringWithHeaderFast(v: string) {
299+
// Worst case: 5-byte varint header plus UTF-16 body.
300+
this.reserve(5 + v.length * 2);
299301
const { serializeString } = this.config.hps!;
300302
this.cursor = serializeString(v, this.platformBuffer, this.cursor);
301303
}

‎javascript/packages/hps/index.ts‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,17 @@ const build = () => {
3131
if (typeof v !== "string") {
3232
throw new Error(`isLatin1 requires string but got ${typeof v}`);
3333
}
34-
// todo boundary check
34+
// The native callback converts offsets to uint32_t before unchecked writes.
35+
if (offset !== offset >>> 0) {
36+
throw new RangeError("serializeString offset must be an unsigned 32-bit integer");
37+
}
38+
// The native writer copies without bounds checks: a 5-byte varint header
39+
// plus at most 2 bytes per UTF-16 code unit must fit in `dist`.
40+
if (offset + 5 + v.length * 2 > dist.byteLength) {
41+
throw new RangeError(
42+
`serializeString needs up to ${5 + v.length * 2} bytes at offset ${offset} but buffer length is ${dist.byteLength}`,
43+
);
44+
}
3545
return _serializeString(dist, v, offset, 0);
3646
},
3747
};

‎javascript/packages/hps/src/fastcall.cc‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,8 +96,10 @@ static void serializeString(const v8::FunctionCallbackInfo<v8::Value> &args) {
9696
uint32_t offset = args[2].As<v8::Number>()->Uint32Value(context).ToChecked();
9797

9898
bool is_one_byte = str->IsOneByte();
99+
// A Uint8Array may be a view into a larger (e.g. pooled) ArrayBuffer.
99100
uint8_t *dst_data =
100-
reinterpret_cast<uint8_t *>(dst->Buffer()->GetBackingStore()->Data());
101+
reinterpret_cast<uint8_t *>(dst->Buffer()->GetBackingStore()->Data()) +
102+
dst->ByteOffset();
101103

102104
if (is_one_byte && str->IsExternalOneByte()) {
103105
offset += writeVarUint32(dst_data, offset,

‎javascript/test/hps.test.ts‎

Lines changed: 51 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,21 @@
1717
* under the License.
1818
*/
1919

20-
import { BinaryReader } from "../packages/core/index";
20+
import Fory, { BinaryReader } from "../packages/core/index";
2121
import hps from "../packages/hps/index";
22-
import { describe, expect, test } from "@jest/globals";
22+
import { beforeAll, describe, expect, test } from "@jest/globals";
2323

24-
const skipableDescribe = hps ? describe : describe.skip;
24+
const { engines } = require("../packages/hps/package.json");
25+
const skipableDescribe = require("semver").satisfies(process.version, engines.node)
26+
? describe
27+
: describe.skip;
2528

2629
skipableDescribe("hps", () => {
30+
beforeAll(() => {
31+
// A missing addon on a supported Node.js version must fail, not skip these tests.
32+
expect(hps).not.toBeNull();
33+
});
34+
2735
test("should isLatin1 work", () => {
2836
const { serializeString } = hps!;
2937
for (let index = 0; index < 10000; index++) {
@@ -39,4 +47,44 @@ skipableDescribe("hps", () => {
3947
expect(reader.stringWithHeader()).toBe("😁");
4048
}
4149
});
50+
51+
test("should reject strings exceeding buffer capacity", () => {
52+
const { serializeString } = hps!;
53+
const bf = Buffer.alloc(32);
54+
expect(() => serializeString("A".repeat(10000), bf, 0)).toThrow(RangeError);
55+
});
56+
57+
test.each([-1, -0.5, 0.5, NaN, Infinity, -Infinity, 2 ** 32])(
58+
"should reject invalid offset %s",
59+
(offset) => {
60+
const bf = Buffer.alloc(32, 0x7f);
61+
expect(() => hps!.serializeString("A", bf, offset)).toThrow(RangeError);
62+
expect(bf.every((b) => b === 0x7f)).toBe(true);
63+
},
64+
);
65+
66+
test.each(["hello", "\u4f60\u597d", "😁"])(
67+
"should write %s into a view with non-zero byteOffset",
68+
(value) => {
69+
const { serializeString } = hps!;
70+
// Exercise both alignments of the UTF-16 body in the native slow callback.
71+
for (const offset of [0, 1]) {
72+
const backing = Buffer.alloc(200);
73+
const view = backing.subarray(101, 190);
74+
const end = serializeString(value, view, offset);
75+
expect(backing.subarray(0, 101 + offset).every((b) => b === 0)).toBe(true);
76+
expect(backing.subarray(101 + end).every((b) => b === 0)).toBe(true);
77+
const reader = new BinaryReader({});
78+
reader.reset(view.subarray(offset, end));
79+
expect(reader.stringWithHeader()).toBe(value);
80+
}
81+
},
82+
);
83+
84+
test("should grow writer buffer for large strings", () => {
85+
const fory = new Fory({ hps });
86+
for (const value of ["A".repeat(200000), "\u4f60".repeat(200000)]) {
87+
expect(fory.deserialize(fory.serialize(value))).toBe(value);
88+
}
89+
});
4290
});

0 commit comments

Comments
 (0)