BudgetScan

Unchecked nil WorkerFunc, connState, and Logger callbacks

low swesweep-fasthttp · workerpool.go · workerFunc · run #4

Description

wp.WorkerFunc(c) is called without a nil check, wp.connState(c, StateClosed/StateHijacked) is called for completed/hijacked connections without a nil check, and wp.Logger.Printf is called when an error must be logged without checking Logger. If the host API constructs workerPool without initializing these callbacks, a single inbound connection can trigger a nil-pointer panic in a worker goroutine. connState is especially notable because it is invoked on every normal connection completion.

Triage

Not triaged.

Raw scan report for this file

## Security analysis

This file implements the fasthttp worker pool. The main untrusted flow is:

`net.Conn` from acceptor → `workerPool.Serve(c)` → `getCh()` → `ch.ch <- c` → worker goroutine → `wp.workerFunc(ch)` → `wp.WorkerFunc(c)`.

Because this is Go code, classic C-style fixed-buffer overflows are not present here. The relevant risks are:

1. **Panic propagation from untrusted connections**
   - Worker goroutines call `wp.WorkerFunc(c)`, `wp.connState`, `wp.Logger.Printf`, and `c.Close()` without `recover()`.
   - A malformed connection that panics in the HTTP parser/handler, or a misbehaving callback, can crash the host process.
   - If a panic were contained elsewhere, the pool would still leak accounting: `wp.workersCount--` and `wp.workerChanPool.Put(vch)` would be skipped.

2. **Unchecked nil callbacks**
   - `wp.WorkerFunc`, `wp.connState`, and `wp.Logger` are dereferenced without nil checks.
   - If a host constructs the pool without setting all callbacks, a single inbound connection can panic a worker.

3. **Lifecycle/state issues**
   - `Stop()` sets `wp.mustStop = true`, but `Start()` does not reset it.
   - After a stop/start cycle, `release()` always returns false, so workers exit after one connection and the pool stops behaving like a pool.
   - `Stop()` only signals idle workers in `wp.ready`; busy workers are not interrupted and can be kept alive by an attacker holding connections open.

4. **Concurrency hygiene**
   - `Start()` and `Stop()` manipulate `wp.stopCh` without synchronization.
   - Concurrent lifecycle calls can race and potentially leave multiple cleaner goroutines or inconsistent state.

These are primarily availability/lifecycle bugs rather than memory-corruption bugs.

```json
[
  {
    "severity": "high",
    "title": "Unrecovered panic in worker goroutine can crash host or corrupt pool accounting",
    "function": "workerFunc / getCh goroutine",
    "description": "The goroutine started in getCh calls wp.workerFunc(ch) and wp.workerChanPool.Put(vch) with no defer/recover. workerFunc processes attacker-supplied net.Conn values via wp.WorkerFunc and also invokes wp.connState, wp.Logger.Printf, and c.Close(). A malformed packet that causes a panic in the downstream parser/handler, or a panic in any callback, escapes the worker goroutine. In Go, an unrecovered panic in a goroutine terminates the host process. Even if recovered elsewhere, the pool would skip workersCount-- and Put, leaking pool state and allowing MaxWorkersCount exhaustion through repeated panics."
  },
  {
    "severity": "low",
    "title": "Unchecked nil WorkerFunc, connState, and Logger callbacks",
    "function": "workerFunc",
    "description": "wp.WorkerFunc(c) is called without a nil check, wp.connState(c, StateClosed/StateHijacked) is called for completed/hijacked connections without a nil check, and wp.Logger.Printf is called when an error must be logged without checking Logger. If the host API constructs workerPool without initializing these callbacks, a single inbound connection can trigger a nil-pointer panic in a worker goroutine. connState is especially notable because it is invoked on every normal connection completion."
  },
  {
    "severity": "low",
    "title": "Stop/Start lifecycle leaves mustStop and workersCount stale",
    "function": "Start / Stop",
    "description": "Stop() sets wp.mustStop = true, but Start() does not reset it. After a Stop/Start cycle, release() always returns false, so every worker exits after serving one connection instead of returning to the ready pool. wp.workersCount also persists across lifetimes. This can cause excessive goroutine/channel churn, degrade performance, and with a low MaxWorkersCount or long-lived connections can reduce availability after restart."
  },
  {
    "severity": "low",
    "title": "Stop does not interrupt busy workers",
    "function": "Stop",
    "description": "Stop() only sends nil to workers currently present in wp.ready. Workers actively executing wp.WorkerFunc are not interrupted and must finish the current net.Conn. An attacker can open or keep up to MaxWorkersCount connections and stall reads/writes to retain worker goroutines, delay shutdown, and keep resources allocated."
  },
  {
    "severity": "low",
    "title": "Data race on stopCh and workerChanPool.New during concurrent lifecycle calls",
    "function": "Start / Stop",
    "description": "Start() and Stop() read and write wp.stopCh without synchronization. Concurrent lifecycle calls can race, potentially starting multiple cleaner goroutines or changing pool state while another lifecycle transition is in progress. This can cause unexpected worker shutdown signals and pool state inconsistencies."
  }
]
```