Skip to content

encoding/xml: Unmarshal ignores error return from Decoder.unmarshalAttr #16158

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
Closed
ericlagergren opened this issue Jun 22, 2016 · 8 comments
Closed

encoding/xml: Unmarshal ignores error return from Decoder.unmarshalAttr #16158

ericlagergren opened this issue Jun 22, 2016 · 8 comments
Labels
FrozenDueToAge
Milestone
Go1.8

Comments

@ericlagergren
Copy link
Contributor

ericlagergren commented Jun 22, 2016 •
edited
Loading

Please answer these questions before submitting your issue. Thanks!

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

1.6.2

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

GOOS=GNU/Linux
GOARCH=amd64

  1. What did you do?
    If possible, provide a recipe for reproducing the error.
    A complete runnable program is good.
    A link on play.golang.org is best.

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

  1. What did you expect to see?

One of two things, in order of preference:

1.) Since C is one byte long I'd expect to see foo.B set to 0x43 (and also error out if the attribute is longer than one byte long.)
2.) An error since C is an invalid number and Decoder.unmarshalAttr calls strconv.ParseUint yet decides to ignore the return value of copyValue.

  1. What did you see instead?

Nothing -- it doesn't touch foo.B and foo.B remains 0x0.
@ericlagergren
Copy link
Contributor Author

ericlagergren commented Jun 22, 2016 •
edited
Loading

The offending commit (if it's in any way relevant to why the return value of copyValue is ignored): 84b0842

I can submit a CL with with either fix 1 or fix 2 if you'd like.

The more I think about it, option 1 sounds less attractive and option 2 more correct.

@ianlancetaylor
Copy link
Member

The fix is to change Decoder.unmarshalAttr to not ignore the error return of copyValue. The obvious fix causes one of the testsuite tests to fail.

This has worked this way since 1.0.3, so leaving for 1.8.

@ianlancetaylor ianlancetaylor added this to the Go1.8 milestone Jun 22, 2016
@ericlagergren
Copy link
Contributor Author

ericlagergren commented Jun 23, 2016 •
edited
Loading

Is this covered under the Go 1.0 promise? (Curious since this has been the behavior for ~7 releases, even though it's obviously incorrect.)
On Wed, Jun 22, 2016 at 4:59 PM Ian Lance Taylor notifications@github.com
wrote:

The fix is to change Decoder.unmarshalAttr to not ignore the error return
of copyValue. The obvious fix causes one of the testsuite tests to fail.

This has worked this way since 1.0.3, so leaving for 1.8.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#16158 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AFnwZ5cDWN_16HAPxBQ-ZFRy5O42ObYDks5qOcxrgaJpZM4I8RcO
.

@ericlagergren ericlagergren changed the title encoding/xml: Unmarshal improperly decodes attr into byte encoding/xml: Unmarshal ignores error return from Decoder.unmarshalAttr Jun 23, 2016
@ianlancetaylor
Copy link
Member

This kind of thing is not strictly covered by the Go 1 promise. We are permitted to fix bugs. However, if fixing this bug causes packages to fail, we might decide that it's not worth fixing.

@ericlagergren
Copy link
Contributor Author

Okay, I'll go ahead with the CL then. I noticed the failed test too—is that
in the scope of this issue or not?
On Wed, Jun 22, 2016 at 6:15 PM Ian Lance Taylor notifications@github.com
wrote:

This kind of thing is not strictly covered by the Go 1 promise. We are
permitted to fix bugs. However, if fixing this bug causes packages to fail,
we might decide that it's not worth fixing.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#16158 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AFnwZ-v5-JKgZ9po_sX8giwtqpW9Us7Hks5qOd47gaJpZM4I8RcO
.

@ianlancetaylor
Copy link
Member

I'm not quite sure what you are asking. Certainly all the tests must pass when the CL is committed.

@ericlagergren
Copy link
Contributor Author

Yes, of course.

The failing test could be indicative of another bug in the xml package, or
it could just be a small issue with the test.

My question was about the former and whether it'd require a separate
issue or not.
On Wed, Jun 22, 2016 at 9:27 PM Ian Lance Taylor notifications@github.com
wrote:

I'm not quite sure what you are asking. Certainly all the tests must pass
when the CL is committed.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#16158 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AFnwZ-HTlSsPPlaGoOb42JEP-HQhLVlsks5qOgs3gaJpZM4I8RcO
.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/27455 mentions this issue.

@golang golang locked and limited conversation to collaborators Aug 22, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.8
Development

No branches or pull requests
3 participants
@ianlancetaylor @ericlagergren @gopherbot