Skip to content

Avoid unnecessary allocation in stripANSI for false-positive escape bytes #28772

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

Closed
robobun wants to merge 1 commit into from
Closed

Avoid unnecessary allocation in stripANSI for false-positive escape bytes #28772

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
84 changes: 56 additions & 28 deletions src/jsc/bindings/stripANSI.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,24 @@ static std::optional<WTF::String> stripANSI(const std::span<const Char> input)
return std::nullopt;
}

auto start = input.data();
const auto end = start + input.size();

// Lazy flat-buffer allocation: don't touch the buffer until we find an
// escape. For no-escape input we return std::nullopt and the caller
// reuses the original JSString with zero copies.
const auto* const end = input.data() + input.size();

// Lazy flat-buffer allocation: don't touch the buffer until we confirm a
// real ANSI escape sequence (not just a broad-mask candidate). `searchPos`
// tracks the search position; `start` tracks the copy origin. They only
// diverge while we're skipping pre-allocation false positives — once
// allocated they move together. findEscapeCharacter scans only bytes past
// searchPos, so total work stays O(input.size()).
Vector<Char> buffer;
Char* cursor = nullptr;
bool foundANSI = false;
auto* start = input.data();
auto* searchPos = start;

while (start != end) {
const auto* escPos = ANSI::findEscapeCharacter(start, end);
while (searchPos != end) {
const auto* escPos = ANSI::findEscapeCharacter(searchPos, end);
if (!escPos) {
// No more escapes.
if (!foundANSI)
// No more escape candidates.
if (cursor == nullptr)
return std::nullopt;
// Copy the rest of the string.
const auto remaining = static_cast<size_t>(end - start);
Expand All @@ -39,37 +42,55 @@ static std::optional<WTF::String> stripANSI(const std::span<const Char> input)
break;
}

// Lazily allocate the worst-case buffer on first ESC candidate. Guard
// on `cursor == nullptr` (not `!foundANSI`) so a broad-mask false
// positive that allocates the buffer doesn't reset the cursor on the
// next iteration when a real escape is finally found.
const auto* newPos = ANSI::consumeANSI(escPos, end);
if (newPos == escPos) {
// Broad-mask false positive (e.g. standalone 0x9C).
if (cursor == nullptr) {
// Pre-allocation: skip the byte without committing to a copy.
// `start` stays put so the byte is included in the prefix copy
// if a later real escape forces allocation.
searchPos = escPos + 1;
continue;
}
// Post-allocation: flush chunk and copy the byte literally.
if (escPos > start) {
const auto chunkLen = static_cast<size_t>(escPos - start);
memcpy(cursor, start, chunkLen * sizeof(Char));
cursor += chunkLen;
}
*cursor++ = *escPos;
start = escPos + 1;
searchPos = start;
continue;
}

// Real ANSI sequence — allocate worst-case buffer on first one.
// POD types skip per-element initialization in Vector::grow.
if (cursor == nullptr) {
buffer.grow(input.size());
cursor = buffer.begin();
}

// Copy everything before the escape sequence.
// Copy everything before the escape (preserves any false-positive
// bytes skipped pre-allocation).
if (escPos > start) {
const auto chunkLen = static_cast<size_t>(escPos - start);
memcpy(cursor, start, chunkLen * sizeof(Char));
cursor += chunkLen;
}

const auto* newPos = ANSI::consumeANSI(escPos, end);
if (newPos == escPos) {
// Broad-mask false positive — copy the byte literally.
*cursor++ = *escPos;
start = escPos + 1;
continue;
}

ASSERT(newPos > start);
ASSERT(newPos > escPos);
ASSERT(newPos <= end);
foundANSI = true;
start = newPos;
searchPos = newPos;
}

// Loop exited via `searchPos == end` without ever allocating — only
// false-positive bytes were found (e.g. "hello\x9C"). Return nullopt so
// the caller reuses the original JSString with zero heap allocations.
if (cursor == nullptr)
return std::nullopt;

const size_t reserved = buffer.size();
const size_t outputLen = static_cast<size_t>(cursor - buffer.begin());
const size_t waste = reserved - outputLen;
Expand Down Expand Up @@ -104,7 +125,9 @@ extern "C" bool Bun__ANSI__next(BunANSIIterator* it)
if (escPos != start) break;
const auto after = ANSI::consumeANSI(start, end);
if (after == start) {
start++;
// Broad-mask false positive (e.g. standalone 0x9C) — not a real
// escape sequence. Break without advancing so the byte falls into
// the next content slice instead of being silently dropped.
break;
}
start = after;
Expand All @@ -117,7 +140,12 @@ extern "C" bool Bun__ANSI__next(BunANSIIterator* it)
return false;
}

const auto escPos = ANSI::findEscapeCharacter(start, end);
auto escPos = ANSI::findEscapeCharacter(start, end);
// If the escape candidate is at `start`, it's a false-positive from the
// skip-loop (e.g. standalone 0x9C — not a real ANSI sequence). Include it
// in the content by scanning from start + 1 for the actual next escape.
if (escPos == start)
escPos = (start + 1 < end) ? ANSI::findEscapeCharacter(start + 1, end) : nullptr;
Comment on lines +143 to +148

