Skip to content

Fix Upgrader.Upgrade never reusing hijacked write buffer#1011

Open
veeceey wants to merge 1 commit into
gorilla:mainfrom
veeceey:fix/server-available-buffer-cap-and-typos
Open

Fix Upgrader.Upgrade never reusing hijacked write buffer#1011
veeceey wants to merge 1 commit into
gorilla:mainfrom
veeceey:fix/server-available-buffer-cap-and-typos

Conversation

@veeceey
Copy link
Copy Markdown

@veeceey veeceey commented Feb 10, 2026

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update
  • Go Version Update
  • Dependency Update

Description

bufio.Writer.AvailableBuffer() returns a slice with len == 0 but cap equal to the available buffer space:

// From bufio package:
func (b *Writer) AvailableBuffer() []byte {
    return b.buf[b.n:][:0]
}

The existing code in Upgrader.Upgrade checked len(buf) which was always 0, making the write buffer reuse path dead code:

buf := brw.Writer.AvailableBuffer()
// len(buf) is always 0 here, so this condition is never true:
if u.WriteBufferPool == nil && u.WriteBufferSize == 0 && len(buf) >= maxFrameHeaderSize+256 {
    writeBuf = buf  // never reached
}

This caused an unnecessary memory allocation on every Upgrade call even when the hijacked write buffer was large enough to be reused.

Changes

  1. server.go - Fix buffer capacity check: Use cap(buf) instead of len(buf) to check the hijacked buffer size, and extend the slice to its full capacity with buf[:cap(buf)] so downstream code using len(c.writeBuf) works correctly.

  2. server.go - Fix response header buffer selection: Use cap(p) instead of len(p) when comparing the hijacked buffer against the connection write buffer for constructing the response header. Without this fix, len(p) is always 0, so the comparison len(c.writeBuf) > len(p) always selects the connection buffer even when the hijacked buffer is larger.

  3. conn.go / server.go - Fix typos: "effor" -> "effort" (2 occurrences in conn.go), "buferred" -> "buffered" (1 occurrence in server.go).

  4. server_test.go - Add TestWriteBufferReuse: Validates that large enough hijacked buffers are reused and small buffers are not. This test fails on the old code and passes with the fix.

Related Tickets & Documents

Added/updated tests?

  • Yes

Run verifications and test

  • go vet ./... is passing
  • go test ./... is passing

bufio.Writer.AvailableBuffer() returns a slice with len==0 but
cap equal to the available buffer space. The existing code checked
len(buf) which was always 0, so the buffer reuse path was dead code.

This caused an unnecessary allocation on every Upgrade when the
hijacked write buffer was large enough to be reused.

Changes:
- Use cap(buf) instead of len(buf) to check hijacked buffer size
- Extend buf to full capacity with buf[:cap(buf)] before passing
  as writeBuf so downstream code using len(c.writeBuf) works
- Use cap(p) instead of len(p) when selecting larger buffer for
  the response header construction
- Fix typos: "effor" -> "effort" (2x in conn.go),
  "buferred" -> "buffered" (1x in server.go)
- Add TestWriteBufferReuse to verify the buffer reuse behavior

Fixes gorilla#973
@veeceey
Copy link
Copy Markdown
Author

veeceey commented Feb 19, 2026

Friendly ping - any chance someone could take a look at this when they get a chance? Happy to make any changes if needed.

@veeceey
Copy link
Copy Markdown
Author

veeceey commented Mar 10, 2026

hey, circling back on this. would love to get it merged if it looks good to you

@veeceey veeceey force-pushed the fix/server-available-buffer-cap-and-typos branch from 8d94a4a to 9a5e229 Compare March 12, 2026 04:29
scalecode-solutions referenced this pull request in scalecode-solutions/magilla Apr 24, 2026
Foundation commit for the personal fork. No behavior change beyond
what the Go toolchain and std-lib bumps imply.

Module & toolchain
  * Rewrite module path to github.com/scalecode-solutions/websocket
    across go.mod, README, examples, and import statements.
  * Bump language version from 1.20 -> 1.26 (matches local toolchain).
  * Bump golang.org/x/net from v0.26.0 -> v0.53.0, closing the path
    flagged by upstream issue #991 (HTTP/2 CVEs in older x/net).

Hygiene (small, mechanical)
  * interface{} -> any across compression.go, conn.go, conn_test.go,
    and json.go (upstream PR #1000). BufferPool interface signature
    changes are source-compatible since any is an alias.
  * Fix "best effor" -> "best effort" and "buferred" -> "buffered"
    typos in conn.go and server.go (upstream PR #975, carried in
    PR #1011).
  * Add the missing blank line before the Deprecated: marker on
    UnderlyingConn so godoc/linters treat it as deprecated
    (upstream PR #998).
  * Extend the DialContext godoc to note that non-101 responses
    produce an error (upstream PR #965).

Drop pre-1.21 compat cruft
  * Delete mask_safe.go and the !appengine build tags. Classic
    App Engine required a sandboxed Go runtime without unsafe; it
    was retired in 2020 and modern App Engine uses the standard
    toolchain.
  * Drop legacy `// +build` lines (Go 1.18+ only needs //go:build).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
scalecode-solutions referenced this pull request in scalecode-solutions/magilla Apr 24, 2026
bufio.Writer.AvailableBuffer() returns a slice with len==0 and cap
equal to the buffer's remaining capacity, so the previous len(buf)
check was always false and the "reuse hijacked buffer" path was
dead. Switch both call sites to cap(buf) and extend the slice to
its full capacity before handing it to newConn. Eliminates an
allocation per upgraded connection on the default pool-less path.

Upstream PRs #1011 and #972.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] fix: len(buf) => cap(buf)

1 participant