From e4c5dadc690e06507423ed284bf712a16ddd7f09 Mon Sep 17 00:00:00 2001 From: Riccardo Perotti Date: Fri, 12 Jun 2026 15:55:26 -0400 Subject: [PATCH] Fix two latent scamp-go bugs: announce-parse panic + Send error swallow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both surfaced during the golangci-lint rollout (dead assignments that hid pre-existing bugs). Bug 1 — newServiceProxy (serviceproxy.go): malformed/short announce data caused index-out-of-range panics (classRecords[0..7] and actionsRawMessages[0..1] were indexed with no length check). Add length guards that return a clean error instead of panicking. Bug 2 — Connection.Send (connection.go): on "use of closed connection" / "broken pipe" write errors the loop broke and Send returned nil, silently losing the message. Propagate a new ErrConnectionClosed sentinel so callers can detect the failure and reconnect/retry. Also early-return on a known-closed connection instead of writing anyway. Co-Authored-By: Claude Opus 4.8 (1M context) --- scamp/connection.go | 23 ++++++++++++----------- scamp/serviceproxy.go | 7 +++++++ 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/scamp/connection.go b/scamp/connection.go index 053f1e1..c54eb28 100644 --- a/scamp/connection.go +++ b/scamp/connection.go @@ -3,6 +3,7 @@ package scamp import ( "bufio" "crypto/tls" + "errors" "fmt" "io" "strings" @@ -254,13 +255,17 @@ func (conn *Connection) routePacket(pkt *Packet) (err error) { const RetryLimit = 50 +// ErrConnectionClosed is returned by Send when a packet write fails because the +// underlying connection is closed or broken; callers should reconnect/retry. +var ErrConnectionClosed = errors.New("scamp: connection closed or broken during send") + // Send sends a scamp message using the current *Connection func (conn *Connection) Send(msg *Message) (err error) { if conn == nil { return fmt.Errorf("cannot send on nil connection") } if conn.isClosed { - err = fmt.Errorf("connection already closed") + return fmt.Errorf("connection already closed") } conn.readWriterLock.Lock() @@ -292,16 +297,12 @@ func (conn *Connection) Send(msg *Message) (err error) { _, err := pkt.Write(conn.readWriter) // TODO: should we actually blacklist this error? if err != nil { - // temprarily - if strings.Contains(err.Error(), "use of closed connection") { - // NOTE: this error is intentionally swallowed (matches - // pre-existing behavior); the outer Send returns nil here. - break - } - // TODO: attempt to reconnect - if strings.Contains(err.Error(), "broken pipe") { - // NOTE: error intentionally swallowed (pre-existing behavior). - break + // A closed or broken connection won't recover by retrying; + // propagate so callers can detect it and reconnect/retry + // (previously these were swallowed and Send returned nil). + if strings.Contains(err.Error(), "use of closed connection") || + strings.Contains(err.Error(), "broken pipe") { + return fmt.Errorf("%w: %v", ErrConnectionClosed, err) } if retries > RetryLimit { diff --git a/scamp/serviceproxy.go b/scamp/serviceproxy.go index ec6d572..da6950a 100644 --- a/scamp/serviceproxy.go +++ b/scamp/serviceproxy.go @@ -182,6 +182,9 @@ func newServiceProxy(classRecordsRaw []byte, certRaw []byte, sigRaw []byte) (sp if err != nil { return } + if len(classRecords) < 8 { + return nil, fmt.Errorf("malformed announce: expected at least 8 class records, got %d", len(classRecords)) + } // OMG, position-based, heterogenously typed values in an array suck to deal with. err = json.Unmarshal(classRecords[0], &sp.version) if err != nil { @@ -270,6 +273,10 @@ func newServiceProxy(classRecordsRaw []byte, certRaw []byte, sigRaw []byte) (sp return nil, err } + if len(actionsRawMessages) < 2 { + return nil, fmt.Errorf("malformed announce: action spec needs at least 2 entries, got %d", len(actionsRawMessages)) + } + err = json.Unmarshal(actionsRawMessages[0], &classes[i].actions[j].actionName) if err != nil { return nil, err