Skip to content

Commit 7f78342

Browse files
xdsclient: fix panic on empty resource in ADS response (grpc#8970)
This PR fixes an intermittent panic in xdsclient `decodeResponse` that occurs when processing an xDS ADS response containing an empty resource name, while doing fuzz testing. RELEASE NOTES: * xds: Fix a panic in the xDS client that can occur when processing an ADS response containing an empty resource name.
1 parent e5563c6 commit 7f78342

File tree

11 files changed

+188
-50
lines changed

11 files changed

+188
-50
lines changed

internal/xds/clients/xdsclient/channel.go

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -214,12 +214,7 @@ func (xc *xdsChannel) onResponse(resp response, onDone func()) ([]string, error)
214214
return nil, xdsresource.NewErrorf(xdsresource.ErrorTypeResourceTypeUnsupported, "Resource type URL %q unknown in response from server", resp.typeURL)
215215
}
216216

217-
// Decode the resources and build the list of resource names to return.
218-
opts := &DecodeOptions{
219-
Config: xc.clientConfig,
220-
ServerConfig: xc.serverConfig,
221-
}
222-
updates, md, err := decodeResponse(opts, &rType, resp)
217+
updates, md, err := xc.decodeResponse(&rType, resp)
223218
var names []string
224219
for name := range updates {
225220
names = append(names, name)
@@ -243,12 +238,16 @@ func (xc *xdsChannel) onResponse(resp response, onDone func()) ([]string, error)
243238
// If there are any errors decoding the resources, the metadata will indicate
244239
// that the update was NACKed, and the returned error will contain information
245240
// about all errors encountered by this function.
246-
func decodeResponse(opts *DecodeOptions, rType *ResourceType, resp response) (map[string]dataAndErrTuple, xdsresource.UpdateMetadata, error) {
241+
func (xc *xdsChannel) decodeResponse(rType *ResourceType, resp response) (map[string]dataAndErrTuple, xdsresource.UpdateMetadata, error) {
247242
timestamp := time.Now()
248243
md := xdsresource.UpdateMetadata{
249244
Version: resp.version,
250245
Timestamp: timestamp,
251246
}
247+
opts := &DecodeOptions{
248+
Config: xc.clientConfig,
249+
ServerConfig: xc.serverConfig,
250+
}
252251

253252
topLevelErrors := make([]error, 0) // Tracks deserialization errors, where we don't have a resource name.
254253
perResourceErrors := make(map[string]error) // Tracks resource validation errors, where we have a resource name.
@@ -268,6 +267,10 @@ func decodeResponse(opts *DecodeOptions, rType *ResourceType, resp response) (ma
268267
// Name field of the result is left unpopulated only when resource
269268
// deserialization fails.
270269
name := ""
270+
if result == nil && err == nil {
271+
xc.logger.Errorf("Decode() returned nil result and nil error for resource: %v", r)
272+
continue
273+
}
271274
if result != nil {
272275
name = xdsresource.ParseName(result.Name).String()
273276
}

internal/xds/clients/xdsclient/channel_test.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -793,7 +793,8 @@ func (s) TestDecodeResponse_PanicRecoveryEnabled(t *testing.T) {
793793
resp := response{resources: []*anypb.Any{{Value: []byte("test")}}}
794794
wantErr := "recovered from panic during resource parsing"
795795

796-
if _, _, err := decodeResponse(&DecodeOptions{}, rType, resp); err == nil || !strings.Contains(err.Error(), wantErr) {
796+
xc := &xdsChannel{}
797+
if _, _, err := xc.decodeResponse(rType, resp); err == nil || !strings.Contains(err.Error(), wantErr) {
797798
t.Fatalf("decodeResponse() failed with err: %v, want %q", err, wantErr)
798799
}
799800
}
@@ -809,11 +810,12 @@ func (s) TestDecodeResponse_PanicRecoveryDisabled(t *testing.T) {
809810
}
810811
resp := response{resources: []*anypb.Any{{Value: []byte("test")}}}
811812
wantErr := "simulate panic"
813+
xc := &xdsChannel{}
812814

813815
defer func() {
814816
if r := recover(); r == nil || !strings.Contains(fmt.Sprint(r), wantErr) {
815817
t.Fatalf("Expected panic in decodeResponse, got: %v, want: %q", r, wantErr)
816818
}
817819
}()
818-
decodeResponse(&DecodeOptions{}, rType, resp)
820+
xc.decodeResponse(rType, resp)
819821
}

0 commit comments

Comments
 (0)