encoding/xml: Decoder does not reject xml-ProcInst preceeded by other data

[Merovius]

Go version

go version go1.22.0 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/mero/.cache/go-build'
GOENV='/home/mero/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/mero/pkg/mod'
GONOPROXY='*.edg.ag,*.vwd.com,bitbucket.org/vauwede/*,infrontfinance.dev/*,iqag.dev/*'
GONOSUMDB='*.edg.ag,*.vwd.com,bitbucket.org/vauwede/*,infrontfinance.dev/*,iqag.dev/*'
GOOS='linux'
GOPATH='/home/mero'
GOPRIVATE='*.edg.ag,*.vwd.com,bitbucket.org/vauwede/*,infrontfinance.dev/*,iqag.dev/*'
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/mero/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='local'
GOTOOLDIR='/home/mero/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.22.0'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/mero/src/r/priips-import/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build194995846=/tmp/go-build -gno-record-gcc-switches'

What did you do?

Playground link:

package main

import (
	"encoding/xml"
	"fmt"
	"io"
	"strings"
)

func main() {
	s := ` <?xml version="1.0"?><!DOCTYPE x><x></x>` // note the space
	d := xml.NewDecoder(strings.NewReader(s))
	for {
		t, err := d.Token()
		fmt.Printf("%#v, %v\n", t, err)
		if err == io.EOF {
			break
		}
	}

	fmt.Println()
	fmt.Println(xml.Unmarshal([]byte(s), new(X)))
}

type X struct{}

What did you see happen?

xml.CharData{0x20}, <nil>
xml.ProcInst{Target:"xml", Inst:[]uint8{0x76, 0x65, 0x72, 0x73, 0x69, 0x6f, 0x6e, 0x3d, 0x22, 0x31, 0x2e, 0x30, 0x22}}, <nil>
xml.Directive{0x44, 0x4f, 0x43, 0x54, 0x59, 0x50, 0x45, 0x20, 0x78}, <nil>
xml.StartElement{Name:xml.Name{Space:"", Local:"x"}, Attr:[]xml.Attr{}}, <nil>
xml.EndElement{Name:xml.Name{Space:"", Local:"x"}}, <nil>
<nil>, EOF

<nil>

What did you expect to see?

xml.CharData{0x20}, <nil>
<nil>, invalid XML: xml declaration is not the first token

invalid XML: xml declaration is not the first token

The specification tells us that a valid XML document must not have whitespace before the <?xml …?> instruction (if present):

document ::= prolog element Misc*
prolog ::= XMLDecl? Misc* (doctypedecl Misc*)?

However, both the Decoder and Unmarshal accept an input with leading whitespace.

This tripped us up, because we use the Decoder to convert an XML document into a token stream, manipulate that and then use an Encoder.EncodeToken to translate it back into XML. This broke when fed such an invalid input, because the Encoder checks this invariant (it returns an error), but the Decoder doesn't: https://go.dev/play/p/Gv89nolZNtM

[Merovius]

Note: this also happens with Decoder.Strict == true: https://go.dev/play/p/bRhbEqXPdTr

[thanm]

@rsc per owners

[thanm]

Thanks for the report. Would you be willing to send a CL?

[Merovius]

Sure

[Merovius]

This doesn't just affect whitespace, but also other tokens, which is definitely not correct: https://go.dev/play/p/7cfsv_Ez5UX

[gopherbot]

Change https://go.dev/cl/564035 mentions this issue: encoding/xml: reject XML declaration after start of document

[nkitchen]

The fix for this issue depends on an implicit assumption that the input stream of a Decoder contains only 1 XML document. That expectation is not expressed by the documentation of the package. Should it be a valid use case to use a single Decoder to parse the concatenation of multiple documents?

[Merovius]

@nkitchen I'm not sure that's the only way in which the Decoder assumes that. Casually skimming the code, it seems to at least also assume it with namespace-handling. And note that the Encoder also assumes that there is a single document. Personally, I think it's a fair assumption (the behavior of json.Decoder always stroke me as kind of strange).

Long-term we probably want an encoding/xml/v2, which, like the proposed encoding/json/v2 really separates the tokenization and validation/unmarshalling steps.

[gopherbot]

Change https://go.dev/cl/570175 mentions this issue: Revert "encoding/xml: reject XML declaration after start of document"

[rsc]

See my comments on CL 570175. At the least this change requires a proposal and a GODEBUG for compatibility.

[DemiMarie]

Long-term we probably want an encoding/xml/v2, which, like the proposed encoding/json/v2 really separates the tokenization and validation/unmarshalling steps.

I think that after this encoding/xml should be deprecated.