BudgetScan

Unchecked interface type assertion in AcquireTimer

low swesweep-fasthttp · timer.go · AcquireTimer · run #4

Description

AcquireTimer casts the value returned by timerPool.Get() to *time.Timer without using the comma-ok assertion form. If the sync.Pool ever contains a non-time.Timer value, the function panics. The risk is currently low because timerPool is unexported and normally populated only by ReleaseTimer, but it is a robustness hazard for a public pooled API.

Triage

Not triaged.

Raw scan report for this file

## Security analysis

This file implements a pooled wrapper around `time.Timer` for fasthttp. It does not parse network data, use fixed-size buffers, perform integer arithmetic, or use `unsafe`. Therefore, the classic C-style memory corruption classes are not applicable here. The main security concerns are:

- public API robustness against invalid arguments,
- panic-DoS potential,
- timer lifetime/pool invariants,
- data races caused by incorrect caller usage.

The trust boundary is the exported surface:

- `AcquireTimer(timeout)`
- `ReleaseTimer(t)`

Internal helpers are only reachable through those:

- `initTimer`
- `stopTimer`

### `initTimer`

```go
func initTimer(t *time.Timer, timeout time.Duration) *time.Timer {
	if t == nil {
		return time.NewTimer(timeout)
	}
	if t.Reset(timeout) {
		// developer sanity-check
		panic("BUG: active timer trapped into initTimer()")
	}
	return t
}
```

This function is nil-safe: if `t == nil`, it allocates a new timer.

The dangerous behavior is the deliberate panic when `t.Reset(timeout)` returns `true`, meaning the timer was still active. Under correct usage, this should not happen because `ReleaseTimer` calls `stopTimer`, which stops the timer before returning it to the pool. However, if a timer is released twice, reused concurrently, or otherwise violates the pool invariant, `initTimer` can panic.

This is not directly attacker-controlled through untrusted payload bytes, but an in-process caller or buggy integration could trigger a panic-DoS in timeout-sensitive code paths.

Severity: low.

### `stopTimer`

```go
func stopTimer(t *time.Timer) {
	if !t.Stop() {
		select {
		case <-t.C:
		default:
		}
	}
}
```

This function assumes `t` is non-nil. If `t == nil`, `t.Stop()` panics with a nil pointer dereference. The drain logic is correct for a `time.Timer` channel, which has capacity one.

Because `stopTimer` is called from the public `ReleaseTimer`, a nil timer passed to `ReleaseTimer` becomes a panic.

Severity: medium if nil is reachable through public API misuse or malformed host logic.

### `AcquireTimer`

```go
func AcquireTimer(timeout time.Duration) *time.Duration) *time.Timer {
	v := timerPool.Get()
	if v == nil {
		return time.NewTimer(timeout)
	}
	t := v.(*time.Timer)
	initTimer(t, timeout)
	return t
}
```

Two issues are relevant:

1. **Unchecked interface type assertion**

   `timerPool.Get()` returns `interface{}`. The code casts it with:

   ```go
   t := v.(*time.Timer)
   ```

   without using the comma-ok form. If the pool ever contains a value that is not `*time.Timer`, this panics.

   In the current codebase, `timerPool` is unexported and only populated by `ReleaseTimer`, which puts `*time.Timer`. Therefore, this is not directly attacker-controlled through external untrusted data. It is mostly a robustness issue, but it could become exploitable if future code, a race, or a maintenance mistake injects the wrong type into the pool.

   Severity: low.

2. **No timeout validation**

   `timeout` is passed directly to `time.NewTimer` or `time.Timer.Reset`. Negative durations are generally clamped or treated as immediate expiry by the runtime, and huge durations may simply create long-pending timers. There is no explicit normalization, minimum, or maximum bound.

   This is not a memory-safety bug, but it can lead to surprising timeout behavior or resource retention if host code derives timeouts from untrusted input.

   Severity: low.

### `ReleaseTimer`

```go
func ReleaseTimer(t *time.Timer) {
	stopTimer(t)
	timerPool.Put(t)
}
```

This is the most significant public API issue.

It does not check whether `t` is nil. Passing nil causes `stopTimer(t)` to dereference a nil pointer, leading to a panic.

It also does not track whether a timer has already been released. Calling `ReleaseTimer` twice on the same timer can put the same timer into `sync.Pool` multiple times. Later, two different goroutines may acquire the same underlying `time.Timer`, causing concurrent use, data races, or the active-timer panic in `initTimer`.

Severity: medium for the nil dereference; low to medium for double-release lifecycle misuse.

### `timerPool`

```go
var timerPool sync.Pool
```

`sync.Pool` is untyped. The safety of the pool depends entirely on the invariant that only `*time.Timer` values are inserted. Currently that invariant is maintained by `ReleaseTimer`.

This is not an exploitable vulnerability by itself, but it is why the unchecked assertion in `AcquireTimer` is worth noting.

## Overall assessment

There is no classic parser memory corruption, integer overflow, out-of-bounds write, or unsafe pointer arithmetic in this file. The practical risks are panic-DoS and lifetime-management issues in the pooled timer API:

- nil timer passed to `ReleaseTimer`,
- duplicate release causing the same timer to be acquired concurrently,
- unchecked pool type assertion,
- deliberate panic if an active timer is reused,
- unvalidated timeout values.

```json
[
  {
    "severity": "medium",
    "title": "ReleaseTimer panics on nil timer",
    "function": "ReleaseTimer",
    "description": "ReleaseTimer calls stopTimer without checking whether t is nil. stopTimer dereferences t.Stop() and t.C, so passing a nil timer causes a nil pointer dereference panic. A public API caller can crash the host process if nil reaches this function through buggy or malicious usage."
  },
  {
    "severity": "low",
    "title": "Unchecked interface type assertion in AcquireTimer",
    "function": "AcquireTimer",
    "description": "AcquireTimer casts the value returned by timerPool.Get() to *time.Timer without using the comma-ok assertion form. If the sync.Pool ever contains a non-time.Timer value, the function panics. The risk is currently low because timerPool is unexported and normally populated only by ReleaseTimer, but it is a robustness hazard for a public pooled API."
  },
  {
    "severity": "low",
    "title": "initTimer can panic if a pooled timer is still active",
    "function": "initTimer",
    "description": "initTimer panics if t.Reset(timeout) returns true, indicating the timer was active. Under correct Acquire/Release usage this should not happen, but double ReleaseTimer, concurrent reuse, or use of a released timer can violate the pool invariant and trigger a panic-DoS in timeout code paths."
  },
  {
    "severity": "low",
    "title": "AcquireTimer does not validate timeout duration",
    "function": "AcquireTimer",
    "description": "The timeout argument is passed directly to time.NewTimer or time.Timer.Reset without normalization or bounds checking. Negative values may create immediately expired timers and very large values may keep timers pending longer than intended. If timeouts are derived from untrusted input, this can cause surprising behavior or resource retention."
  }
]
```