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: createAttrPrefix should ban "XMLanything" case insensitive, not just "xmlanything" #35151

Open
tgulacsi opened this issue Oct 25, 2019 · 21 comments
Milestone

Comments

@tgulacsi
Copy link
Contributor

@tgulacsi tgulacsi commented Oct 25, 2019

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

$ go version
go version go1.13.3 linux/amd64

Does this issue reproduce with the latest release?

AFAIK yes.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN="/home/gthomas/bin"
GOCACHE="/home/gthomas/.cache/go-build"
GOENV="/home/gthomas/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY="unosoft.hu"
GONOSUMDB="unosoft.hu"
GOOS="linux"
GOPATH="/home/gthomas/go"
GOPRIVATE="unosoft.hu"
GOPROXY="http://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
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-build491111576=/tmp/go-build -gno-record-gcc-switches"

What did you do?

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

What did you expect to see?

<element xmlns:_XMLSchema-instance="http://www.w3.org/2001/XMLSchema-instance" _XMLSchema-instance:nil="true"></element>

What did you see instead?

<element xmlns:XMLSchema-instance="http://www.w3.org/2001/XMLSchema-instance" XMLSchema-instance:nil="true"></element>
@tgulacsi
Copy link
Contributor Author

@tgulacsi tgulacsi commented Oct 25, 2019

https://www.w3.org/TR/xml-names/ says

All other prefixes beginning with the three-letter sequence x, m, l, in any case combination, are reserved.

under 3 "Declaring Namespaces".

Also https://www.w3.org/TR/REC-xml/ says

Names beginning with the string "xml", or with any string which would match (('X'|'x') ('M'|'m') ('L'|'l')), are reserved for standardization in this or future versions of this specification.

under "2.3 Common Syntactic Constructs".

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 25, 2019

Change https://golang.org/cl/203417 mentions this issue: encoding/xml: namespace prefix case insensitive check

@tgulacsi
Copy link
Contributor Author

@tgulacsi tgulacsi commented Nov 2, 2019

Can I help with anything?
The second version of my CL is simpler and more explicit, I hope.

@tgulacsi
Copy link
Contributor Author

@tgulacsi tgulacsi commented Mar 19, 2020

Ping.

@odeke-em odeke-em added this to the Go1.15 milestone Mar 20, 2020
@odeke-em
Copy link
Member

@odeke-em odeke-em commented Mar 20, 2020

Thank you for filing this bug and for mailing the CL! I shall provide some feedback on your change, and awesome to have your contribution to the Go project!

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Apr 8, 2020

@tgulacsi I sent some feedback to your CL about 20 days ago, please take a look, so that we can get it going before the code freeze. Thank you.

@tgulacsi
Copy link
Contributor Author

@tgulacsi tgulacsi commented Apr 8, 2020

Sorry, haven't seen it.

@gopherbot gopherbot closed this in 346d7d2 Apr 9, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 26, 2020

Change https://golang.org/cl/240012 mentions this issue: doc/go1.15: remove encoding/xml doc

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 26, 2020

Change https://golang.org/cl/240179 mentions this issue: Revert "encoding/xml: fix reserved namespace check to be case-insensitive"

gopherbot pushed a commit that referenced this issue Jun 29, 2020
…tive"

This reverts CL 203417.

Reason for revert: This change changes uses of tags like "XMLSchema-instance" without any recourse.

For #35151
Fixes #39876

Change-Id: I4c85c8267a46b3748664b5078794dafffb42aa26
Reviewed-on: https://go-review.googlesource.com/c/go/+/240179
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Reviewed-by: Andrew Bonventre <andybons@golang.org>
gopherbot pushed a commit that referenced this issue Jun 29, 2020
The change is rolled back in CL 240179.

For #35151
For #39876

Change-Id: Id26ccbdb482772ac31c642156a9900102397b043
Reviewed-on: https://go-review.googlesource.com/c/go/+/240012
Reviewed-by: Alberto Donizetti <alb.donizetti@gmail.com>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
@odeke-em
Copy link
Member

