-
-
Notifications
You must be signed in to change notification settings - Fork 286
fix(utils): Prevent RangeError from large command output #433
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,8 +69,70 @@ | |
|
|
||
| const childProcess = spawn(executable, args, spawnOpts); | ||
|
|
||
| let stdout = ''; | ||
| let stderr = ''; | ||
| // Accumulate child process output as raw Buffers (not concatenated strings) | ||
| // so we never trip V8's max-string-length limit (~512MB on 64-bit), which | ||
| // previously surfaced as an uncaught `RangeError: Invalid string length` | ||
| // thrown synchronously from the 'data' handler when xcodebuild emitted | ||
| // very large verbose logs. | ||
|
Check warning on line 76 in src/utils/command.ts
|
||
| const DEFAULT_MAX_OUTPUT_BYTES = 64 * 1024 * 1024; // 64 MiB per stream | ||
| const envCap = Number.parseInt(process.env.XCODEBUILDMCP_MAX_OUTPUT_BYTES ?? '', 10); | ||
|
Check warning on line 78 in src/utils/command.ts
|
||
| const maxOutputBytes = | ||
| opts?.maxOutputBytes ?? | ||
| (Number.isFinite(envCap) && envCap > 0 ? envCap : DEFAULT_MAX_OUTPUT_BYTES); | ||
|
|
||
| const stdoutChunks: Buffer[] = []; | ||
| const stderrChunks: Buffer[] = []; | ||
| let stdoutBytes = 0; | ||
| let stderrBytes = 0; | ||
| let stdoutTruncated = false; | ||
| let stderrTruncated = false; | ||
|
|
||
| const appendChunk = ( | ||
| chunks: Buffer[], | ||
| currentBytes: number, | ||
| truncated: boolean, | ||
| data: Buffer, | ||
| ): { bytes: number; truncated: boolean } => { | ||
| if (truncated) { | ||
| return { bytes: currentBytes, truncated: true }; | ||
| } | ||
| const remaining = maxOutputBytes - currentBytes; | ||
| if (data.byteLength <= remaining) { | ||
| chunks.push(data); | ||
| return { bytes: currentBytes + data.byteLength, truncated: false }; | ||
| } | ||
| if (remaining > 0) { | ||
| chunks.push(data.subarray(0, remaining)); | ||
| } | ||
| return { bytes: maxOutputBytes, truncated: true }; | ||
| }; | ||
|
|
||
| const finalizeStream = ( | ||
| chunks: Buffer[], | ||
| totalBytes: number, | ||
| truncated: boolean, | ||
| ): string => { | ||
| try { | ||
| const text = Buffer.concat(chunks, totalBytes).toString('utf8'); | ||
| return truncated | ||
| ? `${text}\n[output truncated after ${totalBytes} bytes]` | ||
| : text; | ||
| } catch (err) { | ||
| // Defensive: if the concatenated string still somehow exceeds V8's | ||
| // string limit, fall back to a heavily truncated slice rather than | ||
| // crashing the MCP process. | ||
| log( | ||
| 'error', | ||
| `Failed to finalize captured output (${totalBytes} bytes): ${(err as Error).message}`, | ||
| ); | ||
| const safeSlice = Math.min(totalBytes, 1 * 1024 * 1024); | ||
| const safeText = Buffer.concat(chunks, totalBytes) | ||
| .subarray(0, safeSlice) | ||
| .toString('utf8'); | ||
| return `${safeText} | ||
| [output truncated after ${safeSlice} bytes due to size]`; | ||
| } | ||
| }; | ||
|
|
||
| const streamClosers: Array<() => void> = []; | ||
| const streamDetachers: Array<() => void> = []; | ||
|
|
@@ -114,6 +176,8 @@ | |
| detachStreamListeners(); | ||
|
|
||
| const success = code === 0; | ||
| const stdout = finalizeStream(stdoutChunks, stdoutBytes, stdoutTruncated); | ||
| const stderr = finalizeStream(stderrChunks, stderrBytes, stderrTruncated); | ||
| const response: CommandResponse = { | ||
| success, | ||
| output: stdout, | ||
|
|
@@ -143,7 +207,7 @@ | |
|
|
||
| const attachStream = ( | ||
| stream: NodeJS.ReadableStream | null | undefined, | ||
| onChunk: (chunk: string) => void, | ||
| onChunk: (chunk: Buffer) => void, | ||
| ): void => { | ||
| if (!stream) { | ||
| return; | ||
|
|
@@ -162,11 +226,19 @@ | |
| }; | ||
|
|
||
| const handleData = (data: Buffer | string): void => { | ||
| if (settled) { | ||
|
Check warning on line 229 in src/utils/command.ts
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No tests for the new buffer accumulation, capping, and truncation logic The new Evidence
Also found at 1 additional location
Identified by Warden code-review · UJ6-YUH |
||
| return; | ||
| } | ||
| const chunk = data.toString(); | ||
| onChunk(chunk); | ||
| try { | ||
| const buf = Buffer.isBuffer(data) ? data : Buffer.from(data); | ||
| onChunk(buf); | ||
| } catch (err) { | ||
| // Any failure inside the data handler (including a future | ||
| // RangeError) must reject the promise rather than escape as an | ||
| // uncaught exception (which previously crashed the MCP process via | ||
| // mechanism=auto.node.onuncaughtexception). | ||
| handleError(err as Error); | ||
| } | ||
| }; | ||
|
|
||
| stream.on('data', handleData); | ||
|
|
@@ -212,25 +284,47 @@ | |
| } | ||
|
|
||
| attachStream(childProcess.stdout, (chunk) => { | ||
| stdout += chunk; | ||
| opts?.onStdout?.(chunk); | ||
| emitTranscript?.({ | ||
| kind: 'transcript', | ||
| fragment: 'process-line', | ||
| stream: 'stdout', | ||
| line: chunk, | ||
| }); | ||
| const result = appendChunk(stdoutChunks, stdoutBytes, stdoutTruncated, chunk); | ||
| stdoutBytes = result.bytes; | ||
| if (!stdoutTruncated && result.truncated) { | ||
| log( | ||
| 'warning', | ||
| `stdout exceeded maxOutputBytes (${maxOutputBytes}); truncating further output`, | ||
| ); | ||
| } | ||
| stdoutTruncated = result.truncated; | ||
| if (opts?.onStdout || emitTranscript) { | ||
| const text = chunk.toString('utf8'); | ||
| opts?.onStdout?.(text); | ||
| emitTranscript?.({ | ||
| kind: 'transcript', | ||
| fragment: 'process-line', | ||
| stream: 'stdout', | ||
| line: text, | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| attachStream(childProcess.stderr, (chunk) => { | ||
| stderr += chunk; | ||
| opts?.onStderr?.(chunk); | ||
| emitTranscript?.({ | ||
| kind: 'transcript', | ||
| fragment: 'process-line', | ||
| stream: 'stderr', | ||
| line: chunk, | ||
| }); | ||
| const result = appendChunk(stderrChunks, stderrBytes, stderrTruncated, chunk); | ||
| stderrBytes = result.bytes; | ||
| if (!stderrTruncated && result.truncated) { | ||
| log( | ||
| 'warning', | ||
| `stderr exceeded maxOutputBytes (${maxOutputBytes}); truncating further output`, | ||
| ); | ||
| } | ||
| stderrTruncated = result.truncated; | ||
| if (opts?.onStderr || emitTranscript) { | ||
| const text = chunk.toString('utf8'); | ||
| opts?.onStderr?.(text); | ||
| emitTranscript?.({ | ||
| kind: 'transcript', | ||
| fragment: 'process-line', | ||
| stream: 'stderr', | ||
| line: text, | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| childProcess.once('error', handleError); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Catch block in finalizeStream can itself throw unhandled
Medium Severity
The defensive
catchblock infinalizeStreamrepeatsBuffer.concat(chunks, totalBytes)without its owntry/catch. If the original failure wasBuffer.concatitself throwing (e.g.,ERR_MEMORY_ALLOCATION_FAILEDwhenmaxOutputBytesis configured very large via the env var), this second call will also throw. SincefinalizeStreamis called fromsettle(), which is invoked from event handlers andsetTimeoutcallbacks, the unhandled throw becomes an uncaught exception — the exact crash scenario this PR aims to prevent.Additional Locations (1)
src/utils/command.ts#L169-L190Reviewed by Cursor Bugbot for commit 6155c40. Configure here.