[Bug Report] sync.Pool use-after-free causes cross-request stats corruption and request count miscounting

[Bug Report] sync.Pool use-after-free causes cross-request stats corruption and request count miscounting

TL;DR

The RoutingContext object is returned to sync.Pool via Delete() in deferred functions inside HandleResponseHeaders/HandleResponseBody, while Process() continues to hold and use the same pointer. When another goroutine picks up the same object from the pool, the original request's subsequent DoneRequestCount call operates on the new request's statsUpdated field, causing the new request's donePodStats to be skipped by CAS and pendingRequests to be double-counted.

Background: How I discovered this

I started by fixing an issue where runningRequests counts would become inaccurate — sometimes going negative — because Pod status updates (non-restart) would reset the counters, and then a late -1 from a historical request would push them below zero. I implemented a sync.Map-based approach to precisely track the relationship between Pods and request IDs (see the detailed fix guide).

After implementing this precise tracking, I discovered a much deeper problem: the tracking data showed that one request's -1 operation was being executed twice, while another request's -1 was being completely skipped. This led me to the root cause documented below.

Root Cause: sync.Pool use-after-free (Defect A)

The Mechanism

1. Request A receives a non-200 response → HandleResponseHeaders deferred function executes:

   • DoneRequestCount() → CAS statsUpdated 1→2 succeeds ✅
   • routerCtx.Delete() → object returned to sync.Pool ⚠️
   • But Process()'s main loop is still running and still holds st.routerCtx pointing to the same object

2. Request B (different goroutine) calls requestPool.Get() → gets the exact same object that Request A just returned

