[go] encoding/xml: fix depth processing in unmarshal
9 views
Skip to first unread message
Ian Alexander (Gerrit)
Jul 21, 2026, 12:07:02 PMJul 21
to goph...@pubsubhelper.golang.org, Nicholas Husin, golang...@luci-project-accounts.iam.gserviceaccount.com, golang-co...@googlegroups.com
Attention needed from Nicholas Husin
New activity on the change![]( "Open in Gerrit")
Related details
Attention is currently required from:
- Nicholas Husin
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Alexander (Gerrit)
Jul 21, 2026, 8:17:45 PMJul 21
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Nicholas Husin
Ian Alexander uploaded new patchset![]( "Open in Gerrit")
Ian Alexander uploaded patch set #4 to this change.
Related details
Attention is currently required from:
- Nicholas Husin
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Jul 22, 2026, 1:00:27 AMJul 22
to Ian Alexander, goph...@pubsubhelper.golang.org, golang...@luci-project-accounts.iam.gserviceaccount.com, golang-co...@googlegroups.com
Attention needed from Ian Alexander
Nicholas Husin added 3 comments![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 4 (Latest):
Nicholas Husin . unresolved
TryBots failure for `gotip-wasip1-wasm_wazero` looks real?
File src/encoding/xml/read.go
Line 156, Patchset 4 (Latest):
return d.unmarshal(val.Elem(), start, d.unmarshalDepth)Nicholas Husin . unresolved
Sorry, I'm not familiar with this package at all. But, I think this might be an incomplete fix?
It seems that `Decoder.unmarshalDepth` only keeps track of the `UnmarshalXML` method depth. So, a recursive XML structure where not all `structs` implement `UnmarshalXML` method can still bypass this limit:
```
type Section struct {
Sub *Section `xml:"section"`
Custom *Extension `xml:"extension"`
}
type Extension struct {
Body Section
}func (e *Extension) UnmarshalXML(d *xml.Decoder, start xml.StartElement) error {
var body Section
if err := d.DecodeElement(&body, &start); err != nil {
return err
}
e.Body = body
return nil
}func TestDecodeElementDepthBypass(t *testing.T) {
// Construct a document with 3 blocks of 5,000 nested <section> tags,
// separated by <extension> tags.
// Total XML nesting depth = 15,003 tags deep (maxUnmarshalDepth is 10,000).
openSections := strings.Repeat("<section>", 5000)
closeSections := strings.Repeat("</section>", 5000)var buf strings.Builder
for i := 0; i < 3; i++ {
buf.WriteString(openSections)
buf.WriteString("<extension>")
}
for i := 0; i < 3; i++ {
buf.WriteString("</extension>")
buf.WriteString(closeSections)
}
var sec Section
err := xml.Unmarshal([]byte(buf.String()), &sec)
if err == nil {
t.Fatalf("FAILED: Unmarshaled 15,003 levels deep without error (depth limit bypassed)")
}
```In this case, after reaching a `depth` of 5000 inside the nested `<section>` tags, the `depth` gets reset back to `d.unmarshalDepth` (1, 2, then 3) whenever `<extension>` is encountered.
File src/encoding/xml/read_test.go
Line 1162, Patchset 4 (Latest):
if runtime.GOARCH == "wasm" {
tests = []struct {
name string
depth int
wantErr error
}{
{
name: "wasm below limit",
depth: 4998,
wantErr: nil,
},
{
name: "wasm above limit",
depth: 4999,
wantErr: errUnmarshalDepth,
},
}
}
Nicholas Husin . unresolved
Probably cleaner to just define a `limit` variable that has different value according to `runtime.GOARCH`. Then, the test cases can define `depth` as `limit-1` and so on.
Related details
Attention is currently required from:
- Ian Alexander
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Alexander (Gerrit)
Jul 22, 2026, 9:40:01 PMJul 22
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Ian Alexander
Ian Alexander uploaded new patchset![]( "Open in Gerrit")
Ian Alexander uploaded patch set #5 to this change.
Following approvals got outdated and were removed:
- TryBots-Pass: LUCI-TryBot-Result-1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
Attention is currently required from:
- Ian Alexander
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Alexander (Gerrit)
Jul 22, 2026, 9:41:01 PMJul 22
to goph...@pubsubhelper.golang.org, Nicholas Husin, golang...@luci-project-accounts.iam.gserviceaccount.com, golang-co...@googlegroups.com
Attention needed from Ian Alexander
Ian Alexander voted and added 2 comments![]( "Open in Gerrit")
Votes added by Ian Alexander
| Commit-Queue | +1 |
2 comments
File src/encoding/xml/read.go
Line 156, Patchset 4:
return d.unmarshal(val.Elem(), start, d.unmarshalDepth)Nicholas Husin . resolved
Ian Alexander
Done
File src/encoding/xml/read_test.go
Line 1162, Patchset 4:
if runtime.GOARCH == "wasm" {
tests = []struct {
name string
depth int
wantErr error
}{
{
name: "wasm below limit",
depth: 4998,
wantErr: nil,
},
{
name: "wasm above limit",
depth: 4999,
wantErr: errUnmarshalDepth,
},
}
}
Nicholas Husin . resolved
Probably cleaner to just define a `limit` variable that has different value according to `runtime.GOARCH`. Then, the test cases can define `depth` as `limit-1` and so on.
Ian Alexander
Done
Related details
Attention is currently required from:
- Ian Alexander
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Alexander (Gerrit)
Jul 22, 2026, 10:07:41 PMJul 22
to goph...@pubsubhelper.golang.org, golang...@luci-project-accounts.iam.gserviceaccount.com, Nicholas Husin, golang-co...@googlegroups.com
Ian Alexander voted Commit-Queue+1![]( "Open in Gerrit")
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Alexander (Gerrit)
Jul 22, 2026, 11:02:07 PMJul 22
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Ian Alexander
Ian Alexander uploaded new patchset![]( "Open in Gerrit")
Ian Alexander uploaded patch set #6 to this change.
Following approvals got outdated and were removed:
- TryBots-Pass: LUCI-TryBot-Result-1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
Attention is currently required from:
- Ian Alexander
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Alexander (Gerrit)
Jul 22, 2026, 11:02:35 PMJul 22
to goph...@pubsubhelper.golang.org, golang...@luci-project-accounts.iam.gserviceaccount.com, Nicholas Husin, golang-co...@googlegroups.com
Ian Alexander voted Commit-Queue+1![]( "Open in Gerrit")
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Alexander (Gerrit)
Jul 22, 2026, 11:42:13 PMJul 22
to goph...@pubsubhelper.golang.org, golang...@luci-project-accounts.iam.gserviceaccount.com, Nicholas Husin, golang-co...@googlegroups.com
Attention needed from Ian Alexander and Nicholas Husin
Ian Alexander voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ian Alexander
| Commit-Queue | +1 |
TryBots failure for `gotip-wasip1-wasm_wazero` looks real?
Ian Alexander
Had to skip the test on wazero. LMK if you don't like the checking of GO_BUILDER_NAME.
Related details
Attention is currently required from:
- Ian Alexander
- Nicholas Husin
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Jul 23, 2026, 12:54:01 PMJul 23
to Ian Alexander, goph...@pubsubhelper.golang.org, golang...@luci-project-accounts.iam.gserviceaccount.com, golang-co...@googlegroups.com
Attention needed from Ian Alexander
Nicholas Husin added 3 comments![]( "Open in Gerrit")
Commit Message
Line 15, Patchset 6 (Latest):
in the unmarshal method. When a custom UnmarshalXML method calledNicholas Husin . unresolved
nit: extra space, probably from automatic line wrapping?
Same before "This allowed documents".
File src/encoding/xml/read.go
Line 210, Patchset 6 (Parent):
d.unmarshalDepth++Nicholas Husin . unresolved
Should these be removed?
I think this will cause the following to fail:
```
type manualNode struct {
Child *manualNode
}
func (m *manualNode) UnmarshalXML(d *xml.Decoder, start xml.StartElement) error {
for {
tok, err := d.Token()
if err != nil {
return err
}
switch t := tok.(type) {
case xml.StartElement:
var child manualNode
if err := d.DecodeElement(&child, &t); err != nil {
return err
}
m.Child = &child
case xml.EndElement:
return nil
}
}
}func TestManual(t *testing.T) {
depth := 10001
payload := bytes.Join([][]byte{
bytes.Repeat([]byte("<a>"), depth),
bytes.Repeat([]byte("</a>"), depth),
}, nil)var node manualNode
err := xml.Unmarshal(payload, &node)
if err == nil {
t.Fatalf("Unmarshal depth limit bypassed: unmarshaled 10,001 levels deep without error")
}
}
```
If we do not increment `d.unmarshalDepth` here, when someone uses `Token` manually before calling `DecodeElement`, we would fail to account for the depth increase.
Although, just adding back `d.unmarshalDepth++` and `d.unmarshalDepth--` increase here would mean that when someones calls `Decode` without `Token`, the depth would increase by 2 instead of 1 I think. Maybe that's fine since our depth limit is pretty high? It would probably be confusing if a user ever hits the limit though.
File src/encoding/xml/read_test.go
Line 1187, Patchset 6 (Latest):
type Section struct {
Sub *Section `xml:"section"`
Custom *Extension `xml:"extension"`
}
type Extension struct {
Body Section
}Nicholas Husin . unresolved
nit: no need to export `Section` and `Extension`. Doesn't matter that much since it's a test, but might as well be consistent. Renaming `Section` and `Extension` to be more descriptive for the test might be nice too.
(I guess this was copied from my comment, my bad.)
Related details
Attention is currently required from:
- Ian Alexander
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Alexander (Gerrit)
Jul 23, 2026, 4:42:53 PMJul 23
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Ian Alexander
Ian Alexander uploaded new patchset![]( "Open in Gerrit")
Ian Alexander uploaded patch set #7 to this change.
Following approvals got outdated and were removed:
- TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
Attention is currently required from:
- Ian Alexander
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Alexander (Gerrit)
Jul 23, 2026, 4:44:42 PMJul 23
to goph...@pubsubhelper.golang.org, golang...@luci-project-accounts.iam.gserviceaccount.com, Nicholas Husin, golang-co...@googlegroups.com
Ian Alexander voted and added 2 comments![]( "Open in Gerrit")
Votes added by Ian Alexander
| Commit-Queue | +1 |
2 comments
Commit Message
Line 15, Patchset 6:
in the unmarshal method. When a custom UnmarshalXML method calledNicholas Husin . resolved
nit: extra space, probably from automatic line wrapping?
Same before "This allowed documents".
Ian Alexander
This is a byproduct of typewriter training in my youth, in which double-spaces after terminal punctuation were taught. With the advent of proportional fonts in computing, it seems to be now viewed as outdated (but not necessarily "incorrect").
File src/encoding/xml/read_test.go
Line 1187, Patchset 6:
type Section struct {
Sub *Section `xml:"section"`
Custom *Extension `xml:"extension"`
}
type Extension struct {
Body Section
}Nicholas Husin . resolved
nit: no need to export `Section` and `Extension`. Doesn't matter that much since it's a test, but might as well be consistent. Renaming `Section` and `Extension` to be more descriptive for the test might be nice too.
(I guess this was copied from my comment, my bad.)
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Alexander (Gerrit)
Jul 23, 2026, 4:56:06 PMJul 23
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Ian Alexander
Ian Alexander uploaded new patchset![]( "Open in Gerrit")
Attention is currently required from:
- Ian Alexander
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Alexander (Gerrit)
Jul 23, 2026, 4:56:49 PMJul 23
to goph...@pubsubhelper.golang.org, golang...@luci-project-accounts.iam.gserviceaccount.com, Nicholas Husin, golang-co...@googlegroups.com
Ian Alexander voted Commit-Queue+1![]( "Open in Gerrit")
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Roland Shoemaker (Gerrit)
Jul 23, 2026, 5:03:42 PMJul 23
to Ian Alexander, goph...@pubsubhelper.golang.org, golang...@luci-project-accounts.iam.gserviceaccount.com, Nicholas Husin, golang-co...@googlegroups.com
Roland Shoemaker added 5 comments![]( "Open in Gerrit")
File src/encoding/xml/read.go
Roland Shoemaker
Yeah I think we need to retain this, its interaction with the `unmarshal` `depth` argument is making everything more complicated though.
Line 323, Patchset 6:
oldDepth := d.unmarshalDepth
d.unmarshalDepth = depth
defer func() { d.unmarshalDepth = oldDepth }()Roland Shoemaker . unresolved
I think we should probably just do away with the `depth` argument and move to always using `unmarshalDepth`, since it makes following this all a bit confusing.
This way we'd remove the depth arg from unmarshal, unmarshalInterface,and unmarshalPath, and add
unmarshalDepth++
defer unmarshalDepth--
to the preamble of each method.
File src/encoding/xml/read_test.go
Line 1162, Patchset 6:
depth: maxDepth,Roland Shoemaker . unresolved
If we switch to using only unmarshalDepth I believe this would need to be `(maxDepth/4)` and the below would be `(maxDepth/4)+1`, since the DecodeElement loop recurses in unmarshal ~4 times.
Line 1226, Patchset 6:
t.Fatalf("FAILED: Unmarshaled 15,003 levels deep without error (depth limit bypassed)")Roland Shoemaker . unresolved
`Fatal`, since there are no formatting args.
Line 1226, Patchset 6:
t.Fatalf("FAILED: Unmarshaled 15,003 levels deep without error (depth limit bypassed)")Roland Shoemaker . unresolved
You can remove this prefix.
Related details
Attention set is empty
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Alexander (Gerrit)
Jul 23, 2026, 5:06:40 PMJul 23
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Ian Alexander
Ian Alexander uploaded new patchset![]( "Open in Gerrit")
Attention is currently required from:
- Ian Alexander
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Alexander (Gerrit)
Jul 23, 2026, 5:07:34 PMJul 23
to goph...@pubsubhelper.golang.org, golang...@luci-project-accounts.iam.gserviceaccount.com, Roland Shoemaker, Nicholas Husin, golang-co...@googlegroups.com
Ian Alexander voted Commit-Queue+1![]( "Open in Gerrit")
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Alexander (Gerrit)
Jul 23, 2026, 5:14:20 PMJul 23
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Ian Alexander
Ian Alexander uploaded new patchset![]( "Open in Gerrit")
Attention is currently required from:
- Ian Alexander
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Alexander (Gerrit)
Jul 23, 2026, 5:14:36 PMJul 23
to goph...@pubsubhelper.golang.org, golang...@luci-project-accounts.iam.gserviceaccount.com, Roland Shoemaker, Nicholas Husin, golang-co...@googlegroups.com
Ian Alexander voted and added 2 comments![]( "Open in Gerrit")
Votes added by Ian Alexander
| Commit-Queue | +1 |
2 comments
File src/encoding/xml/read_test.go
Line 1226, Patchset 6:
t.Fatalf("FAILED: Unmarshaled 15,003 levels deep without error (depth limit bypassed)")Roland Shoemaker . resolved
`Fatal`, since there are no formatting args.
Ian Alexander
Done
Line 1226, Patchset 6:
t.Fatalf("FAILED: Unmarshaled 15,003 levels deep without error (depth limit bypassed)")Roland Shoemaker . resolved
You can remove this prefix.
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Alexander (Gerrit)
Jul 23, 2026, 5:32:25 PMJul 23
to goph...@pubsubhelper.golang.org, golang...@luci-project-accounts.iam.gserviceaccount.com, Roland Shoemaker, Nicholas Husin, golang-co...@googlegroups.com
Attention needed from Nicholas Husin and Roland Shoemaker
Ian Alexander added 3 comments![]( "Open in Gerrit")
File src/encoding/xml/read.go
Line 210, Patchset 6 (Parent):
d.unmarshalDepth++Nicholas Husin . resolved
Ian Alexander
I think I have a better solution for tracking depth by updating it within the push and pop methods on the decoder.
Line 323, Patchset 6:
oldDepth := d.unmarshalDepth
d.unmarshalDepth = depth
defer func() { d.unmarshalDepth = oldDepth }()Roland Shoemaker . resolved
I think we should probably just do away with the `depth` argument and move to always using `unmarshalDepth`, since it makes following this all a bit confusing.
This way we'd remove the depth arg from unmarshal, unmarshalInterface,and unmarshalPath, and add
unmarshalDepth++
defer unmarshalDepth--to the preamble of each method.
Ian Alexander
I think I have a better solution for tracking depth by updating it within the push and pop methods on the decoder.
File src/encoding/xml/read_test.go
Line 1162, Patchset 6:
depth: maxDepth,Roland Shoemaker . resolved
If we switch to using only unmarshalDepth I believe this would need to be `(maxDepth/4)` and the below would be `(maxDepth/4)+1`, since the DecodeElement loop recurses in unmarshal ~4 times.
Attention is currently required from:
- Nicholas Husin
- Roland Shoemaker
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Jul 28, 2026, 3:21:10 PM (12 days ago) Jul 28
to Ian Alexander, goph...@pubsubhelper.golang.org, golang...@luci-project-accounts.iam.gserviceaccount.com, Roland Shoemaker, golang-co...@googlegroups.com
Attention needed from Ian Alexander and Roland Shoemaker
Nicholas Husin added 1 comment![]( "Open in Gerrit")
Commit Message
Line 22, Patchset 10 (Latest):
Fixes #80481Nicholas Husin . unresolved
Reminder to cherrypick to 1.27 release branch too.
Related details
Attention is currently required from:
- Ian Alexander
- Roland Shoemaker
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Roland Shoemaker (Gerrit)
Jul 28, 2026, 3:40:11 PM (12 days ago) Jul 28
to Ian Alexander, goph...@pubsubhelper.golang.org, golang...@luci-project-accounts.iam.gserviceaccount.com, Nicholas Husin, golang-co...@googlegroups.com
Attention needed from Ian Alexander
Roland Shoemaker voted and added 11 comments![]( "Open in Gerrit")
Votes added by Roland Shoemaker
| Code-Review | +2 |
11 comments
Patchset-level comments
File-level comment, Patchset 10 (Latest):
Roland Shoemaker . resolved
From my reading this maintains our old behavior while significantly improving the coverage. Since we now essentially increment the depth every time we parse a start element, I think this should be a concrete fix. Thanks!
Commit Message
Line 19, Patchset 10 (Latest):
interleaving elements with and without custom unmarshalers.Roland Shoemaker . unresolved
Can you include a note about the new behavior, and why we decided on it vs. the old implementation.
File src/encoding/xml/read.go
Line 550, Patchset 10 (Latest):
// unmarshalPath can call unmarshal, so we need to pass the depth through so thatRoland Shoemaker . unresolved
Stale comment.
Line 751, Patchset 10 (Latest):
// the recursion depth of unmarshalPath is limited to the path length specifiedRoland Shoemaker . unresolved
Stale comment.
File src/encoding/xml/read_test.go
Line 1147, Patchset 10 (Latest):
builder := os.Getenv("GO_BUILDER_NAME")Roland Shoemaker . unresolved
We should use the same wasm check/bypass in all of the depth tests (new and old) consistently.
Line 1225, Patchset 10 (Latest):
if err == nil {Roland Shoemaker . unresolved
Check we are returning errUnmarshalDepth here and throughout, so we know we are returning the correct error.
Line 1226, Patchset 10 (Latest):
t.Fatal("Unmarshaled 15,003 levels deep without error (depth limit bypassed)")Roland Shoemaker . unresolved
Use the constant, so if it changes the error message doesn't become confusing.
Line 1253, Patchset 10 (Latest):
func TestManual(t *testing.T) {Roland Shoemaker . unresolved
TestRecursiveUnmarshalInterfaceDepth
Line 1262, Patchset 10 (Latest):
if err == nil {Roland Shoemaker . unresolved
Same here, check for errUnmarshalDepth.
File src/encoding/xml/xml.go
Line 551, Patchset 10 (Latest):
if s := d.stk; s != nil && s.kind == stkStart {Roland Shoemaker . unresolved
I think this breaks our previous behavior. The intention here (if I'm remembering correctly) is to prevent using RawToken from inside of custom UnmarshalXML methods. This change would break streaming Decoders, I believe. Probably we should just add a bool to Decoder that indicates if we're inside of UnmarshalXML and set/unset it appropriately, then check that here.
Related details
Attention is currently required from:
- Ian Alexander
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Alexander (Gerrit)
Jul 28, 2026, 5:11:42 PM (11 days ago) Jul 28
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Ian Alexander
Ian Alexander uploaded new patchset![]( "Open in Gerrit")
Ian Alexander uploaded patch set #11 to this change.
Following approvals got outdated and were removed:
- TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
Attention is currently required from:
- Ian Alexander
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Alexander (Gerrit)
Jul 28, 2026, 5:44:32 PM (11 days ago) Jul 28
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Ian Alexander
Ian Alexander uploaded new patchset![]( "Open in Gerrit")
Ian Alexander uploaded patch set #12 to this change.
Related details
Attention is currently required from:
- Ian Alexander
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Alexander (Gerrit)
Jul 28, 2026, 6:31:39 PM (11 days ago) Jul 28
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Ian Alexander
Ian Alexander uploaded new patchset![]( "Open in Gerrit")
Ian Alexander uploaded patch set #13 to this change.
Related details
Attention is currently required from:
- Ian Alexander
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Alexander (Gerrit)
Jul 28, 2026, 6:31:57 PM (11 days ago) Jul 28
to goph...@pubsubhelper.golang.org, Roland Shoemaker, golang...@luci-project-accounts.iam.gserviceaccount.com, Nicholas Husin, golang-co...@googlegroups.com
Ian Alexander voted and added 10 comments![]( "Open in Gerrit")
Votes added by Ian Alexander
| Commit-Queue | +1 |
10 comments
Commit Message
Line 19, Patchset 10:
interleaving elements with and without custom unmarshalers.Roland Shoemaker . resolved
Can you include a note about the new behavior, and why we decided on it vs. the old implementation.
Ian Alexander
Done
File src/encoding/xml/read.go
Line 550, Patchset 10:
// unmarshalPath can call unmarshal, so we need to pass the depth through so thatRoland Shoemaker . resolved
Ian AlexanderStale comment.
Done
Line 751, Patchset 10:
// the recursion depth of unmarshalPath is limited to the path length specifiedRoland Shoemaker . resolved
Ian AlexanderStale comment.
Done
File src/encoding/xml/read_test.go
Line 1147, Patchset 10:
builder := os.Getenv("GO_BUILDER_NAME")Roland Shoemaker . unresolved
We should use the same wasm check/bypass in all of the depth tests (new and old) consistently.
Ian Alexander
The wazero builder cannot create the executable because of its limited stack size. I'd rather have the test run on the remaining wasm builders. I added a comment explaining. WDYT?
Check we are returning errUnmarshalDepth here and throughout, so we know we are returning the correct error.
Ian Alexander
Done
Line 1226, Patchset 10:
t.Fatal("Unmarshaled 15,003 levels deep without error (depth limit bypassed)")Roland Shoemaker . resolved
Use the constant, so if it changes the error message doesn't become confusing.
Ian Alexander
Done
Line 1253, Patchset 10:
func TestManual(t *testing.T) {Roland Shoemaker . resolved
Ian AlexanderTestRecursiveUnmarshalInterfaceDepth
Done
Line 1254, Patchset 10:
depth := 10001Roland Shoemaker . resolved
Ian AlexandermaxUnmarshalDepth+1
Done
Same here, check for errUnmarshalDepth.
Ian Alexander
Done
File src/encoding/xml/xml.go
I think this breaks our previous behavior. The intention here (if I'm remembering correctly) is to prevent using RawToken from inside of custom UnmarshalXML methods. This change would break streaming Decoders, I believe. Probably we should just add a bool to Decoder that indicates if we're inside of UnmarshalXML and set/unset it appropriately, then check that here.
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Alexander (Gerrit)
Jul 28, 2026, 6:48:11 PM (11 days ago) Jul 28
to goph...@pubsubhelper.golang.org, golang...@luci-project-accounts.iam.gserviceaccount.com, Roland Shoemaker, Nicholas Husin, golang-co...@googlegroups.com
Attention needed from Roland Shoemaker
Ian Alexander voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ian Alexander
| Commit-Queue | +1 |
1 comment
File src/encoding/xml/read_test.go
Line 1147, Patchset 10:
builder := os.Getenv("GO_BUILDER_NAME")Roland Shoemaker . resolved
Ian AlexanderWe should use the same wasm check/bypass in all of the depth tests (new and old) consistently.
The wazero builder cannot create the executable because of its limited stack size. I'd rather have the test run on the remaining wasm builders. I added a comment explaining. WDYT?
Ian Alexander
Done
Related details
Attention is currently required from:
- Roland Shoemaker
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Alexander (Gerrit)
Jul 28, 2026, 7:04:44 PM (11 days ago) Jul 28
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Roland Shoemaker
Ian Alexander uploaded new patchset![]( "Open in Gerrit")
Ian Alexander uploaded patch set #14 to this change.
Related details
Attention is currently required from:
- Roland Shoemaker
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Alexander (Gerrit)
Jul 29, 2026, 12:32:40 PM (11 days ago) Jul 29
to goph...@pubsubhelper.golang.org, golang...@luci-project-accounts.iam.gserviceaccount.com, Roland Shoemaker, Nicholas Husin, golang-co...@googlegroups.com
Ian Alexander added 1 comment![]( "Open in Gerrit")
Commit Message
Reminder to cherrypick to 1.27 release branch too.
Ian Alexander
Acknowledged
Related details
Attention set is empty
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Alexander (Gerrit)
Jul 29, 2026, 12:32:50 PM (11 days ago) Jul 29
to goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, golang...@luci-project-accounts.iam.gserviceaccount.com, Roland Shoemaker, Nicholas Husin, golang-co...@googlegroups.com
Ian Alexander submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
10 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: src/encoding/xml/read.go
Insertions: 4, Deletions: 4.
@@ -207,6 +207,10 @@
// Record that decoder must stop at end tag corresponding to start.
d.pushEOF()
+ savedInUnmarshalXML := d.inUnmarshalXML
+ d.inUnmarshalXML = true
+ defer func() { d.inUnmarshalXML = savedInUnmarshalXML }()
+
err := val.UnmarshalXML(d, *start)
if err != nil {
d.popEOF()
@@ -547,8 +551,6 @@
case StartElement:
consumed := false
if sv.IsValid() {
- // unmarshalPath can call unmarshal, so we need to pass the depth through so that
- // we can continue to enforce the maximum recursion limit.
consumed, err = d.unmarshalPath(tinfo, sv, nil, &t)
if err != nil {
return err
@@ -748,8 +750,6 @@
}
switch t := tok.(type) {
case StartElement:
- // the recursion depth of unmarshalPath is limited to the path length specified
- // by the struct field tag, so we don't increment the depth here.
consumed2, err := d.unmarshalPath(tinfo, sv, parents, &t)
if err != nil {
return true, err
```
```
The name of the file: src/encoding/xml/xml.go
Insertions: 18, Deletions: 17.
@@ -197,22 +197,23 @@
// the attribute xmlns="DefaultSpace".
DefaultSpace string
- r io.ByteReader
- t TokenReader
- buf bytes.Buffer
- saved *bytes.Buffer
- stk *stack
- stkDepth int
- free *stack
- needClose bool
- toClose Name
- nextToken Token
- nextByte int
- ns map[string]string
- err error
- line int
- linestart int64
- offset int64
+ r io.ByteReader
+ t TokenReader
+ buf bytes.Buffer
+ saved *bytes.Buffer
+ stk *stack
+ stkDepth int
+ free *stack
+ needClose bool
+ toClose Name
+ nextToken Token
+ nextByte int
+ ns map[string]string
+ err error
+ line int
+ linestart int64
+ offset int64
+ inUnmarshalXML bool
}
// NewDecoder creates a new XML parser reading from r.
@@ -548,7 +549,7 @@
// start and end elements match and does not translate
// name space prefixes to their corresponding URLs.
func (d *Decoder) RawToken() (Token, error) {
- if s := d.stk; s != nil && s.kind == stkStart {
+ if d.inUnmarshalXML {
return nil, errRawToken
}
return d.rawToken()
```
```
The name of the file: src/encoding/xml/read_test.go
Insertions: 23, Deletions: 6.
@@ -1144,6 +1144,8 @@
}
func TestDecodeElementRecursion(t *testing.T) {
+ // The wazero builder is unable to build the test binary due to its small
+ // stack size.
builder := os.Getenv("GO_BUILDER_NAME")
if testing.Short() || strings.Contains(builder, "wazero") {
t.Skip("test requires significant memory")
@@ -1222,8 +1224,8 @@
var node standardNode
err := Unmarshal(buf.Bytes(), &node)
- if err == nil {
- t.Fatal("Unmarshaled 15,003 levels deep without error (depth limit bypassed)")
+ if err != errUnmarshalDepth {
+ t.Fatalf("Unexpected error: got %q want %q", err, errUnmarshalDepth)
}
}
@@ -1250,8 +1252,8 @@
}
}
-func TestManual(t *testing.T) {
- depth := 10001
+func TestRecursiveUnmarshalInterfaceDepth(t *testing.T) {
+ depth := maxUnmarshalDepth + 1
payload := bytes.Join([][]byte{
bytes.Repeat([]byte("<a>"), depth),
bytes.Repeat([]byte("</a>"), depth),
@@ -1259,7 +1261,22 @@
var node manualNode
err := Unmarshal(payload, &node)
- if err == nil {
- t.Fatalf("Unmarshal depth limit bypassed: unmarshaled %d levels deep without error", depth)
+ if err != errUnmarshalDepth {
+ t.Fatalf("Unexpected error: got %q want %q", err, errUnmarshalDepth)
+ }
+}
+
+type rawTokenNode struct{}
+
+func (r *rawTokenNode) UnmarshalXML(d *Decoder, start StartElement) error {
+ _, err := d.RawToken()
+ return err
+}
+
+func TestUnmarshalXMLRawToken(t *testing.T) {
+ var node rawTokenNode
+ err := Unmarshal([]byte("<a></a>"), &node)
+ if err != errRawToken {
+ t.Fatalf("UnmarshalXML calling RawToken: got error %v, want %v", err, errRawToken)
}
}
```
Change information
Commit message:
encoding/xml: fix depth processing in (*Decoder).unmarshal
(*Decoder).DecodeElement bypassed recursion depth guard by unilaterally
passing the constant 0 to (*Decoder).unmarshal. Previously, unmarshal
depth was tracked via a depth parameter passed down the call stack,
which manual loops inside custom UnmarshalXML methods could bypass.
This change simplifies depth tracking by maintaining a stack depth value
that is adjusted as start elements are pushed / popped. This eliminates
the need to reason about and synchronize two different values storing
the unmarshal depth.
Additionally, guarding (*Decoder).RawToken using parser stack state
broke streaming decoders reading tokens within open XML elements. This
change simplifies the guard by adding an explicit inUnmarshalXML flag.
Thanks to Moran Omer (GitHub: moraneus) for reporting this issue.
Fixes #80481
Fixes CVE-2026-56859
Change-Id: I5e89ebe9101b03545520aba5ea99fbf15bdf0395
Reviewed-on: https://go-review.googlesource.com/c/go/+/803320
Reviewed-by: Roland Shoemaker <rol...@golang.org>
LUCI-TryBot-Result: golang...@luci-project-accounts.iam.gserviceaccount.com <golang...@luci-project-accounts.iam.gserviceaccount.com>
Files:
- M src/encoding/xml/read.go
- M src/encoding/xml/read_test.go
- M src/encoding/xml/xml.go
Change size: M
Delta: 3 files changed, 176 insertions(+), 17 deletions(-)
Branch: refs/heads/master
Submit Requirements:
Code-Review: +2 by Roland Shoemaker
TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages