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

crypto/x509: (*Certificate).Equals panics for nil values #28743

Closed
empijei opened this issue Nov 12, 2018 · 5 comments
Closed

crypto/x509: (*Certificate).Equals panics for nil values #28743

empijei opened this issue Nov 12, 2018 · 5 comments
Assignees
Milestone

Comments

@empijei
Copy link
Contributor

@empijei empijei commented Nov 12, 2018

Most methods in Go do not work on nil values, and that is perfectly fine, but I feel like (*T).Equal(*T) should behave more or less like the == operator.

I'm proposing a change that looks like this:

// Current implementation
func (c *Certificate) Equal(other *Certificate) bool {
	return bytes.Equal(c.Raw, other.Raw)
}

// New implementation
func (c *Certificate) Equal(other *Certificate) bool {
	if c == nil || other == nil {
		return c == other
	}
	return bytes.Equal(c.Raw, other.Raw)
}

This is analogous to how the regex package does it:

func (x *Regexp) Equal(y *Regexp) bool {
	if x == nil || y == nil {
		return x == y
	}
	// [...]
@odeke-em
Copy link
Member

@odeke-em odeke-em commented Nov 12, 2018

Thank you for this change request @empijei!

I'll kindly page @FiloSottile @rsc

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Mar 11, 2019

@empijei would you like to send a CL and a test for this? I could have sent a CL but you've already proposed the fix so please go for it. Thank you.

@empijei
Copy link
Contributor Author

@empijei empijei commented Mar 12, 2019

Thanks, I will asap, but I'm currently stuck on #30776. Will probably submit a fix for this in the next days.

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Mar 12, 2019

Thanks, I will asap, but I'm currently stuck on #30776. Will probably submit a fix for this in the next days.

Cool, no rush. However, I don't think you need to be blocked by ./all.bash. If your fix just involves the nil comparisons and also has some tests, you can submit that but also you can run the tests just in crypto/x509.

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 12, 2019

Change https://golang.org/cl/167118 mentions this issue: crypto/x509: make (*Certificate).Equal not panic

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@gopherbot gopherbot closed this in 89865f8 Aug 28, 2019
tomocy added a commit to tomocy/go that referenced this issue Sep 1, 2019
The current implementation panics on nil certificates,
so introduce a nil check and early return true if both
are nil, false if only one is.

Fixes golang#28743

Change-Id: I71b0dee3e505d3ad562a4470ccc22c3a2579bc52
Reviewed-on: https://go-review.googlesource.com/c/go/+/167118
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
t4n6a1ka added a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
The current implementation panics on nil certificates,
so introduce a nil check and early return true if both
are nil, false if only one is.

Fixes golang#28743

Change-Id: I71b0dee3e505d3ad562a4470ccc22c3a2579bc52
Reviewed-on: https://go-review.googlesource.com/c/go/+/167118
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
@golang golang locked and limited conversation to collaborators Aug 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.