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: Unmarshal ignores error return from Decoder.unmarshalAttr #16158

Closed
ericlagergren opened this Issue Jun 22, 2016 · 8 comments

Comments

Projects
None yet
3 participants
@ericlagergren
Copy link
Contributor

ericlagergren commented Jun 22, 2016

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

This comment has been minimized.

Copy link
Contributor Author

ericlagergren commented Jun 22, 2016

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

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Jun 22, 2016

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

This comment has been minimized.

Copy link
Contributor Author

ericlagergren commented Jun 23, 2016

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

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Jun 23, 2016

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

This comment has been minimized.

Copy link
Contributor Author

ericlagergren commented Jun 23, 2016

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

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Jun 23, 2016

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

@ericlagergren

This comment has been minimized.

Copy link
Contributor Author

ericlagergren commented Jun 23, 2016

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

This comment has been minimized.

Copy link

gopherbot commented Aug 20, 2016

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

@gopherbot gopherbot closed this in 2c58cb3 Aug 22, 2016

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