XHTTP client: Fix a race condition and a data race

[dudkin-2005]

1. uploadWriter.Write reads a buffer it no longer owns

for _, buff := range buffer.MultiBuffer {
	err := w.WriteMultiBuffer(buf.MultiBuffer{buff})
	if err != nil {
		return writed, err
	}
	writed += int(buff.Len())
}

Once WriteMultiBuffer succeeds, the buffer belongs to the pipe's reader. That
reader is the upload loop in Dial, which drains it into the body of the POST
request — MultiBufferContainer.Read → SplitBytes → Buffer.Read — and
Buffer.Read calls Clear() once the buffer runs out, zeroing start and end
while Len() is being read.

This is not only a race. With Len() reading zero, Write reports fewer bytes
than it accepted, and buf.WriteAllBytes advances its payload by the returned
count in a loop:

for len(payload) > 0 {
	n, err := writer.Write(payload)
	wc += n
	if err != nil {
		return err
	}
	payload = payload[n:]
}

so the same bytes go out a second time. They are already in the pipe and already
on their way to the server, so the proxied stream carries duplicated data. Should
the buffer have been recycled and refilled instead, Len() can read larger than
expected and the caller's payload[n:] panics on the slice bounds.

Reading the length before the write keeps the deliberate per-buffer splitting
that bounds how far a single ReadMultiBuffer may exceed the pipe's size limit,
so nothing else about the behaviour changes.

2. DefaultDialerClient.closed is a plain bool

It is written from concurrent goroutines — one per uplink packet in packet-up,
plus the response goroutine in OpenStream — and read by GetXmuxClient under
globalDialerAccess, a mutex the writers never take, so there is no
happens-before between them.

A byte store does not tear on amd64, but nothing orders it either: the reader can
keep observing a stale false and go on handing new proxied requests to a dead
connection until hMaxRequestTimes or hMaxReusableSecs evicts it. Making it an
atomic.Bool matches LeftRequests, Running and NotUsed used a few lines
away in the very same GetXmuxClient check.

Reproduction

The first one, in package splithttp:

func Test_UploadWriterOwnership(t *testing.T) {
	reader, writer := pipe.New(pipe.WithSizeLimit(512 * 1024))
	uw := uploadWriter{writer, 512 * 1024}

	drained := make(chan struct{})
	go func() { // what the upload loop does with the buffers
		defer close(drained)
		for {
			mb, err := reader.ReadMultiBuffer()
			if err != nil {
				return
			}
			c := buf.MultiBufferContainer{MultiBuffer: mb}
			io.Copy(io.Discard, &c)
		}
	}()

	payload := make([]byte, 64*1024)
	short := 0
	for i := 0; i < 3000; i++ {
		n, err := uw.Write(payload)
		if err != nil {
			break
		}
		if n != len(payload) {
			short++
		}
	}
	writer.Close()
	<-drained

	if short > 0 {
		t.Errorf("Write reported a short count %d times", short)
	}
}

The second one needs only concurrent PostPacket calls against a transport that
always errors, plus one goroutine calling IsClosed().

On the unpatched tree the detector reports the race on every run, while the short
count itself lands in roughly 3 runs out of 5, 1–4 times per 3000 writes.

Verification

• go build ./... clean, go vet unchanged, package tests pass.
• go test -race ./transport/internet/splithttp/ drops from ~20 race reports to
  ~12. What remains is WaitReadCloser.ReadCloser and the certificate cache in
  transport/internet/tls, both unrelated to this change — the same 7 tests fail
  under -race before and after it.

[RPRX]

第二个，在 amd64 架构上会这样的深层原因是？

[Fangliding]

主要是回的writed不对 那个closed理论上是没什么问题的 爱改atomic就顺带改了吧

[RPRX]

Google AI 的说法是：

在 Go 语言中，如果一个布尔变量（bool）在多个 Goroutine 之间被并发地读写，就需要使用原子操作（如 Go 1.19 及以上引入的 sync/atomic.Bool）或互斥锁来避免数据竞争（Data Race）。

为什么需要？
非线程安全：Go 语言中对普通 bool 类型的简单读写虽然在绝大多数硬件上不会出现“读到半个残缺值”的情况，但不保证可见性和指令重排。多核 CPU 缓存可能导致一个 Goroutine 修改了 bool 值，其他 Goroutine 很久之后甚至永远看不到更新。
避免数据竞争：运行 go test -race 时，对未加锁保护共享 bool 的并发读写会被检测出数据竞争。

什么时候用？
状态标志（Flags）：如控制协程退出的 isClosed、isRunning，或者开关配置。
高频读写：如果只是简单的状态切换或读取，相比于沉重的 sync.Mutex，使用无锁的 atomic.Bool 性能更好、开销极低。

不过我又查了下多核 CPU 缓存没那么蠢，但总之 atomic.Bool 既然被推出了应该有些意义，以后类似场景都改用这个吧，没多少开销

[Fangliding]

这么写的旧代码在全世界都不少 真出问题就大爆爆了 现代调度不会这么蠢的 也就百来个周期而已 实际上不是极其高频而且强要求一致性基本遇不到 更别说这几个认错了也问题不大可以恢复的

[RPRX]

我也觉得，主要是它这块内存只有 0 或 1 两种可能，多核并发读写理应也没啥问题，不过若有多核缓存不一致问题的话就另说，不过若真广泛存在多核缓存没能快速同步的问题的话那 CPU 也太蠢了，不少东西都会炸，甚至加锁都不一定能解决吧，所以应该不会那么蠢吧