Aias00 opened a new issue, #3510:
URL: https://github.com/apache/dubbo-go/issues/3510
### Problem
`conshashLoadBalance.Select` caches selectors in a package-global
`selectors` map. `newSelector` can return `nil`, and that `nil` is stored in
the map. On a subsequent `Select` for the same key, the code reads the stored
`nil` selector and dereferences it (`selector.hashCode`, `selector.Select`),
causing a nil-pointer panic.
Separately, when `hash.nodes` is configured to a value smaller than 4,
`replicaNum/4 == 0`, the consistent-hash ring (`selector.keys`) is never
populated. `selectForKey` then does `c.keys[idx]` on an empty slice →
index-out-of-range panic.
These are distinct from the map-access race fixed in #3317 and the `toKey`
panics fixed in #3430 / #3432.
### Current behavior
`cluster/loadbalance/consistenthashing/selector.go:52-58` — `newSelector`
returns `nil` when `hash.arguments` contains a non-numeric index:
```go
indices := re.Split(url.GetMethodParam(methodName, HashArguments, "0"), -1)
for _, index := range indices {
i, err := strconv.Atoi(index)
if err != nil {
return nil // nil cached in selectors[key]
}
selector.argumentIndex = append(selector.argumentIndex, i)
}
```
`cluster/loadbalance/consistenthashing/loadbalance.go:77-90` — the cached
`nil` is dereferenced on the next call:
```go
if !ok || selector.hashCode != hashCode { // selector is nil here →
selector.hashCode panics
selectorsMu.Lock()
selector, ok = selectors[key]
if !ok || selector.hashCode != hashCode {
selectors[key] = newSelector(invokers, methodName, hashCode) //
stores nil
}
selector = selectors[key] // nil
selectorsMu.Unlock()
}
return selector.Select(invocation) // nil.Select → panic
```
`cluster/loadbalance/consistenthashing/selector.go:99-106` — empty ring when
`hash.nodes < 4`:
```go
func (c *selector) selectForKey(hash uint32) base.Invoker {
idx := sort.Search(len(c.keys), func(i int) bool {
return c.keys[i] >= hash
})
if idx == len(c.keys) {
idx = 0
}
return c.virtualInvokers[c.keys[idx]] // c.keys empty → index out of
range → panic
}
```
### Expected behavior
1. A malformed `hash.arguments` value must not poison the selector cache
with `nil` and must not panic; it should fail with a clear configuration error
or fall back to the default index.
2. `hash.nodes < 4` (or any config yielding `replicaNum/4 == 0`) must be
rejected at startup or handled with a non-empty ring, not panic at request time.
3. `Select` must never dereference a `nil` selector.
### Suggested approach
- Change `newSelector` to return `(*selector, error)`; do not cache `nil` in
`selectors[key]`. On error, `Select` logs and returns a degraded invoker (e.g.
first available) or a deterministic fallback.
- Validate `hash.nodes` lower bound (e.g. ≥ 4, defaulting to 160) at
selector construction; guard `selectForKey` against an empty `c.keys`.
- Add regression tests for: non-numeric `hash.arguments` index, `hash.nodes`
∈ {1,2,3}, and that a prior failed `newSelector` does not break subsequent
selections.
### Acceptance criteria
- [ ] Non-numeric `hash.arguments` index no longer panics; selector cache
never holds `nil`.
- [ ] `hash.nodes` set to 1–3 no longer panics.
- [ ] `Select` never dereferences a `nil` selector.
- [ ] Regression tests cover the two panic paths above (`-race` clean).
--
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]
