cnYui opened a new pull request, #1032:
URL: https://github.com/apache/dubbo-go-pixiu/pull/1032
**What this PR does**:
`descriptor_source_strategy` defaults to `AUTO` (`grpc.go` —
`default:"auto"`), which `getDescriptorSource` documents as `// file +
reflection`. But `initDescriptorSource` only loaded the local proto files for
`LOCAL`:
```go
func (dr *Descriptor) initDescriptorSource(cfg *Config) *Descriptor {
if cfg.DescriptorSourceStrategy.String() == LOCAL {
dr.initFileDescriptorSource(cfg)
}
return dr
}
```
and it is the only wiring call (`grpc.go`, in `FilterFactory.Apply`). So
under the default strategy `dr.fileSource` stays `nil`, and
`getDescriptorCompose` assigns it straight into an interface field:
```go
cs.file = dr.getFileSource() // nil *fileSource -> non-nil DescriptorSource
```
`cs.file == nil` is therefore **false**, and the fallback in
`compositeSource.FindSymbol` dereferences the nil receiver:
```go
return cs.file.FindSymbol(fullyQualifiedName) // descriptor_source.go:163
```
Two consequences on the out-of-the-box configuration:
1. Any request whose server-reflection lookup fails — a backend that does
not implement the reflection API, or an unknown service name — hits a
nil-pointer dereference inside the filter chain. `pkg/common/http/manager.go`
recovers it, so this is not a process crash, but the caller gets a 500 with a
stack trace in the log instead of the intended 405/400.
2. The `file` half of `AUTO` never runs at all, so local `.proto` files are
silently ignored under the default strategy.
`getFileDescriptorCompose` returns the same typed-nil, so `LOCAL` with a
`path` that cannot be read fails the same way.
The change, confined to `descriptor.go` and `descriptor_source.go`:
- `initDescriptorSource` loads the local proto files for `AUTO` as well as
`LOCAL`, once at `Apply()` time. It now uses `strings.ToLower(...)` to match
the comparison `getDescriptorSource` already does.
- `getDescriptorCompose` only assigns `cs.file` when the file source is
non-nil, so a typed-nil is never boxed into the interface.
- `getFileDescriptorCompose` returns an error instead of a nil source when
the local proto files could not be loaded.
- `compositeSource.FindSymbol` and `AllExtensionsForType` guard `cs.file`,
so a missing file source surfaces an error rather than a panic.
- `AllExtensionsForType` is restructured flat. Its `cs.reflection == nil`
branch used to compute `cs.file.AllExtensionsForType(...)` and then fall
through to `return nil, nil`, discarding the successful result; it now returns
it. (Collapsing that branch in place would leave the trailing `return nil, nil`
unreachable and fail `go vet`, hence the flat rewrite.)
**Which issue(s) this PR fixes**:
Fixes #
**Special notes for your reviewer**:
`pkg/filter/http/grpcproxy` had no test file for `descriptor.go`, so this
adds `descriptor_test.go` with four cases that drive the real production path
(`initDescriptorSource` → `getDescriptorSource` → `getMethodDescriptor`)
against a gRPC server with no reflection service registered.
All four fail on `develop` (`fd9a27e`) and pass with this change. Before, on
unmodified sources:
```
--- FAIL: TestDescriptorAutoStrategyFallsBackToLocalProtoFiles
Error: Expected value not to be nil. # AUTO never loaded
the protos
--- FAIL: TestDescriptorLocalStrategyReportsUnloadableProtoFiles
Error: An error is expected but got nil.
--- FAIL: TestCompositeSourceFindSymbolWithoutFileSource
panic: runtime error: invalid memory address or nil pointer dereference
--- FAIL: TestCompositeSourceAllExtensionsForTypeWithoutReflection
Error: "[]" should have 1 item(s), but has 0
```
With the `require.NotNil` guard temporarily removed so the first test
reaches the fallback, unmodified sources panic exactly on the live request path:
```
panic: runtime error: invalid memory address or nil pointer dereference
grpcproxy.(*fileSource).FindSymbol
pkg/filter/http/grpcproxy/descriptor_source.go:123
grpcproxy.(*compositeSource).FindSymbol
pkg/filter/http/grpcproxy/descriptor_source.go:163
grpcproxy.(*Descriptor).getMethodDescriptor
pkg/filter/http/grpcproxy/descriptor.go:216
```
After the change (Go 1.25.7):
```
$ go build ./pkg/filter/http/grpcproxy/ # rc=0
$ go vet ./pkg/filter/http/grpcproxy/ # rc=0, silent
$ golangci-lint run ./pkg/filter/http/grpcproxy/...
0 issues.
$ gofmt -s -l <the three changed files> # no output
$ imports-formatter # leaves the three files
unchanged
$ go test ./pkg/filter/http/grpcproxy/ -v
--- PASS: TestGRPCConnectionManagerSharesConcurrentConnection (0.01s)
... 14 pre-existing tests, all PASS ...
--- PASS: TestDescriptorAutoStrategyFallsBackToLocalProtoFiles (0.02s)
--- PASS: TestDescriptorLocalStrategyReportsUnloadableProtoFiles (0.00s)
--- PASS: TestCompositeSourceFindSymbolWithoutFileSource (0.00s)
--- PASS: TestCompositeSourceAllExtensionsForTypeWithoutReflection (0.00s)
PASS
ok github.com/apache/dubbo-go-pixiu/pkg/filter/http/grpcproxy 0.730s
```
Notes:
- Loading the file source for `AUTO` runs once in `Apply()`, not per
request, so a misconfigured `path` logs one error at startup rather than on
every request. With `path` unset the loader reads the executable's directory,
finds no `.proto` files and yields a valid empty source (INFO only, no error),
so reflection-only `AUTO` deployments are unaffected.
- `AllExtensionsForType`'s discarded-result bug is currently latent:
`cs.reflection` is nil only when `getDescriptorCompose` also returns a non-nil
error, which `Decode` turns into `filter.Stop`. It is fixed here because it is
the same function, not because it is separately reachable.
- Scope is kept to `descriptor.go` and `descriptor_source.go` to avoid
conflicting with #1023, which is editing `grpc.go` and `connection_manager.go`
in this package.
- I verified the touched package only. A full `go build ./...` was not
possible on my machine: I am on Windows, where `admin/core` cannot build
because `utils.GetWriteSyncer` lives in the Unix-only
`admin/utils/rotatelogs_unix.go`, and the build then ran the disk out of space.
Neither is related to this change, and `pkg/filter/http/grpcproxy` builds, vets
and tests clean.
**Does this PR introduce a user-facing change?**:
```release-note
Fix a nil-pointer dereference in the gRPC proxy filter under the default
`AUTO` descriptor source strategy, which returned a 500 whenever a
server-reflection lookup failed, and make `AUTO` actually load the local .proto
files it documents as its fallback.
```
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
