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

Closed
tgulacsi opened this issue Oct 25, 2019 · 25 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@tgulacsi
Copy link
Contributor

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

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

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

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 25, 2019
@tgulacsi
Copy link
Contributor Author

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

Ping.

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

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 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 commented Apr 8, 2020

Sorry, haven't seen it.

@gopherbot
Copy link

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

@gopherbot
Copy link

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

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

Ping

1 similar comment
@tgulacsi
Copy link
Contributor Author

Ping

@ianlancetaylor
Copy link
Contributor

@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

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

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

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

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

@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

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

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

@tgulacsi
Copy link
Contributor Author

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].

@ianlancetaylor
Copy link
Contributor

@tgulacsi OK, I've looked into this, and I've come around to the view that you are correct: we should make this change and fix code that breaks.

@ianlancetaylor
Copy link
Contributor

Thanks for your patience with this.

@gopherbot
Copy link

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

@tgulacsi
Copy link
Contributor Author

Thank you @ianlancetaylor and @odeke-em !
This is not a big issue for me, I have and use a not-so-simple-but-working workaround.

The big thing is that I thought I can contribute to the Go project, and have a tiny improvement on Go,
my beloved language, standard library and ecosystem.
I was in joy and then it's been rolled back.

So thanks again for looking again into it, and rolling forward!

gopherbot pushed a commit that referenced this issue Oct 21, 2020
Fixes the check for the reserved namespace prefix
"xml" to be case insensitive, so as to match all variants of:

    (('X'|'x')('M'|'m')('L'|'l'))

as mandated by Section 2.3 of https://www.w3.org/TR/REC-xml/

This is a roll forward of CL 203417, which was rolled back by CL 240179.
We've decided that the roll back was incorrect, and any broken tests
should be fixed.

The original CL 203417 was by Tamás Gulácsi.

Fixes #35151
For #39876

Change-Id: I2e6daa7aeb252531fba0b8a56086613e13059528
Reviewed-on: https://go-review.googlesource.com/c/go/+/264024
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
@golang golang locked and limited conversation to collaborators Oct 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants