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

Regression in implementation of mail.ParseDate(...) #41822

Closed
jameshoulahan opened this issue Oct 6, 2020 · 2 comments
Closed

Regression in implementation of mail.ParseDate(...) #41822

jameshoulahan opened this issue Oct 6, 2020 · 2 comments

Comments

@jameshoulahan
Copy link

@jameshoulahan jameshoulahan commented Oct 6, 2020

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

$ go version
go version go1.15.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/james/.cache/go-build"
GOENV="/home/james/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/james/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/james/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/james/Code/bridge/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-build171979470=/tmp/go-build -gno-record-gcc-switches"

What did you do?

func TestParseDate(t *testing.T) {
	if _, err := mail.ParseDate("Tue, 21 Aug 2018 13:41:49 GMT"); err != nil {
		t.Fail()
	}
}

What did you expect to see?

I expected it to pass, as it did on go 1.13.

What did you see instead?

It fails on newer versions of go (1.14.7 and later, possibly earlier).

The go 1.15 implementation contains the following code block in ParseDate(string) (lines 110-120 of mail/message.go, as of the time of writing):

	// RFC 5322: zone = (FWS ( "+" / "-" ) 4DIGIT) / obs-zone
	// zone length is always 5 chars unless obsolete (obs-zone)
	if ind := strings.IndexAny(p.s, "+-"); ind != -1 && len(p.s) >= ind+5 {
		date = p.s[:ind+5]
		p.s = p.s[ind+5:]
	} else if ind := strings.Index(p.s, "T"); ind != -1 && len(p.s) >= ind+1 {
		// The last letter T of the obsolete time zone is checked when no standard time zone is found.
		// If T is misplaced, the date to parse is garbage.
		date = p.s[:ind+1]
		p.s = p.s[ind+1:]
	}

When executing the above test, we enter the else case, and because the string begins with T for Tue, the variable date ends up with value "T" and p.s ends up with "ue, 21 Aug 2018 13:41:49 GMT".

The function then continues and executes the following loop (lines 124-129 of mail/message.go):

	for _, layout := range dateLayouts {
		t, err := time.Parse(layout, date)
		if err == nil {
			return t, nil
		}
	}

This should succeed, because dateLayouts does indeed contain a matching layout (namely Mon, 2 Jan 2006 15:04:05 MST), but by this point date has been reduced to just T, so it fails.

I feel that the first block of code, which checks the placement of T, shouldn't break the date string, as in the test case provided above.

Go 1.13 did not contain the above mentioned block of code that reduces date down to "T" and thus successfully parses the provided date string.

@jameshoulahan jameshoulahan changed the title Regression in implementation of `mail.ParseDate(...)` Regression in implementation of mail.ParseDate(...) Oct 6, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 6, 2020

Dup of #39260?

@jameshoulahan
Copy link
Author

@jameshoulahan jameshoulahan commented Oct 6, 2020

Indeed, that's exactly the issue I describe here. Apologies for the noise! Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.