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

fix possible nil before casting #5017

Merged
merged 2 commits into from Dec 1, 2021
Merged

Conversation

charlesxsh
Copy link
Contributor

@charlesxsh charlesxsh commented Nov 30, 2021

This PR aims to fix a possible nil before casting to error.

--- FAIL: TestClusterManagerForwardsBalancerBuildOptions (1.00s)
panic: interface conversion: interface is nil, not error [recovered]
	panic: interface conversion: interface is nil, not error

goroutine 93 [running]:
testing.tRunner.func1.2(0xceec40, 0xc00020f230)
	/usr/local/go/src/testing/testing.go:1143 +0x335
testing.tRunner.func1(0xc0003be280)
	/usr/local/go/src/testing/testing.go:1146 +0x4c2
panic(0xceec40, 0xc00020f230)
	/usr/local/go/src/runtime/panic.go:965 +0x1b9
google.golang.org/grpc/xds/internal/balancer/clustermanager.TestClusterManagerForwardsBalancerBuildOptions(0xc0003be280)
	/fuzz/target/xds/internal/balancer/clustermanager/clustermanager_test.go:635 +0x68f
testing.tRunner(0xc0003be280, 0xe8bbb0)
	/usr/local/go/src/testing/testing.go:1193 +0xef
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:1238 +0x2b5

RELEASE NOTES: none

Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@easwars it looks like you originally wrote this - what do you think?

@@ -560,6 +560,9 @@ func TestClusterManagerForwardsBalancerBuildOptions(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer cancel()
if v, err := ccsCh.Receive(ctx); err != nil {
if v == nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this actually a guarantee here?

I think this is written incorrectly, because it's not actually validating v at all in the normal case (where there is no timeout). Probably what was meant was:

v, err := ccsCh.Receive(ctx)
if err != nil {
	t.Fatalf("timed out waiting for UpdateClientConnState result: %v", err)
}
if v != nil {
	t.Fatal(v)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes. We need to check err first.

@easwars
Copy link
Contributor

easwars commented Dec 1, 2021

@easwars it looks like you originally wrote this - what do you think?

Yup, good catch and fix I think.

@easwars easwars merged commit 46935b9 into grpc:master Dec 1, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants