From b4588fd2f795c4a50b01f170f2a6b36af1b99a85 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 3 Sep 2026 15:39:35 -0700 Subject: [PATCH 1/3] Remove unused timeout id reuse and emit U+FFFD for malformed UTF-16 DispatchImpl still accepted an id after #220, but Dispatch always passed 0. If a future caller reused an existing id, unordered_map::insert would keep the old Timeout and m_timeMap would gain a second entry pointing at it; Clear() erases only one. Always allocate a fresh id. The UTF-16 decoder dropped a trailing odd byte and passed unpaired surrogates through to Napi::String. The Encoding Standard replacement mode requires U+FFFD for both, in either endianness. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: da74bc94-a7dc-4817-bd81-59b5c6b123fc --- .../Scheduling/Source/TimeoutDispatcher.cpp | 20 +++--- .../Scheduling/Source/TimeoutDispatcher.h | 2 - Polyfills/TextDecoder/README.md | 3 +- Polyfills/TextDecoder/Source/TextDecoder.cpp | 63 ++++++++++++++++--- Tests/UnitTests/Scripts/tests.ts | 22 +++++++ 5 files changed, 88 insertions(+), 22 deletions(-) diff --git a/Polyfills/Scheduling/Source/TimeoutDispatcher.cpp b/Polyfills/Scheduling/Source/TimeoutDispatcher.cpp index 1dea6585..0cbbe1a9 100644 --- a/Polyfills/Scheduling/Source/TimeoutDispatcher.cpp +++ b/Polyfills/Scheduling/Source/TimeoutDispatcher.cpp @@ -63,11 +63,6 @@ namespace Babylon::Polyfills::Internal } TimeoutDispatcher::TimeoutId TimeoutDispatcher::Dispatch(std::shared_ptr function, std::chrono::milliseconds delay, bool repeat) - { - return DispatchImpl(function, delay, repeat, 0); - } - - TimeoutDispatcher::TimeoutId TimeoutDispatcher::DispatchImpl(std::shared_ptr function, std::chrono::milliseconds delay, bool repeat, TimeoutId id) { if (delay.count() < 0) { @@ -76,14 +71,17 @@ namespace Babylon::Polyfills::Internal std::unique_lock lk{m_mutex}; - if (id == 0) - { - id = NextTimeoutId(); - } + // Always a fresh id: re-arming a repeating timeout no longer goes through + // Dispatch, so there is no caller that reuses an existing id. Passing one + // into unordered_map::insert would keep the old Timeout and then add a + // second m_timeMap entry pointing at it; Clear() erases only one. + const auto id = NextTimeoutId(); const auto earliestTime = m_timeMap.empty() ? TimePoint::max() : m_timeMap.cbegin()->second->time; const auto time = Now() + delay; - const auto result = m_idMap.insert({id, std::make_unique(id, ++m_lastSequence, std::move(function), time, repeat ? std::make_optional(delay) : std::nullopt)}); - m_timeMap.insert({time, result.first->second.get()}); + auto timeout = std::make_unique(id, ++m_lastSequence, std::move(function), time, repeat ? std::make_optional(delay) : std::nullopt); + Timeout* const raw = timeout.get(); + m_idMap.emplace(id, std::move(timeout)); + m_timeMap.insert({time, raw}); if (time <= earliestTime) { diff --git a/Polyfills/Scheduling/Source/TimeoutDispatcher.h b/Polyfills/Scheduling/Source/TimeoutDispatcher.h index 0ab4b135..78004203 100644 --- a/Polyfills/Scheduling/Source/TimeoutDispatcher.h +++ b/Polyfills/Scheduling/Source/TimeoutDispatcher.h @@ -28,8 +28,6 @@ namespace Babylon::Polyfills::Internal private: using TimePoint = std::chrono::time_point; - TimeoutId DispatchImpl(std::shared_ptr function, std::chrono::milliseconds delay, bool repeat, TimeoutId id); - TimeoutId NextTimeoutId(); void ThreadFunction(); void CallFunction(TimeoutId id, uint64_t sequence); diff --git a/Polyfills/TextDecoder/README.md b/Polyfills/TextDecoder/README.md index bd34b5b3..d9f009ab 100644 --- a/Polyfills/TextDecoder/README.md +++ b/Polyfills/TextDecoder/README.md @@ -11,6 +11,7 @@ A C++ implementation of the [WHATWG Encoding API](https://encoding.spec.whatwg.o - Constructing `TextDecoder` with no argument (defaults to `utf-8`). - Constructing `TextDecoder` with any WHATWG label for UTF-8 (`"utf-8"`, `"utf8"`, `"unicode-1-1-utf-8"`, `"unicode11utf8"`, `"unicode20utf8"`, `"x-unicode20utf8"`), UTF-16LE (`"utf-16"`, `"utf-16le"`, `"ucs-2"`, `"unicode"`, `"unicodeFEFF"`, `"csunicode"`, `"iso-10646-ucs-2"`) or UTF-16BE (`"utf-16be"`, `"unicodeFFFE"`). Labels are matched case-insensitively and ignore surrounding whitespace. - Stripping a leading byte order mark when decoding UTF-16. +- Replacing malformed UTF-16 (a trailing odd byte, or an unpaired surrogate) with U+FFFD, for both endiannesses. - Calling `decode()` with no argument or `undefined` returns an empty string (matches the Web API). ### Not Supported @@ -18,7 +19,7 @@ A C++ implementation of the [WHATWG Encoding API](https://encoding.spec.whatwg.o - Encodings other than UTF-8 and UTF-16 — passing any other label (e.g. `"iso-8859-1"`) throws a JavaScript `Error`. - `DataView` is not accepted by `decode()` — due to missing `Napi::DataView` support in the underlying JSI layer. - Passing a non-BufferSource value (e.g. a string or number) to `decode()` throws a `TypeError`. -- The `fatal` option: decoding errors are not detected and do not throw a `TypeError`. A trailing odd byte in a UTF-16 sequence is dropped rather than decoded as U+FFFD. +- The `fatal` option: decoding errors are not detected and do not throw a `TypeError`. Malformed UTF-16 (a trailing odd byte, or an unpaired surrogate) is replaced with U+FFFD. - The `ignoreBOM` option: a leading UTF-16 byte order mark is always stripped and cannot be retained. A UTF-8 byte order mark is never stripped. - Streaming decode (passing `{ stream: true }` to `decode()`) — each call is stateless. - The `encoding` property on the `TextDecoder` instance is not exposed. diff --git a/Polyfills/TextDecoder/Source/TextDecoder.cpp b/Polyfills/TextDecoder/Source/TextDecoder.cpp index 640e5d25..2b9a7a45 100644 --- a/Polyfills/TextDecoder/Source/TextDecoder.cpp +++ b/Polyfills/TextDecoder/Source/TextDecoder.cpp @@ -113,17 +113,64 @@ namespace Napi::Value DecodeUtf16(Napi::Env env, const std::string& data) const { - // Trailing odd byte is dropped: the WHATWG decoder would emit U+FFFD for it, but - // every producer we care about hands over whole code units. - const size_t unitCount = data.size() / 2; - std::u16string units(unitCount, u'\0'); - for (size_t index = 0; index < unitCount; ++index) + // WHATWG UTF-16 decoder in replacement mode: unpaired surrogates and a + // leftover odd byte become U+FFFD. A lead surrogate followed by a + // non-trail is one replacement, then the second unit is reprocessed. + const bool littleEndian = m_encoding == Encoding::Utf16LittleEndian; + std::u16string units; + units.reserve(data.size() / 2 + 1); + + constexpr char16_t LEAD_MIN = 0xD800; + constexpr char16_t LEAD_MAX = 0xDBFF; + constexpr char16_t TRAIL_MIN = 0xDC00; + constexpr char16_t TRAIL_MAX = 0xDFFF; + constexpr char16_t REPLACEMENT = 0xFFFD; + + bool pendingLead = false; + char16_t lead = 0; + size_t byteIndex = 0; + while (byteIndex + 1 < data.size()) { - const auto first = static_cast(data[index * 2]); - const auto second = static_cast(data[index * 2 + 1]); - units[index] = m_encoding == Encoding::Utf16LittleEndian + const auto first = static_cast(data[byteIndex]); + const auto second = static_cast(data[byteIndex + 1]); + byteIndex += 2; + const auto unit = littleEndian ? static_cast(first | (second << 8)) : static_cast(second | (first << 8)); + + if (pendingLead) + { + pendingLead = false; + if (unit >= TRAIL_MIN && unit <= TRAIL_MAX) + { + units.push_back(lead); + units.push_back(unit); + continue; + } + + units.push_back(REPLACEMENT); + // Fall through and reprocess `unit` as a standalone code unit. + } + + if (unit >= LEAD_MIN && unit <= LEAD_MAX) + { + pendingLead = true; + lead = unit; + } + else if (unit >= TRAIL_MIN && unit <= TRAIL_MAX) + { + units.push_back(REPLACEMENT); + } + else + { + units.push_back(unit); + } + } + + // End-of-queue: an unpaired lead and/or leftover odd byte is one error. + if (pendingLead || byteIndex < data.size()) + { + units.push_back(REPLACEMENT); } if (!units.empty() && units.front() == u'\uFEFF') diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index bb484ba7..c1795116 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -1851,6 +1851,28 @@ describe("TextDecoder", function () { expect(result).to.equal("\u{1F600}\0A"); expect(result.length).to.equal(4); }); + + it("should replace a trailing odd UTF-16 byte with U+FFFD", function () { + expect(new TextDecoder("utf-16le").decode(new Uint8Array([0x48, 0x00, 0x00]))).to.equal("H\uFFFD"); + expect(new TextDecoder("utf-16be").decode(new Uint8Array([0x00, 0x48, 0x00]))).to.equal("H\uFFFD"); + expect(new TextDecoder("utf-16le").decode(new Uint8Array([0x00]))).to.equal("\uFFFD"); + expect(new TextDecoder("utf-16be").decode(new Uint8Array([0x00]))).to.equal("\uFFFD"); + }); + + it("should replace unpaired UTF-16 surrogates with U+FFFD", function () { + // Lone lead U+D800. + expect(new TextDecoder("utf-16le").decode(new Uint8Array([0x00, 0xD8]))).to.equal("\uFFFD"); + expect(new TextDecoder("utf-16be").decode(new Uint8Array([0xD8, 0x00]))).to.equal("\uFFFD"); + // Lone trail U+DC00. + expect(new TextDecoder("utf-16le").decode(new Uint8Array([0x00, 0xDC]))).to.equal("\uFFFD"); + expect(new TextDecoder("utf-16be").decode(new Uint8Array([0xDC, 0x00]))).to.equal("\uFFFD"); + // Lead followed by BMP 'A': replacement, then reprocess 'A'. + expect(new TextDecoder("utf-16le").decode(new Uint8Array([0x00, 0xD8, 0x41, 0x00]))).to.equal("\uFFFDA"); + expect(new TextDecoder("utf-16be").decode(new Uint8Array([0xD8, 0x00, 0x00, 0x41]))).to.equal("\uFFFDA"); + // Unpaired lead plus leftover odd byte is a single end-of-queue replacement. + expect(new TextDecoder("utf-16le").decode(new Uint8Array([0x00, 0xD8, 0x00]))).to.equal("\uFFFD"); + expect(new TextDecoder("utf-16be").decode(new Uint8Array([0xD8, 0x00, 0x00]))).to.equal("\uFFFD"); + }); }); describe("TextEncoder", function () { From 05db7725873517e5d8771e363ee21e8aa6cb695c Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 3 Sep 2026 15:50:47 -0700 Subject: [PATCH 2/3] Reword TextDecoder fatal-option README bullet The previous wording said errors were "not detected" while also describing U+FFFD replacement. fatal still does not throw; replacement is the supported behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: da74bc94-a7dc-4817-bd81-59b5c6b123fc --- Polyfills/TextDecoder/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Polyfills/TextDecoder/README.md b/Polyfills/TextDecoder/README.md index d9f009ab..2b274809 100644 --- a/Polyfills/TextDecoder/README.md +++ b/Polyfills/TextDecoder/README.md @@ -19,7 +19,7 @@ A C++ implementation of the [WHATWG Encoding API](https://encoding.spec.whatwg.o - Encodings other than UTF-8 and UTF-16 — passing any other label (e.g. `"iso-8859-1"`) throws a JavaScript `Error`. - `DataView` is not accepted by `decode()` — due to missing `Napi::DataView` support in the underlying JSI layer. - Passing a non-BufferSource value (e.g. a string or number) to `decode()` throws a `TypeError`. -- The `fatal` option: decoding errors are not detected and do not throw a `TypeError`. Malformed UTF-16 (a trailing odd byte, or an unpaired surrogate) is replaced with U+FFFD. +- The `fatal` option: decoding errors do not throw a `TypeError`. Malformed UTF-16 (a trailing odd byte, or an unpaired surrogate) is replaced with U+FFFD instead. - The `ignoreBOM` option: a leading UTF-16 byte order mark is always stripped and cannot be retained. A UTF-8 byte order mark is never stripped. - Streaming decode (passing `{ stream: true }` to `decode()`) — each call is stateless. - The `encoding` property on the `TextDecoder` instance is not exposed. From 205707d769fd59792b0fb7f4de79af3d393ec16e Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 3 Sep 2026 16:25:35 -0700 Subject: [PATCH 3/3] Guard TimeoutDispatcher against duplicate-id emplace Use try_emplace and only insert into m_timeMap from the owned map entry so a colliding id cannot destroy the Timeout and leave a dangling pointer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: da74bc94-a7dc-4817-bd81-59b5c6b123fc --- Polyfills/Scheduling/Source/TimeoutDispatcher.cpp | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/Polyfills/Scheduling/Source/TimeoutDispatcher.cpp b/Polyfills/Scheduling/Source/TimeoutDispatcher.cpp index 0cbbe1a9..c198a4f4 100644 --- a/Polyfills/Scheduling/Source/TimeoutDispatcher.cpp +++ b/Polyfills/Scheduling/Source/TimeoutDispatcher.cpp @@ -2,6 +2,7 @@ #include #include +#include namespace Babylon::Polyfills::Internal { @@ -79,9 +80,15 @@ namespace Babylon::Polyfills::Internal const auto earliestTime = m_timeMap.empty() ? TimePoint::max() : m_timeMap.cbegin()->second->time; const auto time = Now() + delay; auto timeout = std::make_unique(id, ++m_lastSequence, std::move(function), time, repeat ? std::make_optional(delay) : std::nullopt); - Timeout* const raw = timeout.get(); - m_idMap.emplace(id, std::move(timeout)); - m_timeMap.insert({time, raw}); + // try_emplace does not consume timeout if the id is already present, unlike + // emplace, so a failed insert cannot destroy the Timeout and leave a + // dangling m_timeMap pointer. NextTimeoutId should already skip live ids. + const auto [it, inserted] = m_idMap.try_emplace(id, std::move(timeout)); + if (!inserted) + { + throw std::logic_error{"TimeoutDispatcher: NextTimeoutId returned a duplicate id"}; + } + m_timeMap.insert({time, it->second.get()}); if (time <= earliestTime) {