WebSocket: hand text frames to C++ as an owned BunString, delete EncodedSlice::utf16_global

Closed
RO
1mo ago
+70-58

Fixes #31968

Problem

  • EncodedSlice::utf16_global(&[u16]) (src/bun_core/string/mod.rs:1262 on main) was a safe fn that set the global ptr-tag on a borrowed slice. The tag tells C++ (Zig::toString) that it owns the bytes and must free them through the default allocator. Safe code could pass a Vec's storage and get an invalid free, then a double free when the Vec dropped.
  • Its only caller, the WebSocket client text-receive path (src/http_jsc/websocket_client.rs), held the contract with ManuallyDrop and a comment.

Fix

  • The text-receive path hands C++ an owned BunString by value. A transcoded Vec<u16> moves into an external WTF string through String::create_external_globally_allocated_utf16. ASCII text is copied with String::clone_latin1. WebSocket__didReceiveText consumes it with transferToWTFString, as WebSocket__didClose already does.
  • utf16_global is deleted. It was the only setter of bit 62, so is_globally_allocated, the two substring_with_len propagation branches, and the vacuous debug_assert in EncodedSliceJsc::to_js go with it. Rust can no longer create a global-tagged EncodedSlice at all.
  • Correct because ownership now moves in the type: BunString owns one WTF ref and C++ adopts it. transferToWTFString returns an empty string for the Empty, Dead, and OutOfMemory tags, the same as the old toString/toStringCopy fallbacks.
  • Verified: test/js/web/websocket/websocket.test.js (new test drives both branches with ASCII, short and long non-ASCII, and a 70 KB frame, then releases the strings and forces a GC). Also the rest of websocket.test.js, websocket-client.test.ts, websocket-utf16-headers.test.ts, parse-args.test.ts and xml.test.ts for the substring change, and clippy on the three crates.

Background

  • EncodedSlice<'a> is a {ptr, len} view with encoding bits in the pointer's high byte. Bit 62 meant "the consumer owns this allocation": an ownership claim that a borrowed &'a [u16] cannot back.
  • BunString is the FFI string union shared with C++. Passed by value across extern "C" it transfers its ref. transferToWTFString() is the C++ consumer that takes that ref.
  • The C++ readers of bit 62 (isTaggedExternalPtr in helpers.h, BunString.cpp:743) now always see it clear. They sit in the adopt path that #40404 rewrites, so they are left for that PR. free_global_string stays live through EncodedSlice__toExternalValue.
  • The new test cannot fail on the unfixed build. The fix deletes an API, and the compiler is the remaining guard. The test is coverage for the rewritten receive path.
Notes

The original PR targeted ZigString::from16, from16_slice, deinit_global, and From<&[u16]> for String. Those were removed on main in #35002 and #40374 (ZigString became EncodedSlice<'a>). This is the rework for the survivor of that family, in the shape the stale-review comment above asked for. A source-text lint test was dropped after the same shape was rejected on #31981.

Local baselines on a stock build of this main: websocket.test.js has 3 failures that need ws.postman-echo.com, and xml.test.ts "deep values are a catchable error" times out at 5 s under ASAN. Both fail identically with and without this diff.

CI on earlier heads: test-crypto-dh-leak.js fails the same ~23 MB RSS threshold on x64-asan across unrelated branches (builds 112252 through 112369) and is reported for main-break triage.