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
Closed

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

awalterschulze opened this issue Feb 13, 2017 · 11 comments
Milestone

Comments

@awalterschulze
Copy link

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

dsnet commented Feb 13, 2017

\cc @rsc @bradfitz

@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
Copy link
Member

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

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

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

@awalterschulze
Copy link
Author

credit to @Civil for reporting.

@ianlancetaylor
Copy link
Contributor

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

CC @broady @bradfitz

@bradfitz
Copy link
Contributor

bradfitz commented Feb 15, 2017 via email

@gopherbot
Copy link

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

@rsc
Copy link
Contributor

rsc commented Feb 15, 2017

Merged.

@rsc rsc closed this as completed Feb 15, 2017
gopherbot pushed a commit that referenced this issue Feb 15, 2017
…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
Copy link
Author

Thank you

albertjin added a commit to albertjin/golang-go that referenced this issue Feb 16, 2017
…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
@rsc rsc removed their assignment Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants