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: Panic when unmarshaling specially crafted structs #16497

Open
SamWhited opened this issue Jul 26, 2016 · 6 comments

Comments

Projects
None yet
5 participants
@SamWhited
Copy link
Member

commented Jul 26, 2016

Please answer these questions before submitting your issue. Thanks!

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

go version devel +67f799c Tue Jul 26 00:18:42 2016 +0000 linux/amd64

But it also affects the version of 1.6 on the playground at the moment.

  1. What operating system and processor architecture are you using (go env)?
GOARCH="amd64"
GOBIN="/home/sam/bin"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/sam"
GORACE=""
GOROOT="/home/sam/src/go"
GOTOOLDIR="/home/sam/src/go/pkg/tool/linux_amd64"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build800221808=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
  1. What did you do?

When attempting to unmarshal XML into a struct that is composed of another struct that has an XMLName field that has a field number greater than or equal to the number of fields in the original struct, finfo.Value panics because reflect tries to get the value of the field in the parent struct at the location of XMLName in the child struct (which has a field index that is out of bounds when applied to the parent struct).

For example, this contrived example panics:

package main

import (
    "encoding/xml"
)

type IQ struct {
    Type    string   `xml:"type,attr"`
    XMLName xml.Name `xml:"iq"`
}

func main() {
    resp := struct {
        IQ
    }{}
    xml.Unmarshal([]byte(`<iq/>`), &resp)
}

(Playground)

but if we move the fields around:

type IQ struct {
    XMLName xml.Name `xml:"iq"`
    Type    string   `xml:"type,attr"`
}

it works (Playground)

@bradfitz bradfitz added this to the Go1.8 milestone Jul 26, 2016

@SamWhited

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2016

Now that it's not the middle of the night I decided to take a look at this; it appears that when composed fields are used in the parent structs metadata, their indexes are rewritten, but this was not done for xmlname fields (which are handled separately). This is a relatively easy fix (CL).

@gopherbot

This comment has been minimized.

Copy link

commented Jul 26, 2016

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

@SamWhited

This comment has been minimized.

Copy link
Member Author

commented Jul 27, 2016

I was going to post this on the CL, but I think GitHub or the dev list might be a better place (I never know where to post things). If this is incorrect, please let me know and I'll move it.

Currently the patch I have in the CL breaks two tests, but I think the behavior is wrong and the tests need to be changed. I'm not sure if this behavior is covered by the compatibility promise or not, but XMLNS fixes have been put in place in the past that changed the behavior of marshaling/unmarshaling with the rational that they were bug fixes and not part of the compatibility promise, so I'm hoping this will be simliar.

The issue is that right now if you unmarshal XML into a struct that contains an inner struct with an XMLName the XMLName will be matched against the XML correctly and the struct (and all its attributes) will be unmarshaled correctly however the XMLName field on the inner struct will not be set. If you go the other way and marshal that struct back to XML however the inner XML's XMLName tag will be used and the resulting XML will have the correct name and namespace.

Example — https://play.golang.org/p/bunS8dHMAO

This means that Marshal and Unmarshal are not inverse functions, which feels like a bug to me.

Thoughts? Is it okay to change this behavior so that the example above, instead of printing:

<outerstruct xmlns="urn:example:issue:16497"></outerstruct>
InnerStruct: {XMLName:{Space:urn:example:issue:16497 Local:outerstruct}}
OuterStruct: {InnerStruct:{XMLName:{Space: Local:}}}
OuterOuterStruct: {OuterStruct:{InnerStruct:{XMLName:{Space: Local:}}}}

would print:

<outerstruct xmlns="urn:example:issue:16497"></outerstruct>
InnerStruct: {XMLName:{Space:urn:example:issue:16497 Local:outerstruct}}
OuterStruct: {InnerStruct:{XMLName:{Space:urn:example:issue:16497 Local:outerstruct}}}
OuterOuterStruct: {OuterStruct:{InnerStruct:{XMLName:{Space:urn:example:issue:16497 Local:outerstruct}}}}

@rsc rsc modified the milestones: Go1.9Early, Go1.8 Oct 26, 2016

@bradfitz bradfitz modified the milestones: Go1.10Early, Go1.9Early May 3, 2017

@bradfitz bradfitz modified the milestones: Go1.10Early, Go1.10 Jun 14, 2017

@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017

@gopherbot

This comment has been minimized.

Copy link

commented Apr 20, 2018

Change https://golang.org/cl/108395 mentions this issue: encoding/xml : fix panic of unmarshaling of anonymous structs

@iWdGo

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2018

The crash is caused by known and documented panics of the reflection package. The crash cannot be prevented by fixing the type. The submitted fix checks that no value is requested by anonymous structs which have no value anyway.

@gopherbot

This comment has been minimized.

Copy link

commented Apr 27, 2018

Change https://golang.org/cl/109855 mentions this issue: encoding/xml : Fixes to enforce XML namespace standard

@bradfitz bradfitz modified the milestones: Go1.11, Go1.12 May 18, 2018

@gopherbot gopherbot modified the milestones: Go1.12, Unplanned May 23, 2018

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