Skip to content

Commit 9a5e229

Browse files
committed
Fix Upgrader.Upgrade never reusing hijacked write buffer
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 #973
1 parent e064f32 commit 9a5e229

3 files changed

Lines changed: 74 additions & 6 deletions

File tree

conn.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -991,7 +991,7 @@ func (c *Conn) handleProtocolError(message string) error {
991991
if len(data) > maxControlFramePayloadSize {
992992
data = data[:maxControlFramePayloadSize]
993993
}
994-
// Make a best effor to send a close message describing the problem.
994+
// Make a best effort to send a close message describing the problem.
995995
_ = c.WriteControl(CloseMessage, data, time.Now().Add(writeWait))
996996
return errors.New("websocket: " + message)
997997
}
@@ -1147,7 +1147,7 @@ func (c *Conn) SetCloseHandler(h func(code int, text string) error) {
11471147
if h == nil {
11481148
h = func(code int, text string) error {
11491149
message := FormatCloseMessage(code, "")
1150-
// Make a best effor to send the close message.
1150+
// Make a best effort to send the close message.
11511151
_ = c.WriteControl(CloseMessage, message, time.Now().Add(writeWait))
11521152
return nil
11531153
}

server.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -205,9 +205,9 @@ func (u *Upgrader) Upgrade(w http.ResponseWriter, r *http.Request, responseHeade
205205
buf := brw.Writer.AvailableBuffer()
206206

207207
var writeBuf []byte
208-
if u.WriteBufferPool == nil && u.WriteBufferSize == 0 && len(buf) >= maxFrameHeaderSize+256 {
208+
if u.WriteBufferPool == nil && u.WriteBufferSize == 0 && cap(buf) >= maxFrameHeaderSize+256 {
209209
// Reuse hijacked write buffer as connection buffer.
210-
writeBuf = buf
210+
writeBuf = buf[:cap(buf)]
211211
}
212212

213213
c := newConn(netConn, true, u.ReadBufferSize, u.WriteBufferSize, u.WriteBufferPool, br, writeBuf)
@@ -220,7 +220,7 @@ func (u *Upgrader) Upgrade(w http.ResponseWriter, r *http.Request, responseHeade
220220

221221
// Use larger of hijacked buffer and connection write buffer for header.
222222
p := buf
223-
if len(c.writeBuf) > len(p) {
223+
if len(c.writeBuf) > cap(p) {
224224
p = c.writeBuf
225225
}
226226
p = p[:0]
@@ -353,7 +353,7 @@ type brNetConn struct {
353353

354354
func (b *brNetConn) Read(p []byte) (n int, err error) {
355355
if b.br != nil {
356-
// Limit read to buferred data.
356+
// Limit read to buffered data.
357357
if n := b.br.Buffered(); len(p) > n {
358358
p = p[:n]
359359
}

server_test.go

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,3 +169,71 @@ func TestHijack_NotSupported(t *testing.T) {
169169
t.Fatalf("got err=%T and status_code=%d", err, recorder.Code)
170170
}
171171
}
172+
173+
func TestWriteBufferReuse(t *testing.T) {
174+
// Test that Upgrader.Upgrade correctly reuses the hijacked write buffer
175+
// from bufio.Writer.AvailableBuffer(). AvailableBuffer() returns a slice
176+
// with len==0 and cap equal to the available buffer space, so the check
177+
// must use cap(buf) instead of len(buf).
178+
179+
for _, tt := range []struct {
180+
name string
181+
bufSize int
182+
wantReuse bool
183+
}{
184+
{"large enough buffer", 4096, true},
185+
{"too small buffer", 128, false},
186+
} {
187+
t.Run(tt.name, func(t *testing.T) {
188+
var writeBuf bytes.Buffer
189+
br := bufio.NewReaderSize(strings.NewReader(""), tt.bufSize)
190+
bw := bufio.NewWriterSize(&writeBuf, tt.bufSize)
191+
192+
// Get the AvailableBuffer to compare addresses later.
193+
availBuf := bw.AvailableBuffer()
194+
if len(availBuf) != 0 {
195+
t.Fatalf("AvailableBuffer len=%d, want 0", len(availBuf))
196+
}
197+
if cap(availBuf) != tt.bufSize {
198+
t.Fatalf("AvailableBuffer cap=%d, want %d", cap(availBuf), tt.bufSize)
199+
}
200+
201+
brw := bufio.NewReadWriter(br, bw)
202+
resp := &reuseTestResponseWriter{
203+
brw: brw,
204+
ResponseWriter: httptest.NewRecorder(),
205+
}
206+
207+
upgrader := Upgrader{
208+
CheckOrigin: func(r *http.Request) bool { return true },
209+
}
210+
c, err := upgrader.Upgrade(resp, &http.Request{
211+
Method: http.MethodGet,
212+
Header: http.Header{
213+
"Upgrade": []string{"websocket"},
214+
"Connection": []string{"upgrade"},
215+
"Sec-Websocket-Key": []string{"dGhlIHNhbXBsZSBub25jZQ=="},
216+
"Sec-Websocket-Version": []string{"13"},
217+
},
218+
}, nil)
219+
if err != nil {
220+
t.Fatalf("Upgrade: %v", err)
221+
}
222+
defer c.Close()
223+
224+
if tt.wantReuse {
225+
// When the buffer is large enough, the connection write buffer
226+
// should be backed by the same underlying array as the hijacked
227+
// writer's AvailableBuffer.
228+
if cap(availBuf) > 0 && len(c.writeBuf) > 0 && &c.writeBuf[0] != &availBuf[:cap(availBuf)][0] {
229+
t.Error("write buffer was not reused from hijacked connection")
230+
}
231+
} else {
232+
// When the buffer is too small, a new buffer should be allocated.
233+
if cap(availBuf) > 0 && len(c.writeBuf) > 0 && &c.writeBuf[0] == &availBuf[:cap(availBuf)][0] {
234+
t.Error("write buffer was unexpectedly reused from small hijacked buffer")
235+
}
236+
}
237+
})
238+
}
239+
}

0 commit comments

Comments
 (0)