3. Request A's Process() loop continues → srv.Recv() returns an error (e.g., context canceled) → calls DoneRequestCount(st.routerCtx, ...) → operates on Request B's statsUpdated! → CAS statsUpdated 1→2 succeeds (but this is Request B's state!)

4. Request B completes normally → HandleResponseBody defer calls DoneRequestTrace() → CAS statsUpdated 1→2 fails (already set to 2 by Request A!) → donePodStats is skipped → Request B's count is never decremented

Concrete Evidence (from tracing logs)

Time	Event	ctx_ptr	request_id	statsUpdated
10:48:46.943	Request A defer: Delete() → pool.Put	0xc005a7cf20	fbfbd6ae	2
10:48:47.082	Request B: pool.Get() → same object!	0xc005a7cf20	b2f760e8	old=2
10:48:49.028	Request A: DoneRequestCount on Request B's object	0xc005a7cf20	requestID=fbfbd6ae, ctx_request_id=b2f760e8	1→2 🔴
10:51:47.644	Request B: DoneRequestTrace CAS fails	0xc005a7cf20	b2f760e8	CAS 1→2 false 🔴

Affected Code Locations (v0.7.0)

• pkg/plugins/gateway/gateway_rsp_headers.go:37-43 — defer calls Delete() but Process() still holds st.routerCtx
• pkg/plugins/gateway/gateway_rsp_body.go:82-89 — same pattern
• pkg/plugins/gateway/gateway.go:236-257 (preRecvCheck) — calls DoneRequestCount(st.routerCtx, ...) on potentially freed object
• pkg/plugins/gateway/gateway.go:260-316 (handleRecvError) — same problem (5 exit paths)
• pkg/plugins/gateway/gateway.go:405-414 (sendProcessingResponse) — calls DoneRequestCount + Delete() on Send error, risking double pool.Put

Additional Related Defects

Defect B1: pendingRequests -1 is NOT protected by CAS

cache_impl.go:239-242 and cache_impl.go:269-272 — atomic.AddInt32(&meta.pendingRequests, -1) executes outside the if ctx != nil && ctx.CanDoneStats() block, meaning pendingRequests is decremented even when CAS fails. This means a single request can decrement pendingRequests multiple times (once per done path that fires).

Defect C1: traceAdded is NOT reset in reset()

router_context.go:366 — only statsUpdated is reset to statusInitial, but traceAdded is NOT reset. When an object is recycled from the pool, CanAddTrace() CAS 0→1 fails because traceAdded is still 1 from the previous request. This causes pendingRequests +1 to be skipped for the new request.

New in v0.7.0: Double pool.Put (Defect N1)

When isRespError=true, HandleResponseHeaders defer already calls Delete(). If srv.Send() also fails, sendProcessingResponse (gateway.go:412-414) calls st.routerCtx.Delete() again → same object is pool.Put twice, corrupting sync.Pool internal state.

Design Question

I have a question about the overall design approach: Why are DoneRequestCount, DoneRequestTrace, and routerCtx.Delete() scattered across multiple defer blocks in three different files (gateway.go, gateway_rsp_headers.go, gateway_rsp_body.go) instead of being centralized?

Specifically:

1. Delete() and DoneRequestCount/DoneRequestTrace are not atomic — some paths call only one of them
2. It's very easy for DoneRequestCount to execute after Delete() has already returned the object to the pool
3. There is no safety mechanism to ensure DoneRequestCount is never called on a ctx that has already been returned to the pool
4. Is there a specific reason for this scattered design, or was it an oversight? Are there plans to centralize the lifecycle management of RoutingContext?

Follow-up Question: Why call routerCtx.Delete() prematurely in gateway_rsp_headers.go's defer?

This is something I find particularly puzzling. Looking at gateway_rsp_headers.go:37-43:

defer func() {
    if isProcessingError {
        s.cache.DoneRequestCount(routerCtx, requestID, model, 0)
        if routerCtx != nil {
            routerCtx.Delete()   // ← returns the object to sync.Pool here
        }
    }
}()

This defer has two issues that I can't reason about:

Issue 1: Why call Delete() in the middle of the request lifecycle?

HandleResponseHeaders is just one intermediate step in Process()'s main loop for handling response headers. After this step, Process()'s loop continues to run — it may still need to process ResponseBody (HandleResponseBody), and may use routerCtx again in subsequent srv.Recv() or ctx.Done() exit paths. Returning routerCtx to sync.Pool at an intermediate step is like handing your room key back to the front desk while you're still inside the room — it's almost guaranteed that subsequent accesses to routerCtx will operate on another request's data.

Why not call Delete() at a single unified exit point in Process()'s main loop — right before the loop returns, when we're absolutely certain no more code will use routerCtx? That would guarantee Delete() always happens after the last use of routerCtx.

Issue 2: Why isn't routerCtx set to nil after Delete()?

Even though the routerCtx in the defer is a function parameter of HandleResponseHeaders (a different variable from st.routerCtx in Process()), at minimum within HandleResponseHeaders itself, if any code after the defer were to access routerCtx (e.g., if routerCtx != nil checks), it would operate on a pool-returned object. While the defer is currently the last thing to execute in the function, this safety relies on coincidence — if someone adds routerCtx usage after the defer in the future, a bug is introduced.

More critically, Process()'s st.routerCtx points to the same object, and HandleResponseHeaders's defer calling Delete() is completely invisible to st.routerCtx — st.routerCtx remains non-nil, pointing to an object that has already been returned to the pool. All subsequent if st.routerCtx != nil checks in Process() will pass, and code will continue to operate on this "zombie" object.

To summarize my confusion:

What was the design rationale behind placing Delete() in HandleResponseHeaders/HandleResponseBody's defer, rather than at a unified exit point in Process()'s main loop? Is there a specific scenario that requires the object to be returned to the pool immediately at that point? Or was the initial implementation done without fully considering that the Process() loop would continue to use routerCtx afterwards?

Suggested Fix Direction

The core principle should be: Once a RoutingContext object is Delete()'d back to the pool, it MUST never be used again by any code.

A possible approach:

1. Introduce a routerCtxDeleted flag in processState to track whether Delete() has been called
2. All exit paths check this flag before calling DoneRequestCount or Delete()
3. Centralize RoutingContext lifecycle management so that DoneRequestCount + Delete() always happen atomically in one place
4. Move pendingRequests -1 inside the CAS success block
5. Reset traceAdded in reset() using atomic.StoreInt32

Related: This issue is related to another issue I previously filed #2421 — both concern request counting and RoutingContext lifecycle management. @googs1025, would appreciate your attention and a reasonable explanation on these. Thanks!

Impact

• Cross-request data corruption: Request A modifies Request B's statsUpdated, causing Request B's donePodStats to never execute
• pendingRequests systematic drift: Multiple decrements per request + skipped increments → pendingRequests drifts negative over time
• sync.Pool corruption: Double pool.Put can corrupt the pool's internal linked list
• Memory leak: Multiple exit paths never call Delete() at all

Environment

• Version: v0.7.0 (also confirmed in v0.5.0)

Read the lifecycle paths in pkg/plugins/gateway/gateway.go, gateway_rsp_headers.go, gateway_rsp_body.go, cache_impl.go, and router_context.go, focusing on Process, preRecvCheck, handleRecvError, sendProcessingResponse, and reset. Trace every DoneRequestCount, DoneRequestTrace, and Delete call before choosing a lifecycle design. Done means pooled contexts are not reused while still referenced, completion counters are updated once, and trace state is reset safely.