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

encoding/xml: bad type for comment field of "pointer to string" #19063

Closed
awalterschulze opened this Issue Feb 13, 2017 · 11 comments

Comments

Projects
None yet
7 participants
@awalterschulze
Copy link

awalterschulze commented Feb 13, 2017

Please answer these questions before submitting your issue. Thanks!

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

go1.8rc3

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/waschulze/code"
GORACE=""
GOROOT="/Users/waschulze/go1.8rc3"
GOTOOLDIR="/Users/waschulze/go1.8rc3/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/vg/phjwfkn14tjdh3l838gn4q_c395_bf/T/go-build149844404=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

https://play.golang.org/p/AgMlZ8P4eN

What did you expect to see?

Program exited

What did you see instead?

panic: xml: bad type for comment field of main.A

@dsnet

This comment has been minimized.

Copy link
Member

dsnet commented Feb 13, 2017

@dsnet dsnet added this to the Go1.8Maybe milestone Feb 13, 2017

@josharian josharian changed the title go1.8rc3 xml: bad type for comment field of "pointer to string" encoding/xml: bad type for comment field of "pointer to string" Feb 13, 2017

@odeke-em

This comment has been minimized.

Copy link
Member

odeke-em commented Feb 13, 2017

From empirical examination this seems to be the offending commit daa1211 and CL https://go-review.googlesource.com/c/15684.

In particular the code patch from that CL is the one that causes it, so undoing it the bug goes away

diff --git a/src/encoding/xml/marshal.go b/src/encoding/xml/marshal.go
index abb078c..4fa1de0 100644
--- a/src/encoding/xml/marshal.go
+++ b/src/encoding/xml/marshal.go
@@ -760,14 +760,6 @@
 		}
 		vf := finfo.value(val)
 
-		// Dereference or skip nil pointer, interface values.
-		switch vf.Kind() {
-		case reflect.Ptr, reflect.Interface:
-			if !vf.IsNil() {
-				vf = vf.Elem()
-			}
-		}
-
 		switch finfo.flags & fMode {
 		case fCDATA, fCharData:
 			emit := EscapeText
@@ -800,6 +792,16 @@
 					continue
 				}
 			}
+			// Drill into interfaces and pointers.
+			// This can turn into an infinite loop given a cyclic chain,
+			// but it matches the Go 1 behavior.
+			for vf.Kind() == reflect.Interface || vf.Kind() == reflect.Ptr {
+				if vf.IsNil() {
+					return nil
+				}
+				vf = vf.Elem()
+			}
+
 			var scratch [64]byte
 			switch vf.Kind() {
 			case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
@odeke-em

This comment has been minimized.

Copy link
Member

odeke-em commented Feb 13, 2017

Just an FYI: that CL was written in October 2015, but submitted a year later in October 2016. On first glance the dates might be confusing.

@rsc rsc self-assigned this Feb 13, 2017

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Feb 13, 2017

Will look later tonight.

@rsc rsc modified the milestones: Go1.8, Go1.8Maybe Feb 14, 2017

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Feb 14, 2017

CL https://golang.org/cl/36954 mentions this issue.

@awalterschulze

This comment has been minimized.

Copy link
Author

awalterschulze commented Feb 14, 2017

credit to @Civil for reporting.

@gopherbot gopherbot closed this in 72aa757 Feb 14, 2017

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Feb 14, 2017

Reopening for backport of CL 36954 to Go 1.8 release branch.

CC @broady @bradfitz

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Feb 15, 2017

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Feb 15, 2017

CL https://golang.org/cl/37016 mentions this issue.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Feb 15, 2017

Merged.

@rsc rsc closed this Feb 15, 2017

gopherbot pushed a commit that referenced this issue Feb 15, 2017

[release-branch.go1.8] encoding/xml: fix incorrect indirect code in c…
…hardata, comment, innerxml fields

The new tests in this CL have been checked against Go 1.7 as well
and all pass in Go 1.7, with the one exception noted in a comment
(an intentional change to omitempty already present before this CL).

CL 15684 made the intentional change to omitempty.
This CL fixes bugs introduced along the way.

Most of these are corner cases that are arguably not that important,
but they've always worked all the way back to Go 1, and someone
cared enough to file #19063. The most significant problem found
while adding tests is that in the case of a nil *string field with
`xml:",chardata"`, the existing code silently stops processing not just
that field but the entire remainder of the struct.
Even if #19063 were not worth fixing, this chardata bug would be.

Fixes #19063.

Change-Id: I318cf8f9945e1a4615982d9904e109fde577ebf9
Reviewed-on: https://go-review.googlesource.com/36954
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
(cherry picked from commit 72aa757)
Reviewed-on: https://go-review.googlesource.com/37016
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
@awalterschulze

This comment has been minimized.

Copy link
Author

awalterschulze commented Feb 15, 2017

Thank you

albertjin added a commit to albertjin/golang-go that referenced this issue Feb 16, 2017

[release-branch.go1.8] encoding/xml: fix incorrect indirect code in c…
…hardata, comment, innerxml fields

The new tests in this CL have been checked against Go 1.7 as well
and all pass in Go 1.7, with the one exception noted in a comment
(an intentional change to omitempty already present before this CL).

CL 15684 made the intentional change to omitempty.
This CL fixes bugs introduced along the way.

Most of these are corner cases that are arguably not that important,
but they've always worked all the way back to Go 1, and someone
cared enough to file golang#19063. The most significant problem found
while adding tests is that in the case of a nil *string field with
`xml:",chardata"`, the existing code silently stops processing not just
that field but the entire remainder of the struct.
Even if golang#19063 were not worth fixing, this chardata bug would be.

Fixes golang#19063.

Change-Id: I318cf8f9945e1a4615982d9904e109fde577ebf9
Reviewed-on: https://go-review.googlesource.com/36954
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
(cherry picked from commit 72aa757)
Reviewed-on: https://go-review.googlesource.com/37016
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>

@golang golang locked and limited conversation to collaborators Feb 15, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.