Skip to content

Conversation

@kixelated
Copy link
Collaborator

@kixelated kixelated commented Nov 17, 2025

The capacity check was comparing against the buffer's total byteLength without accounting for byteOffset. For Uint8Array views over a larger buffer, this could incorrectly allow writes that exceed the available space.

Use dst.byteOffset + dst.byteLength + size > dst.buffer.byteLength to properly check the actual end position of the appended data.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Fixed buffer boundary validation to properly account for typed array offsets, preventing write operations from exceeding allocated memory boundaries.

The capacity check was comparing against the buffer's total byteLength
without accounting for byteOffset. For Uint8Array views over a larger
buffer, this could incorrectly allow writes that exceed the available space.

Use dst.byteOffset + dst.byteLength + size > dst.buffer.byteLength to
properly check the actual end position of the appended data.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@kixelated kixelated enabled auto-merge (squash) November 17, 2025 21:45
@coderabbitai
Copy link

coderabbitai bot commented Nov 17, 2025

Walkthrough

The change modifies the destination buffer space validation in VarInt.encode method. Previously, the check compared the underlying buffer's total byte length against the required space without accounting for the buffer's starting offset. The updated check incorporates the byteOffset property, calculating available space by comparing dst.byteOffset + dst.byteLength + size against dst.buffer.byteLength. This adjustment ensures validation correctly accounts for TypedArray views that don't start at offset zero within their underlying ArrayBuffer. The error handling and write operations remain unchanged.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and accurately describes the main change: fixing a buffer capacity check in varint encode to account for byteOffset, which matches the primary objective of the pull request.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-guest

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 62fd026 and a9943da.

📒 Files selected for processing (1)
  • web-transport-ws/src/varint.ts (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build
🔇 Additional comments (1)
web-transport-ws/src/varint.ts (1)

31-31: Critical fix correctly prevents buffer overflow with offset views.

The updated check properly accounts for byteOffset when validating buffer capacity. The previous logic would incorrectly allow writes beyond the buffer bounds when dst is a view over a larger ArrayBuffer starting at a non-zero offset.

Example scenario where the old check failed:

  • ArrayBuffer(100) with Uint8Array at offset 50, length 40, and size = 20
  • Old: 100 < 60 → false (no error, but writes to position 110!)
  • New: 50 + 40 + 20 > 100 → true (correctly throws error)

The fix aligns with the DataView creation on line 35 and the return statement on line 49, both of which use dst.byteOffset.

Consider adding a test case that verifies this behavior with a Uint8Array view that has a non-zero byteOffset:

// Test case to verify the fix
const buffer = new ArrayBuffer(100);
const view = new Uint8Array(buffer, 50, 40); // offset 50, length 40
const varint = new VarInt(BigInt(1 << 20)); // requires 4 bytes
// Should throw when trying to encode beyond available space

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@kixelated kixelated merged commit 9239033 into main Nov 17, 2025
1 check passed
@kixelated kixelated deleted the fix-guest branch November 17, 2025 21:51
@github-actions github-actions bot mentioned this pull request Nov 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants