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/asn1: error instead of panic on invalid value to Unmarshal #41509

Closed
KimMachineGun opened this issue Sep 20, 2020 · 8 comments
Closed

encoding/asn1: error instead of panic on invalid value to Unmarshal #41509

KimMachineGun opened this issue Sep 20, 2020 · 8 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.16

Comments

@KimMachineGun
Copy link
Contributor

KimMachineGun commented Sep 20, 2020 •
edited

Most packages for encoding return an error when decoding(or unamrshaling) fails due to an invalid argument.
But sadly, the encoding/asn1 panics instead of returning an error, and I think it's pretty dangerous because it is unpredictable and fragile.

So, I propose that asn1.Unmarshal should return an error on invalid argument.

Since this proposal can make breaking changes, I'm not sure if this poposal comply to the go 1 compatibility promise. However, even if this proposal is not accepted, it may be worth mentioning that 'a panic can occur during unmarshaling' in the encoding/asn1 document.

PR

PR for this proposal: #41485
In this PR, the asn1.Unmarshal verifies that argument is non-nil pointer. And if it isn't, asn1.Unmarshal will return a new error InvalidUnmarshalError like encoding/json.

Examples

https://play.golang.org/p/7ChlnCY_ioI

package main

import (
	"encoding/asn1"
)

func main() {
	// panic: reflect: call of reflect.Value.Elem on zero Value
	asn1.Unmarshal([]byte{0x05, 0x00}, nil)

	// panic: reflect: call of reflect.Value.Elem on struct Value
	asn1.Unmarshal([]byte{0x05, 0x00}, asn1.RawValue{})

	// panic: reflect: call of reflect.Value.Type on zero Value
	var p *asn1.RawValue
	asn1.Unmarshal([]byte{0x05, 0x00}, p)
}
@gopherbot gopherbot added this to the Proposal milestone Sep 20, 2020
@odeke-em odeke-em mentioned this issue Sep 20, 2020
Closed
@gopherbot
Copy link

Change https://golang.org/cl/255881 mentions this issue: encoding/asn1: error instead of panic on invalid value to Unmarshal

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Sep 21, 2020
@rolandshoemaker
Copy link
Member

Seems like a reasonable change. This would only break code that relied on a panic happening to prevent brokenness, which seems less than ideal in the first place. Is there a precedent for holding up changes that fix this kind of behavior (there is a similar issue in crypto/x509 as well).

@slrz
Copy link

slrz commented Sep 21, 2020

Most packages for encoding return an error when decoding(or unamrshaling) fails due to an invalid argument.
But sadly, the encoding/asn1 panics instead of returning an error, and I think it's pretty dangerous because it is unpredictable and fragile.

It seems to me that a panic can only be triggered by passing a bogus second argument to Unmarshal. That's a programmer error and thus should panic. It cannot happen with correct code, so I don't see what's unpredictable about it.

Now, if you noticed a panic that can be triggered by passing a garbage first argument, things would be entirely different: the encoded data may have been received over the network or otherwise originated outside your program and should not be able to cause a panic.

@KimMachineGun
Copy link
Contributor Author

KimMachineGun commented Sep 22, 2020 •
edited

Most packages for encoding return an error when decoding(or unamrshaling) fails due to an invalid argument.
But sadly, the encoding/asn1 panics instead of returning an error, and I think it's pretty dangerous because it is unpredictable and fragile.

It seems to me that a panic can only be triggered by passing a bogus second argument to Unmarshal. That's a programmer error and thus should panic. It cannot happen with correct code, so I don't see what's unpredictable about it.

Now, if you noticed a panic that can be triggered by passing a garbage first argument, things would be entirely different: the encoded data may have been received over the network or otherwise originated outside your program and should not be able to cause a panic.

@slrz
I can't agree that programmer error should panic, and even if so, I still think it's a dangerous point because people would expect asn1.Unmarshal to return an error like other Unmarshal functions.
(Sorry for the ambiguous use of 'predictable'. I meant that it could make unexpected result.)

Since humans can make mistakes at any time, I think all programmer errors should be reported at compile time or handled gracefully.

But, this function cannot report invalid argument at compile time, so it should provide a way to handle error gracefully. I think panic is not that graceful, and current panic message is also not informative at all.

@FiloSottile
Copy link
Contributor

Especially in packages that are exposed to outside data, the least we panic the better. Behavior when the library is misused is not covered by the compatibility promise, so I don't think this needs a proposal.

@KimMachineGun
Copy link
Contributor Author

Especially in packages that are exposed to outside data, the least we panic the better. Behavior when the library is misused is not covered by the compatibility promise, so I don't think this needs a proposal.

@FiloSottile
Thank you for your opinion. So, shall I close this issue myself?

@FiloSottile
Copy link
Contributor

No need, the issue will get closed as fixed automatically when the CL is merged.

@odeke-em odeke-em changed the title proposal: encoding/asn1: error instead of panic on invalid value to Unmarshal encoding/asn1: error instead of panic on invalid value to Unmarshal Sep 29, 2020
@odeke-em odeke-em removed this from Incoming in Proposals (old) Sep 29, 2020
@odeke-em odeke-em removed the Proposal label Sep 29, 2020
@odeke-em odeke-em modified the milestones: Proposal, Go1.16 Sep 29, 2020
@KimMachineGun KimMachineGun mentioned this issue Nov 29, 2020
Closed
@gopherbot
Copy link

Change https://golang.org/cl/267057 mentions this issue: doc/go1.16: document new behavior of asn1.Unmarshal on invalid argument

@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Dec 1, 2020
gopherbot pushed a commit that referenced this issue Dec 3, 2020
@KimMachineGun @ianlancetaylor
doc/go1.16: document new behavior of asn1.Unmarshal on invalid argument
da54dfb 
For #41509

Change-Id: Ie761c428710d15848cb80ffd2d85de747113f2d4
GitHub-Last-Rev: 0554162
GitHub-Pull-Request: #42315
Reviewed-on: https://go-review.googlesource.com/c/go/+/267057
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Dec 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.16
Development

Successfully merging a pull request may close this issue.

encoding/asn1: error instead of panic on invalid value to Unmarshal KimMachineGun/go
7 participants
@FiloSottile @dmitshur @odeke-em @rolandshoemaker @slrz @gopherbot @KimMachineGun