@claude claude Bot May 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The behavioral changes to Bun__ANSI__next (preserving standalone 0x9C bytes via break without advancing, plus the escPos == start re-scan to maintain liveness) have no test coverage — the new test only exercises Bun.stripANSI, not the C iterator. Given that an earlier iteration of this exact fix introduced an infinite loop caught only by manual review, it would be worth adding a test that drives ANSIIterator over input containing standalone 0x9C and asserts (a) termination and (b) that the yielded slices concatenate back to the original input.

Extended reasoning...

What the gap is

This PR makes two non-trivial behavioral changes to Bun__ANSI__next in src/jsc/bindings/stripANSI.cpp:

  1. At line ~128, start++; break; becomes break; so that broad-mask false-positive bytes (standalone 0x9C) are no longer silently dropped.
  2. At lines 143–148, an escPos == start guard is added that re-scans from start + 1 so the iterator does not infinite-loop on the byte it just declined to advance past.

Neither change is exercised by any test. grep confirms that ANSIIterator, Bun__ANSI__next, and copyToClipboardOSC52 are referenced only in src/string/immutable.zig, src/jsc/bindings/stripANSI.cpp, and src/cli/repl.zig — there are zero hits in test/. The only new test in this PR (stripANSI.test.ts:17–28) calls Bun.stripANSI, which goes through the templated stripANSI<Char> function — a completely separate code path from the C iterator.

Why this matters here specifically

The PR timeline shows that this exact code went through a broken intermediate state during this review cycle: the first attempt at change (1) — removing start++ without adding the re-scan — introduced an infinite loop where Bun__ANSI__next would forever return true with a zero-length slice at an unchanging cursor, hanging copyToClipboardOSC52. That regression was caught only by a manual review comment, not by a failing test. The fix (the re-scan at lines 143–148) is itself subtle enough that a separate nit was filed about it fragmenting slices for consecutive 0x9C bytes. Code that has already regressed once within a single PR cycle is a strong signal that it needs a regression test.

Why existing coverage does not help

The Bun.stripANSI test suite is comprehensive for the templated path, but Bun__ANSI__next shares only findEscapeCharacter/consumeANSI with it — the loop structure, cursor bookkeeping, and slice-boundary logic are entirely independent. A bug in the iterator's liveness or byte-preservation would not be detected by any existing test; the infinite-loop regression in this PR is direct proof of that.

Step-by-step: what a regression test would have caught

For input "\x9C" against the intermediate (broken) commit:

  1. Call 1, cursor=0: skip-loop finds escPos == start, consumeANSI returns start, break without advancing. Post-loop findEscapeCharacter(start, end) returns start again → slice_len = 0, cursor = 0, returns true.
  2. Call 2, cursor=0: identical state → identical result. The Zig while (it.next()) |slice| loop in copyToClipboardOSC52 never terminates.

A test that simply iterates strings.ANSIIterator over "a\x9Cb" and asserts the concatenated slices equal "a\x9Cb" would have (a) hung on the broken commit and (b) failed on the pre-PR code (which yielded "a" ++ "b", dropping the byte).

Suggested fix

Add a Zig test (e.g. in src/string/immutable.zig alongside ANSIIterator, or a small case in the bindings tests) that iterates over inputs like "\x9C", "a\x9Cb", and "\x9C\x9C", concatenates the yielded slices, and asserts the result equals the input. This pins down both the termination guarantee and the byte-preservation guarantee that this PR establishes.

This is nit severity — the current implementation traces correctly by hand and all verifiers agree it is sound; this is purely a coverage gap, not a correctness bug, and need not block the PR.

const auto slice_end = escPos ? escPos : end;

it->slice_ptr = start;
Expand Down
13 changes: 13 additions & 0 deletions test/js/bun/util/stripANSI.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,19 @@ describe("Bun.stripANSI", () => {
expect(heapStats().objectTypeCounts.string).toBe(numStrings);
});

test("returns same string object when input ends with false-positive escape byte", () => {
// Standalone C1 ST (0x9C) is in the SIMD broad-mask but is not a valid
// ANSI sequence introducer — consumeANSI treats it as a false positive.
// The pre-allocation false-positive skip path must advance past the byte
// without allocating, so the original JSString is reused.
const input = "hello\x9C";
const stripANSI = Bun.stripANSI;
const numStrings = heapStats().objectTypeCounts.string;
const result = stripANSI(input);
expect(result).toBe(input);
expect(heapStats().objectTypeCounts.string).toBe(numStrings);
});
Comment thread
robobun marked this conversation as resolved.

test("returns new string when ANSI sequences are removed", () => {
const input = "\x1b[31mhello\x1b[0m world";
const result = Bun.stripANSI(input);
Expand Down
Loading