@odeke-em odeke-em commented Jun 30, 2020

Reopening and retargeting for Go1.16 to address the raised cause of rollback during Go1.15.

@odeke-em odeke-em reopened this Jun 30, 2020
@odeke-em odeke-em modified the milestones: Go1.15, Go1.16 Jun 30, 2020
@tgulacsi
Copy link
Contributor Author

@tgulacsi tgulacsi commented Aug 23, 2020

Ping

1 similar comment
@tgulacsi
Copy link
Contributor Author

@tgulacsi tgulacsi commented Sep 14, 2020

Ping

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 14, 2020

@tgulacsi A minor point, but if you want us to review a change, please ping the change, not the associated issue. Thanks.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 14, 2020

But now I'm not sure what is going on here. Is there a change to review? What is the ping for? Who are you pinging?

@tgulacsi
Copy link
Contributor Author

@tgulacsi tgulacsi commented Sep 15, 2020

I'd like some directions - my previous CL was reviewed, accepted, then reverted.

Now I don't know what was the problem with it, what should I change to have it accepted - without information, I can only reissue the same CL.

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 15, 2020

Change https://golang.org/cl/254978 mentions this issue: encoding/xml: opt-in fix reserved namespace check to be case-insensitive

@tgulacsi
Copy link
Contributor Author

@tgulacsi tgulacsi commented Sep 15, 2020

PTAL https://go-review.googlesource.com/c/go/+/254978 - it reintroduces the same code change, but puts it behind an Encoder option.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 16, 2020

@tgulacsi Thanks for the context. I look at dozens of different issues every day, so I can't always keep them all in my head.

The CL that reverted your CL, https://golang.org/cl/240179, says "Reason for revert: This change changes uses of tags like "XMLSchema-instance" without any recourse" and refers to #39876. #39876 explains the problem, and you commented there. That issue has a test case whose behavior changed. Any change we make should not change the behavior of that test case, or other similar test cases.

The Go project takes backward compatibility very seriously. We do not want to break existing programs. There needs to be a very good reason to not do that. Perhaps we made a mistake in the fix for #5040, but we still don't want to break existing programs.

Worse, as #39876 says, after your earlier change (https://golang.org/cl/203417) there was no way for a program to get the string that it got before. So not only did the behavior change, there was no way to get the old behavior.

In your new CL (https://golang.org/cl/254978) you are introducing new API: a new method xml.(*Encoder).XMLPrefixCaseInsensitive. We usually use the proposal process for new API.

But first let's take a step back.

What is the real problem here that we are trying to solve? What goes wrong if we don't fix this? Perhaps you understand that, but I don't. Perhaps that can help guide us to the right solution.

Thanks.

@tgulacsi
Copy link
Contributor Author

@tgulacsi tgulacsi commented Sep 16, 2020

From one of my clients, I got back this:

<bmRating xmlns:XMLSchema-instance="http://www.w3.org/2001/XMLSchema-instance" XMLSchema-instance:nil="true"></bmRating>

error: Prefix can't begin with XML: XMLSchema-instance

Prefixes beginning with "xml" (regardless of casing) are reserved for use by XML.

And https://www.w3.org/TR/REC-xml/ says she's right.

That's what I try to solve.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 18, 2020

Sorry if this is a dumb question, but where is the string XMLSchema-instance coming from?

@tgulacsi
Copy link
Contributor Author

@tgulacsi tgulacsi commented Sep 18, 2020

As far as I see, it's https://github.com/golang/go/blob/master/src/encoding/xml/marshal.go#L339 where it starts to pick a short name (tag?) for the namespace. If possible, it uses the namespace url's path's last section.

AFAIK a random name would work, too - that's why I don't understand what does it break.
That short name could be anything - though it cannot start with [xX][mM][lL].

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
5 participants
You can’t perform that action at this time.