test(ext/node): regression test for empty buffer TLS write panic#34412
Merged
Conversation
Writing an empty (zero-length) Uint8Array through node:tls's TLSWrap used to panic with `called Option::unwrap() on a None value` at `ext/node/ops/tls_wrap.rs` (the `ab.data().unwrap()` in `writev`). A zero-length ArrayBuffer has a null backing-store data pointer, so `ArrayBuffer::data()` returns `None`. This is the path hit by `@deno/sandbox`, which connects via `npm:ws`; the `ws` Sender batches frame writes with `socket.cork()` + `socket.write(header)` + `socket.write(data)` + `socket.uncork()`, and control/empty frames write `Buffer.alloc(0)`, producing a `writev` with a zero-length chunk. The write ops were fixed in denoland#33737 to skip chunks whose `data()` is `None`. The existing `tls_write_detached_buffer` spec test covers the detached-buffer trigger via direct handle calls; this adds coverage for the empty/zero-length buffer trigger through both direct handle calls and the natural `socket.write()`/cork path that the sandbox uses. Verified the test reproduces the original panic (same `Option::unwrap() on a None value`, same column) when the guard is reverted, and passes with the guard in place. Closes denoland/orchid#241 Co-Authored-By: Divy Srivastava <me@littledivy.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the Deno Sandbox panic reported in #34404 by adding regression
coverage for the underlying crash.
Running:
panicked with:
Root cause
@deno/sandboxconnects vianpm:ws. ThewsSender batches framewrites through the socket with
cork()+write(header)+write(data)+uncork(), and control / empty frames writeBuffer.alloc(0). That produces a node:tlswritevcontaining azero-length chunk.
A zero-length
ArrayBufferhas a null backing-store data pointer, sov8::ArrayBuffer::data()returnsNone. Thewritevop used to doab.data().unwrap(), panicking on thatNone.This was fixed in #33737, which changed the TLSWrap write ops to skip
chunks whose
data()isNone. The existingtests/specs/node/tls_write_detached_bufferspec test covers thedetached-buffer trigger via direct handle calls. This PR adds
coverage for the empty / zero-length buffer trigger — the one
@deno/sandboxactually hits — through both direct handle calls and thenatural
socket.write()+cork()/uncork()path.Verification
data()guard reverted to the oldab.data().unwrap(), thenew test reproduces the original panic (
called Option::unwrap() on a None value, same column:31).main), the test passes (ok).No runtime code change —
mainalready contains the fix; this locks itin against regression for the reported scenario.
Closes denoland/orchid#241