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
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@empijei
Copy link
Contributor

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

Thank you for this change request @empijei!

I'll kindly page @FiloSottile @rsc

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 19, 2018
@bcmills bcmills added this to the Go1.13 milestone Nov 19, 2018
@bcmills bcmills added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 19, 2018
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 19, 2018
@odeke-em
Copy link
Member

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

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

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
tomocy pushed 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 pushed 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.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

6 participants