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

[KimMachineGun]

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]

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

[rolandshoemaker]

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]

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]

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]

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]

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]

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

[gopherbot]

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