BudgetScan

RemoveClients accepts nil callback and panics without releasing lock

low swesweep-fasthttp · lbclient.go · LBClient.RemoveClients · run #4

Description

The rc parameter is invoked without a nil check. Passing nil causes a nil function call panic; because Unlock is not deferred, the panic leaves cc.mu locked. This is a lower-severity caller-error path, but it can be triggered if rc comes from untrusted configuration or code.

Triage

Not triaged.

Raw scan report for this file

## Security analysis

This file implements a load-balancing client wrapper. The main trust boundary is the public `LBClient` API: `Do`, `DoTimeout`, `DoDeadline`, `AddClient`, and `RemoveClients`. The wrapper forwards caller-supplied `Request`/`Response` values to user-provided `BalancingClient` implementations and user-provided callbacks. The most relevant impact here is availability: panics, nil dereferences, slice out-of-range errors, and deadlocks in the host process.

### Data flow

- `LBClient.Do`
  - reads `cc.Timeout`
  - calls `DoTimeout`

- `LBClient.DoTimeout`
  - computes a deadline
  - calls `get()`
  - calls the selected `lbClient.DoDeadline`

- `LBClient.DoDeadline`
  - calls `get()`
  - calls the selected `lbClient.DoDeadline`

- `LBClient.get`
  - lazily initializes via `init()`
  - reads `cc.cs` under `RLock`
  - selects one `*lbClient`
  - releases the lock
  - returns the selected client

- `lbClient.DoDeadline`
  - calls `c.c.DoDeadline`
  - evaluates health
  - updates `penalty` or `total`

### Notable issues

1. **`RemoveClients` can leave zero clients**
   - `RemoveClients` removes clients based on `rc`.
   - If `rc` returns `true` for every client, `cc.cs` is truncated to length zero.
   - `get()` then executes `minC := cs[0]` without checking `len(cs) > 0`.
   - This causes a slice index out-of-range panic on the next request.

2. **Nil `BalancingClient` is not rejected**
   - `init()` and `AddClient()` store `BalancingClient` values directly.
   - If a nil `BalancingClient` is present, `lbClient.DoDeadline` calls `c.c.DoDeadline` on a nil interface.
   - This causes a nil pointer dereference panic.

3. **`RemoveClients` invokes a callback while holding the write lock**
   - `rc` is called while `cc.mu` is locked.
   - If `rc` reentrantly calls `AddClient`, `RemoveClients`, or `Do`/`DoTimeout`/`DoDeadline`, it can block on the same mutex and deadlock.
   - If `rc` panics, `cc.mu.Unlock` is not deferred, so the mutex remains locked forever.

4. **Lazy init can panic if `Clients` is empty but `AddClient` was used**
   - `init()` checks only `len(cc.Clients)`, not `len(cc.cs)`.
   - A zero-value `LBClient` can have clients added via `AddClient` before the first request.
   - The first `get()` still calls `init()`, which panics because `cc.Clients` is empty.

5. **Nil `req`/`resp` are forwarded unchecked**
   - The public entry points accept `req` and `resp` without nil checks.
   - They are passed to `c.c.DoDeadline` and, during health evaluation, to `c.healthCheck`.
   - If the downstream client or callback assumes non-nil pointers, this can panic.

6. **Lower-confidence concurrency issues**
   - `RemoveClients` does not check whether `rc` is nil.
   - Exported fields such as `HealthCheck` and `Timeout` can be mutated concurrently with method calls, even though methods are documented as safe for concurrent use.

### Non-issues / lower-confidence observations

- `penalty` and `total` use atomic operations consistently.
- `decPenalty` subtracts one from `penalty`; under normal operation this should not underflow because decrements are only created by successful `incPenalty` calls.
- `time.AfterFunc` timers are bounded per client by `maxPenalty`, so they are not an obvious unbounded goroutine/timer leak under normal use.
- No fixed-size buffer copies exist in this file.

```json
[
  {
    "severity": "high",
    "title": "Empty client list causes slice index out-of-range panic",
    "function": "LBClient.get",
    "description": "RemoveClients can truncate cc.cs to length zero if rc returns true for every client. A later Do, DoTimeout, or DoDeadline calls get, which executes minC := cs[0] without checking len(cs) > 0, causing a runtime panic and host process crash."
  },
  {
    "severity": "high",
    "title": "Nil BalancingClient causes nil pointer dereference",
    "function": "lbClient.DoDeadline",
    "description": "LBClient.init and LBClient.AddClient store nil BalancingClient values without validation. When get selects such an lbClient, DoDeadline calls c.c.DoDeadline on a nil interface, causing a panic. If client lists or AddClient input can be influenced by untrusted data, this is a DoS."
  },
  {
    "severity": "high",
    "title": "RemoveClients callback invoked while holding write lock can deadlock or lock permanently",
    "function": "LBClient.RemoveClients",
    "description": "rc is called while cc.mu is write-locked and without a deferred Unlock. If rc reentrantly calls AddClient, RemoveClients, or Do/DoTimeout/DoDeadline, it blocks on the same mutex and deadlocks. If rc panics, the mutex is not released, permanently locking the LBClient."
  },
  {
    "severity": "medium",
    "title": "Lazy init panics when Clients is empty even if AddClient populated cs",
    "function": "LBClient.init",
    "description": "get calls init via sync.Once. init only checks len(cc.Clients), not len(cc.cs). A zero-value LBClient can have one or more clients added with AddClient before the first Do; the later init still panics because cc.Clients is empty, crashing the host process."
  },
  {
    "severity": "medium",
    "title": "Nil request or response forwarded unchecked to client and health callback",
    "function": "lbClient.DoDeadline",
    "description": "DoDeadline, DoTimeout, and Do accept req and resp without nil checks and pass them to c.c.DoDeadline and, during health evaluation, to c.healthCheck. If the BalancingClient implementation or HealthCheck assumes non-nil pointers, an attacker-supplied nil request or response can trigger a panic."
  },
  {
    "severity": "low",
    "title": "RemoveClients accepts nil callback and panics without releasing lock",
    "function": "LBClient.RemoveClients",
    "description": "The rc parameter is invoked without a nil check. Passing nil causes a nil function call panic; because Unlock is not deferred, the panic leaves cc.mu locked. This is a lower-severity caller-error path, but it can be triggered if rc comes from untrusted configuration or code."
  },
  {
    "severity": "low",
    "title": "Exported field mutation can race with AddClient and Do",
    "function": "LBClient.AddClient",
    "description": "AddClient reads cc.HealthCheck while holding the mutex, but HealthCheck is an exported field that callers may assign to concurrently. Do also reads Timeout without synchronization. The type documents concurrent method safety, but unsynchronized exported field updates can cause data races and inconsistent health-checking behavior."
  }
]
```