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.

Sign up for GitHub

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

Jump to bottom

encoding/xml: crash on android/arm64 due to https://go.dev/cl/417062 #53814

Closed
mknyszek opened this issue Jul 12, 2022 · 13 comments
Closed

encoding/xml: crash on android/arm64 due to https://go.dev/cl/417062 #53814

mknyszek opened this issue Jul 12, 2022 · 13 comments
Assignees
@rolandshoemaker
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Go1.19

Comments

@mknyszek
Copy link
Contributor

This appears to be a crash in a test introduced by that CL.

CC @golang/security
@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 12, 2022
@mknyszek
Copy link
Contributor Author

It's a guess anyway, since that test caused problems on js/wasm. Could be something else though.

@mknyszek mknyszek added this to the Go1.19 milestone Jul 12, 2022
@mknyszek
Copy link
Contributor Author

Example failure: https://build.golang.org/log/a10e03286e0126e78bdb4bf7f17ebce513b4f1aa

@bcmills
Copy link
Contributor

bcmills commented Jul 12, 2022

It looks like it also broke at least plan9/arm.

@rolandshoemaker
Copy link
Member

Bah, these tests are a bit too memory intensive. This was discussed on the internal reviews but we didn't really have a way of testing it since there are no internal trybots (yet.) I think the test can just be rewritten to avoid this issue, at the expense of making the limit non-const.

@aclements
Copy link
Member

This failure isn't specific to android/arm64. The test could fail on any machine with limited memory and that platform just happens to expose it.

@mknyszek
Copy link
Contributor Author

@rolandshoemaker Do you plan to work on this before the release?

@rolandshoemaker
Copy link
Member

Yup, will do.

@rolandshoemaker rolandshoemaker self-assigned this Jul 13, 2022
@gopherbot
Copy link

Change https://go.dev/cl/417655 mentions this issue: encoding/xml: skip TestCVE202230633 for short tests

@cherrymui
Copy link
Member

Should we backport this? The android-amd64 builder are failing on the 1.17 and 1.18 release branches without this. Thanks.

cc @golang/release

@dmitshur
Copy link
Contributor

@cherrymui Sure, I think the backport request can be approved trivially. It's a test-only skip, very safe, needed for passing builders on the release branches.

@cherrymui
Copy link
Member

@gopherbot please backport this. This causes failures on release branches and is a test-only skip.

@gopherbot gopherbot mentioned this issue Jul 29, 2022
Closed
@gopherbot
Copy link

Backport issue(s) opened: #54127 (for 1.17), #54128 (for 1.18).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@gopherbot gopherbot mentioned this issue Jul 29, 2022
Closed
@dmitshur dmitshur added Testing An issue that has been verified to require only test changes, not just a test failure. NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 1, 2022
@gopherbot
Copy link

Change https://go.dev/cl/421094 mentions this issue: [release-branch.go1.18] encoding/xml: skip TestCVE202230633 for short tests

gopherbot pushed a commit that referenced this issue Aug 3, 2022
@rolandshoemaker @dmitshur
[release-branch.go1.18] encoding/xml: skip TestCVE202230633 for short…
fcdd099 
… tests

TestCVE202230633 uses a bunch of memory, and the input cannot be
feasibly reduced while maintaining the behavior hasn't regressed. This
test could be reasonably removed, but I'd rather keep it around if we
can.

Updates #53814.
Fixes #54128.

Change-Id: Ie8b3f306efd20b2d9c0fb73122c26351a55694c4
Reviewed-on: https://go-review.googlesource.com/c/go/+/417655
Run-TryBot: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
(cherry picked from commit 783ff7d)
Reviewed-on: https://go-review.googlesource.com/c/go/+/421094
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
@rolandshoemaker @jproberts
encoding/xml: skip TestCVE202230633 for short tests
178b813 
TestCVE202230633 uses a bunch of memory, and the input cannot be
feasibly reduced while maintaining the behavior hasn't regressed. This
test could be reasonably removed, but I'd rather keep it around if we
can.

Fixes golang#53814

Change-Id: Ie8b3f306efd20b2d9c0fb73122c26351a55694c4
Reviewed-on: https://go-review.googlesource.com/c/go/+/417655
Run-TryBot: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
@golang golang locked and limited conversation to collaborators Aug 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@rolandshoemaker rolandshoemaker

Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Go1.19
Development

No branches or pull requests
7 participants
@mknyszek @dmitshur @aclements @rolandshoemaker @bcmills @gopherbot @cherrymui