Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
tomarus opened this issue May 25, 2021 · 4 comments
Closed

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

tomarus opened this issue May 25, 2021 · 4 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@tomarus
Copy link

tomarus commented May 25, 2021

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 mknyszek added this to the Backlog milestone May 25, 2021
@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 25, 2021
@mknyszek
Copy link
Contributor

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.

@mknyszek mknyszek added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 25, 2021
@bcmills bcmills changed the title net/textproto: fatal error: concurrent map read and map write net/textproto: ReadMIMEHeader fails to initialize commonHeader prior to use May 25, 2021
@bcmills bcmills changed the title net/textproto: ReadMIMEHeader fails to initialize commonHeader prior to use net/textproto: NewConn fails to initialize commonHeader prior to use May 25, 2021
@bcmills
Copy link
Contributor

bcmills commented May 25, 2021

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
Copy link
Contributor

bcmills commented May 25, 2021

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

@gopherbot
Copy link
Contributor

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

@golang golang locked and limited conversation to collaborators Apr 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants