Repository: grpc/grpc-go

Issue:
Panic seen in google.golang.org/grpc/internal/resolver/delegatingresolver.(*delegatingResolver).updateProxyResolverState
NOTE: if you are reporting is a potential security vulnerability or a crash,
please follow our CVE process at
https://github.com/grpc/proposal/blob/master/P4-grpc-cve-process.md instead of
filing an issue here.

Please see the FAQ in our main README.md, then answer the questions below
before submitting your issue.

### What version of gRPC are you using?
v1.71.0

### What version of Go are you using (`go version`)?
1.23.7

### What operating system (Linux, Windows, …) and version?
Linux

### What did you do?
If possible, provide a recipe for reproducing the error.

We are trying to connect to non existing server like test:8882
There is no host/domain like test in the setup. Its just a dummy value to execute some piece of code in unit test.

### What did you expect to see?
Test works fine as earlier and there is no panic. v1.69.4 and previous versions did not face this panic issue. So seems to be seeing this post v1.69.4 .

### What did you see instead?
Below panic is seen in one of our unit-test:
```
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x55e73a5a3456]

goroutine 1007 [running]:
google.golang.org/grpc/internal/resolver/delegatingresolver.(*delegatingResolver).updateProxyResolverState(0xc000fc2690, {{0xc00095a080, 0x1, 0x1}, {0x0, 0x0, 0x0}, 0x0, 0x0})
	/opt/workspace/test-app/pkg/mod/google.golang.org/grpc@v1.71.0/internal/resolver/delegatingresolver/delegatingresolver.go:274 +0x2b6
google.golang.org/grpc/internal/resolver/delegatingresolver.(*wrappingClientConn).UpdateState(0xc000fd0300?, {{0xc00095a080, 0x1, 0x1}, {0x0, 0x0, 0x0}, 0x0, 0x0})
	/opt/workspace/test-app/pkg/mod/google.golang.org/grpc@v1.71.0/internal/resolver/delegatingresolver/delegatingresolver.go:311 +0x68
google.golang.org/grpc/internal/resolver/dns.(*dnsResolver).watcher(0xc000fd0300)
	/opt/workspace/test-app/pkg/mod/google.golang.org/grpc@v1.71.0/internal/resolver/dns/dns_resolver.go:212 +0x138
created by google.golang.org/grpc/internal/resolver/dns.(*dnsBuilder).Build in goroutine 1005
	/opt/workspace/test-app/pkg/mod/google.golang.org/grpc@v1.71.0/internal/resolver/dns/dns_resolver.go:150 +0x2d9
```

Discussion:
@eshitachandwani: Hey @nkawli , can you share if you recently changed from using `grpc.Dial()` to `grpc.NewClient()` . Also it looks like you are using a proxy , is that correct ? Also if you could share what the test is doing? Along with that if you could share the logs by turning on the logging using :
```
export GRPC_GO_LOG_VERBOSITY_LEVEL=99
export GRPC_GO_LOG_SEVERITY_LEVEL=info
```
then I would be able to help you better.@nkawli: > Hey [@nkawli](https://github.com/nkawli) , can you share if you recently changed from using `grpc.Dial()` to `grpc.NewClient()` . Also it looks like you are using a proxy , is that correct ? Also if you could share what the test is doing? Along with that if you could share the logs by turning on the logging using :
> 
> ```
> export GRPC_GO_LOG_VERBOSITY_LEVEL=99
> export GRPC_GO_LOG_SEVERITY_LEVEL=info
> ```
> 
> then I would be able to help you better.

@eshitachandwani Sure, give me a couple of days to get back on the logs.
We are still using grpc.Dial in the current case. And I believe we are using proxy on the env where we are seeing this issue.
I believe the test is failing where we have `grpc.Dial("test:8085", grpc.WithTransportCredentials(insecure.NewCredentials())` where test is a dummy name ie. not a name which is resolvable. The test is just hitting our code to connect to a remote server. It was not failing earlier with v1.61.0, post upgrade to grpc-go v1.71.0, faced this issue couple of times. So went to the latest working version which was v1.69.4.@eshitachandwani: Hey, @nkawli , I tried reproducing the error , but I was not able to do it locally. Could you also provide a repro using the [helloworld example](https://github.com/grpc/grpc-go/tree/master/examples/helloworld). Atleast from the stack trace it looks like it is not setting the passthrough resolver correctly and hence getting the nil reference. Can you also try switching to `grpc.NewClient` ?@nkawli: @eshitachandwani - was able to reproduce with verbose logging enabled. Please find it below:

[grpc-debug-logs.zip](https://github.com/user-attachments/files/19324886/grpc-debug-logs.zip)

Narrowed down the test that it tries to create 5 connections each to test-2:8882 and test-3:8882 addresses which are non existent server endpoints. And then it closes the 5 connections to test-2:8882.
Also this is only seen when a https proxy is set. IP address masked and proxy URL changed for security reasons.
This is not seen everytime. But if i run the test 10 times, the crash is seen atleast once.

Also seen when the code is updated to use `grpc.NewClient `instead of `grpc.Dial`
Hopefully this helps.@eshitachandwani: Hey @nkawli , are you saying that this is flaky failure and not happening always? I am still not able to reproduce this error using `grpc.Dial()` and `HTTPS_PROXY`  environment variable , even with 10k runs, are you doing something differently. But what I could understand from the logs is that the resolver is closed (when clientconn gets closed or when entering idle mode , but I could not see entering idle mode log so it most likely because clientconn is being closed) before the channel is able to send an error update back to the resolver and that is why it cannot find the resolver and raising panic. If you could modify the helloworld example and provide me with failing code , or try with a dummy test case , I might be able to help with why the clientconn might be closing before updates reach the resolver in some case.@arjan-bal: @eshitachandwani  this seems to be a race where [updateProxyResolverState](https://github.com/grpc/grpc-go/blob/b0d120384670bde5a2fa830d65e43b250c24d8fd/internal/resolver/delegatingresolver/delegatingresolver.go#L249) is being called by the DNS resolver at the same time the channel calls [Close](https://github.com/grpc/grpc-go/blob/b0d120384670bde5a2fa830d65e43b250c24d8fd/internal/resolver/delegatingresolver/delegatingresolver.go#L172).

Since updateProxyResolverState is called by the resolver asynchronously,  a mutex is acquired at the beginning of the method. `Close` updates `delegatingResolver.proxyResolver` without acquiring the mutex. The fix is to acquire the mutex in `Close`. Once the resolver is closed, it should not be calling `UpdateState` on the channel after that.

Solution patch:
diff --git a/internal/resolver/delegatingresolver/delegatingresolver.go b/internal/resolver/delegatingresolver/delegatingresolver.go
index a6c64701..7b93f692 100644
--- a/internal/resolver/delegatingresolver/delegatingresolver.go
+++ b/internal/resolver/delegatingresolver/delegatingresolver.go
@@ -44,15 +44,19 @@ var (
 //
 // It implements the [resolver.Resolver] interface.
 type delegatingResolver struct {
-	target         resolver.Target     // parsed target URI to be resolved
-	cc             resolver.ClientConn // gRPC ClientConn
-	targetResolver resolver.Resolver   // resolver for the target URI, based on its scheme
-	proxyResolver  resolver.Resolver   // resolver for the proxy URI; nil if no proxy is configured
-	proxyURL       *url.URL            // proxy URL, derived from proxy environment and target
+	target   resolver.Target     // parsed target URI to be resolved
+	cc       resolver.ClientConn // gRPC ClientConn
+	proxyURL *url.URL            // proxy URL, derived from proxy environment and target
 
 	mu                  sync.Mutex         // protects all the fields below
 	targetResolverState *resolver.State    // state of the target resolver
 	proxyAddrs          []resolver.Address // resolved proxy addresses; empty if no proxy is configured
+
+	// childMu serializes calls into child resolvers. It also protects access to
+	// the following fields.
+	childMu        sync.Mutex
+	targetResolver resolver.Resolver // resolver for the target URI, based on its scheme
+	proxyResolver  resolver.Resolver // resolver for the proxy URI; nil if no proxy is configured
 }
 
 // nopResolver is a resolver that does nothing.
@@ -111,6 +115,10 @@ func New(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOpti
 		logger.Infof("Proxy URL detected : %s", r.proxyURL)
 	}
 
+	// Resolver updates from one child may trigger calls into the other. Block
+	// updates until the children are initialized.
+	r.childMu.Lock()
+	defer r.childMu.Unlock()
 	// When the scheme is 'dns' and target resolution on client is not enabled,
 	// resolution should be handled by the proxy, not the client. Therefore, we
 	// bypass the target resolver and store the unresolved target address.
@@ -165,11 +173,15 @@ func (r *delegatingResolver) proxyURIResolver(opts resolver.BuildOptions) (resol
 }
 
 func (r *delegatingResolver) ResolveNow(o resolver.ResolveNowOptions) {
+	r.childMu.Lock()
+	defer r.childMu.Unlock()
 	r.targetResolver.ResolveNow(o)
 	r.proxyResolver.ResolveNow(o)
 }
 
 func (r *delegatingResolver) Close() {
+	r.childMu.Lock()
+	defer r.childMu.Unlock()
 	r.targetResolver.Close()
 	r.targetResolver = nil
 
@@ -267,11 +279,17 @@ func (r *delegatingResolver) updateProxyResolverState(state resolver.State) erro
 	err := r.updateClientConnStateLocked()
 	// Another possible approach was to block until updates are received from
 	// both resolvers. But this is not used because calling `New()` triggers
-	// `Build()`  for the first resolver, which calls `UpdateState()`. And the
+	// `Build()` for the first resolver, which calls `UpdateState()`. And the
 	// second resolver hasn't sent an update yet, so it would cause `New()` to
 	// block indefinitely.
 	if err != nil {
-		r.targetResolver.ResolveNow(resolver.ResolveNowOptions{})
+		go func() {
+			r.childMu.Lock()
+			defer r.childMu.Unlock()
+			if r.targetResolver != nil {
+				r.targetResolver.ResolveNow(resolver.ResolveNowOptions{})
+			}
+		}()
 	}
 	return err
 }
@@ -291,7 +309,13 @@ func (r *delegatingResolver) updateTargetResolverState(state resolver.State) err
 	r.targetResolverState = &state
 	err := r.updateClientConnStateLocked()
 	if err != nil {
-		r.proxyResolver.ResolveNow(resolver.ResolveNowOptions{})
+		go func() {
+			r.childMu.Lock()
+			defer r.childMu.Unlock()
+			if r.proxyResolver != nil {
+				r.proxyResolver.ResolveNow(resolver.ResolveNowOptions{})
+			}
+		}()
 	}
 	return nil
 }



Test patch:
diff --git a/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go b/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go
index 6f60153c..18ae17ef 100644
--- a/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go
+++ b/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go
@@ -19,6 +19,8 @@
 package delegatingresolver_test
 
 import (
+	"context"
+	"errors"
 	"net/http"
 	"net/url"
 	"testing"
@@ -548,3 +550,213 @@ func (s) TestDelegatingResolverForMutipleProxyAddress(t *testing.T) {
 		t.Fatalf("Unexpected state from delegating resolver. Diff (-got +want):\n%v", diff)
 	}
 }
+
+// Tests that delegatingresolver doesn't panic when the channel closes the
+// resolver while it's handling an update from it's child. The test closes the
+// delegating resolver, verifies the target resolver is closed and blocks the
+// proxy resolver from being closed. The test sends an update from the proxy
+// resolver and verifies that the target resolver's ResolveNow method is not
+// called after the channels returns an error.
+func (s) TestDelegatingResolverUpdateStateDuringClose(t *testing.T) {
+	const envProxyAddr = "proxytest.com"
+
+	hpfe := func(req *http.Request) (*url.URL, error) {
+		return &url.URL{
+			Scheme: "https",
+			Host:   envProxyAddr,
+		}, nil
+	}
+	originalhpfe := delegatingresolver.HTTPSProxyFromEnvironment
+	delegatingresolver.HTTPSProxyFromEnvironment = hpfe
+	defer func() {
+		delegatingresolver.HTTPSProxyFromEnvironment = originalhpfe
+	}()
+
+	// Manual resolver to control the target resolution.
+	targetResolver := manual.NewBuilderWithScheme("test")
+	targetResolverCalled := make(chan struct{})
+	targetResolver.ResolveNowCallback = func(resolver.ResolveNowOptions) {
+		close(targetResolverCalled)
+	}
+	targetResolverCloseCalled := make(chan struct{})
+	targetResolver.CloseCallback = func() {
+		close(targetResolverCloseCalled)
+		t.Log("Target resolver is closed.")
+	}
+
+	target := targetResolver.Scheme() + ":///" + "ignored"
+	// Set up a manual DNS resolver to control the proxy address resolution.
+	proxyResolver := setupDNS(t)
+
+	unblockProxyResolverClose := make(chan struct{})
+	proxyResolver.CloseCallback = func() {
+		<-unblockProxyResolverClose
+		t.Log("Proxy resolver is closed.")
+	}
+
+	tcc, _, _ := createTestResolverClientConn(t)
+	tcc.UpdateStateF = func(resolver.State) error {
+		return errors.New("test error")
+	}
+	dr, err := delegatingresolver.New(resolver.Target{URL: *testutils.MustParseURL(target)}, tcc, resolver.BuildOptions{}, targetResolver, false)
+	if err != nil {
+		t.Fatalf("Failed to create delegating resolver: %v", err)
+	}
+
+	targetResolver.UpdateState(resolver.State{
+		Endpoints: []resolver.Endpoint{{Addresses: []resolver.Address{{Addr: "1.1.1.1"}}}},
+	})
+
+	// Closing the delegating resolver will block until the test writes to the
+	// unblockProxyResolverClose channel.
+	go dr.Close()
+	ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
+	defer cancel()
+	select {
+	case <-targetResolverCloseCalled:
+	case <-ctx.Done():
+		t.Fatalf("Context timed out waiting for target resolver's Close method to be called.")
+	}
+
+	// Updating the channel will result in an error being returned. Since the
+	// target resolver's Close method is already called, the delegating resolver
+	// must not call "ResolveNow" on it.
+	go proxyResolver.UpdateState(resolver.State{
+		Endpoints: []resolver.Endpoint{{Addresses: []resolver.Address{{Addr: "1.1.1.1"}}}},
+	})
+	unblockProxyResolverClose <- struct{}{}
+
+	select {
+	case <-targetResolverCalled:
+		t.Fatalf("targetResolver.ResolveNow() called unexpectedly.")
+	case <-time.After(defaultTestShortTimeout):
+	}
+}
+
+// Tests that calling cc.UpdateState in a blocking manner from a child resolver
+// while handling a ResolveNow call doesn't result in a deadlock. The test uses
+// a fake ClientConn that returns an error when calling cc.UpdateState. The test
+// makes the proxy resolver update the resolver state. The test verifies that
+// the delegating resolver calls ResolveNow on the target resolver when the
+// ClientConn returns an error.
+func (s) TestDelegatingResolverUpdateStateFromResolveNow(t *testing.T) {
+	const envProxyAddr = "proxytest.com"
+
+	hpfe := func(req *http.Request) (*url.URL, error) {
+		return &url.URL{
+			Scheme: "https",
+			Host:   envProxyAddr,
+		}, nil
+	}
+	originalhpfe := delegatingresolver.HTTPSProxyFromEnvironment
+	delegatingresolver.HTTPSProxyFromEnvironment = hpfe
+	defer func() {
+		delegatingresolver.HTTPSProxyFromEnvironment = originalhpfe
+	}()
+
+	// Manual resolver to control the target resolution.
+	targetResolver := manual.NewBuilderWithScheme("test")
+	targetResolverCalled := make(chan struct{})
+	targetResolver.ResolveNowCallback = func(resolver.ResolveNowOptions) {
+		// Updating the resolver state should not deadlock.
+		targetResolver.CC.UpdateState(resolver.State{
+			Endpoints: []resolver.Endpoint{{Addresses: []resolver.Address{{Addr: "1.1.1.1"}}}},
+		})
+		close(targetResolverCalled)
+	}
+
+	target := targetResolver.Scheme() + ":///" + "ignored"
+	// Set up a manual DNS resolver to control the proxy address resolution.
+	proxyResolver := setupDNS(t)
+
+	tcc, _, _ := createTestResolverClientConn(t)
+	tcc.UpdateStateF = func(resolver.State) error {
+		return errors.New("test error")
+	}
+	_, err := delegatingresolver.New(resolver.Target{URL: *testutils.MustParseURL(target)}, tcc, resolver.BuildOptions{}, targetResolver, false)
+	if err != nil {
+		t.Fatalf("Failed to create delegating resolver: %v", err)
+	}
+
+	targetResolver.UpdateState(resolver.State{
+		Endpoints: []resolver.Endpoint{{Addresses: []resolver.Address{{Addr: "1.1.1.1"}}}},
+	})
+
+	// Updating the channel will result in an error being returned. The
+	// delegating resolver should call call "ResolveNow" on the target resolver.
+	proxyResolver.UpdateState(resolver.State{
+		Endpoints: []resolver.Endpoint{{Addresses: []resolver.Address{{Addr: "1.1.1.1"}}}},
+	})
+
+	ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
+	defer cancel()
+	select {
+	case <-targetResolverCalled:
+	case <-ctx.Done():
+		t.Fatalf("context timed out waiting for targetResolver.ResolveNow() to be called.")
+	}
+}
+
+// Tests that calling cc.UpdateState in a blocking manner from child resolvers
+// doesn't result in deadlocks.
+func (s) TestDelegatingResolverResolveNow(t *testing.T) {
+	const envProxyAddr = "proxytest.com"
+
+	hpfe := func(req *http.Request) (*url.URL, error) {
+		return &url.URL{
+			Scheme: "https",
+			Host:   envProxyAddr,
+		}, nil
+	}
+	originalhpfe := delegatingresolver.HTTPSProxyFromEnvironment
+	delegatingresolver.HTTPSProxyFromEnvironment = hpfe
+	defer func() {
+		delegatingresolver.HTTPSProxyFromEnvironment = originalhpfe
+	}()
+
+	// Manual resolver to control the target resolution.
+	targetResolver := manual.NewBuilderWithScheme("test")
+	targetResolverCalled := make(chan struct{})
+	targetResolver.ResolveNowCallback = func(resolver.ResolveNowOptions) {
+		// Updating the resolver state should not deadlock.
+		targetResolver.CC.UpdateState(resolver.State{
+			Endpoints: []resolver.Endpoint{{Addresses: []resolver.Address{{Addr: "1.1.1.1"}}}},
+		})
+		close(targetResolverCalled)
+	}
+
+	target := targetResolver.Scheme() + ":///" + "ignored"
+	// Set up a manual DNS resolver to control the proxy address resolution.
+	proxyResolver := setupDNS(t)
+	proxyResolverCalled := make(chan struct{})
+	proxyResolver.ResolveNowCallback = func(resolver.ResolveNowOptions) {
+		// Updating the resolver state should not deadlock.
+		proxyResolver.CC.UpdateState(resolver.State{
+			Endpoints: []resolver.Endpoint{{Addresses: []resolver.Address{{Addr: "1.1.1.1"}}}},
+		})
+		close(proxyResolverCalled)
+	}
+
+	tcc, _, _ := createTestResolverClientConn(t)
+	dr, err := delegatingresolver.New(resolver.Target{URL: *testutils.MustParseURL(target)}, tcc, resolver.BuildOptions{}, targetResolver, false)
+	if err != nil {
+		t.Fatalf("Failed to create delegating resolver: %v", err)
+	}
+
+	// Call ResolveNow on the delegatingResolver and verify both children
+	// receive the ResolveNow call.
+	dr.ResolveNow(resolver.ResolveNowOptions{})
+
+	ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
+	defer cancel()
+	select {
+	case <-targetResolverCalled:
+	case <-ctx.Done():
+		t.Fatalf("context timed out waiting for targetResolver.ResolveNow() to be called.")
+	}
+	select {
+	case <-proxyResolverCalled:
+	case <-ctx.Done():
+		t.Fatalf("context timed out waiting for proxyResolver.ResolveNow() to be called.")
+	}
+}