bufio.CachedConn: data race on the cached buffer between Read/ReaderReplaceable and Close

未关闭
  • 哪里不对？
#112 1 条评论 0 个 reaction 已指派 0 人 在 GitHub 查看

还没有人认领这个 Issue。

评估

难度
4/5
预计耗时
3-5 天
新手友好度
55/100
Issue 类型
缺陷
描述清晰度
基本清楚
活跃度
活跃
技术栈
go
领域
networking

调研方向

Start in common/bufio/cache.go with CachedConn.Read, ReaderReplaceable, Close, and the corresponding CachedPacketConn methods. Reproduce the reported concurrent paths with the race detector, including the routing and DNS-over-TCP entry points named in the issue. Done means the cache has one consistent ownership path, without races or double release.

由索引模型根据 Issue 内容生成。

描述

sing v0.8.10 (via sing-box v1.13.12), go 1.26, -race

CachedConn.buffer is read and cleared without synchronization in Read and ReaderReplaceable, while Close (usually called from another goroutine) reads and clears it too. Close guards itself with taken, but Read / ReaderReplaceable don't check or set taken:

// common/bufio/cache.go
func (c *CachedConn) Read(p []byte) (n int, err error) {
	if c.buffer != nil {              // read
		...
		c.buffer.DecRef()
		c.buffer.Release()
		c.buffer = nil                // write
	}
	return c.Conn.Read(p)
}

func (c *CachedConn) ReaderReplaceable() bool {
	return c.buffer == nil            // read
}

func (c *CachedConn) Close() error {
	if c.taken.CompareAndSwap(false, true) {
		if buffer := c.buffer; buffer != nil {   // read (races with Read)
			c.buffer = nil
			buffer.DecRef()
			buffer.Release()          // can release a buffer Read already released
		}
	}
	return c.Conn.Close()
}

CachedPacketConn.ReadPacket / Close have the same pattern.

Besides the race report, the worst case is the pooled buffer being released twice, so it can come back from buf.New to another connection while still referenced.

Where it shows up in sing-box 1.13.12 (both found with -race):

  1. Routing a sniffed TCP connection: route.(*Router).routeConnection wraps it in bufio.NewCachedConn; ConnectionManager.connectionCopy then calls ReaderReplaceable (via N.UnwrapCountReader in bufio.CopyWithCounters) on one goroutine, while the other copy direction calls Close (route/conn.go:262 vs :271).
  2. Hijacked DNS over TCP: protocol/dns.HandleStreamDNSRequest reads the next query from the conn (handle.go:24) while the exchange goroutine of the previous query calls conn.Close() on error (handle.go:46).

When the wrapper can be unwrapped before copying, ReadCached (which does use taken) avoids it, but callers such as a ConnectionTracker that wraps the conn force the unsynchronized Read path.

Suggested fix: make every consumer of the cache take it through the same atomic flag, e.g.

func (c *CachedConn) Read(p []byte) (n int, err error) {
	if !c.taken.Load() {
		c.mu.Lock() // or take the buffer once with CompareAndSwap and keep it reader-local
		...
	}
	return c.Conn.Read(p)
}

or have Read take the buffer once (CompareAndSwap(false, true)), move it into a reader-owned field, and never touch it from Close after that. ReaderReplaceable would then read an atomic. Happy to send a PR if the approach sounds right.

主要语言
Go
星标
131
派生
119
30 天内没有已合并 PR

环境准备

这个项目没有提供开发容器、Dockerfile 或贡献指南，环境需要你自己搭建：先看它的 README，通用步骤见我们的新手贡献指南。

从这里开始

  1. 先读完整个 Issue，再读项目的贡献指南。
  2. 在 Issue 下留言说明你要接手 —— 这能避免两个人做同样的事。
  3. Fork 仓库，在一个分支上完成修改。
  4. 提交 Pull Request，并在描述里引用这个 Issue 编号。

SagerNet/sing 的其他 Issue

查看 SagerNet/sing 的全部 Issue

相似的 Issue

更多 Go Issue

把新 issue 发到你的邮箱

精选适合新手参与的 GitHub issue 摘要。