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/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
Milestone

Comments

@KimMachineGun
Copy link
Contributor

@KimMachineGun KimMachineGun commented Sep 20, 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.

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
Copy link

@gopherbot gopherbot commented Sep 20, 2020

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 Sep 21, 2020
@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented Sep 21, 2020

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 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 KimMachineGun commented Sep 22, 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.

@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
Member

@FiloSottile FiloSottile commented Sep 28, 2020

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

@KimMachineGun KimMachineGun commented Sep 28, 2020

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
Member

@FiloSottile FiloSottile commented Sep 28, 2020

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 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
@gopherbot gopherbot closed this in a28edbf Sep 29, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 29, 2020

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 label Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

7 participants
You can’t perform that action at this time.