net/textproto: NewConn fails to initialize commonHeader prior to use

[tomarus]

What version of Go are you using (go version)?

$ go version
go version go1.16.2 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/tommy/.cache/go-build"
GOENV="/home/tommy/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/tommy/go/pkg/mod"
GONOPROXY="code.xsnews.nl"
GONOSUMDB="code.xsnews.nl"
GOOS="linux"
GOPATH="/home/tommy/go"
GOPRIVATE="code.xsnews.nl"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.16.2"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/tommy/src/xsnews/src/ufilter/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1908059502=/tmp/go-build -gno-record-gcc-switches"

What did you do?

1. Create a new net.Conn
2. Use textproto.NewConn() to initialize the textproto connection.
3. Use ReadMIMEHeader() on this textproto.Reader conn.

In this case, the canonicalMimeHeader map is uninitialized. If i would use textproto.NewReader() this map is correctly initialized using the sync.Once mechanism.

ReadMIMEHeader() uses canonicalMIMEHeaderKey() which doesn't initialize the map. If you use the public function CanonicalMIMEHeaderKey() then the map is correctly initialized.

This should fix it:

diff --git a/src/net/textproto/reader.go b/src/net/textproto/reader.go
index 5c3084f8a7..2c10067880 100644
--- a/src/net/textproto/reader.go
+++ b/src/net/textproto/reader.go
@@ -633,6 +633,8 @@ func validHeaderFieldByte(b byte) bool {
 // For invalid inputs (if a contains spaces or non-token bytes), a
 // is unchanged and a string copy is returned.
 func canonicalMIMEHeaderKey(a []byte) string {
+       commonHeaderOnce.Do(initCommonHeader)
+
        // See if a looks like a header key. If not, return it unchanged.
        for _, c := range a {
                if validHeaderFieldByte(c) {

But i believe this is a better solution: https://go-review.googlesource.com/c/go/+/170317/

What did you expect to see?

No fatal error or data race.

What did you see instead?

==================
WARNING: DATA RACE
Write at 0x000000c761c0 by goroutine 162:
  code.xsnews.nl/fork/textproto.initCommonHeader()
      /home/tommy/go/pkg/mod/code.xsnews.nl/fork/textproto@v0.0.0-20200302122552-071cbcc3bb82/reader.go:689 +0x64
  sync.(*Once).doSlow()
      /usr/local/go/src/sync/once.go:66 +0x109
  sync.(*Once).Do()
      /usr/local/go/src/sync/once.go:57 +0x68
  code.xsnews.nl/fork/textproto.CanonicalMIMEHeaderKey()
      /home/tommy/go/pkg/mod/code.xsnews.nl/fork/textproto@v0.0.0-20200302122552-071cbcc3bb82/reader.go:611 +0x64
  code.xsnews.nl/fork/textproto.MIMEHeader.Get()
      /home/tommy/go/pkg/mod/code.xsnews.nl/fork/textproto@v0.0.0-20200302122552-071cbcc3bb82/header.go:34 +0x54
  main.writeOutFile()
      /home/tommy/src/xsnews/src/ntd/cmd/collector/main.go:166 +0x1f0
  main.do.func2()
      /home/tommy/src/xsnews/src/ntd/cmd/collector/main.go:89 +0x38

Previous read at 0x000000c761c0 by goroutine 54:
  code.xsnews.nl/fork/textproto.canonicalMIMEHeaderKey()
      /home/tommy/go/pkg/mod/code.xsnews.nl/fork/textproto@v0.0.0-20200302122552-071cbcc3bb82/reader.go:677 +0x1e4
  code.xsnews.nl/fork/textproto.(*Reader).ReadMIMEHeader()
      /home/tommy/go/pkg/mod/code.xsnews.nl/fork/textproto@v0.0.0-20200302122552-071cbcc3bb82/reader.go:530 +0x1fb
  code.xsnews.nl/lib/pnntp.NewHeaderFromTextproto()
      /home/tommy/go/pkg/mod/code.xsnews.nl/lib/pnntp@v0.0.0-20200508113836-035d731406d9/reader.go:54 +0x3c
  code.xsnews.nl/lib/pnntp.(*Client).Head()
      /home/tommy/go/pkg/mod/code.xsnews.nl/lib/pnntp@v0.0.0-20200508113836-035d731406d9/client.go:176 +0x46b
  main.importNNTPConn()
      /home/tommy/src/xsnews/src/ntd/cmd/collector/main.go:118 +0x166
  main.do.func1()
      /home/tommy/src/xsnews/src/ntd/cmd/collector/main.go:81 +0xb6

(We forked net/textproto to better debug it but it's basically the same)

[mknyszek]

CC @bradfitz @rsc via https://dev.golang.org/owners, but also @bcmills who wrote https://go-review.googlesource.com/c/go/+/170317/.

Since it looks like you have a patch that works, feel free to send it if you feel inclined. See https://golang.org/doc/contribute for details.

[bcmills]

canonicalMIMEHeaderKey is called in an inner loop, so it seems preferable to avoid the init synchronization there.

Probably it would suffice to add a (guarded) call to initCommonHeader in NewConn.

[bcmills]

(The more laborious part of this change, I think, is to add a regression test that demonstrates the bug.)

[gopherbot]

Change https://go.dev/cl/397575 mentions this issue: net/textproto: initialize commonHeader in NewConn