Fix infinite recursion when parsing HTTP chunks #1553

This change stops infinite HttpStateData recursion with at-max-capacity inBuf. Such inBuf prevents progress in the following call chain: * processReply() * processReplyBody() and decodeAndWriteReplyBody() * maybeReadVirginBody() * maybeMakeSpaceAvailable() -- tries but fails to quit processing * processReply() To stop recursion, decodeAndWriteReplyBodyI() now predicts that maybeMakeSpaceAvailable() will detect at-max-capacity inBuf and breaks the recursion by declaring a fatal chunked parsing error. XXX: This change addresses a few known infinite recursion cases, but it is conceptually wrong for several (partially overlapping) reasons: 1. It assumes that at-max-capacity inBuf is a permanent state (i.e. things cannot improve with time). This assumption is false. For example, as more space frees up in the adaptation pipeline, we will clear responseBodyBuffer, and Client::calcBufferSpaceToReserve() may allow us to grow inBuf and read more chunked metadata. 2. New decodeAndWriteReplyBody() code assumption that inability (or lack of necessity) to provide usable buffer space implies that chunk metadata is huge is false: calcReadBufferSpaceToReserve() may become zero even with tiny buffered metadata. 3. The resulting recursion termination is arguably accidental. The call loop remains intact, and minor code changes (or unknown use cases) may resurrect the same infinite recursion problem. ---- This change is based on small portions of the stuck official PR squid-cache#736 (i.e. old Factory branch SQUID-533-Optimize-memalloc-server-reads). It does not include many important fixes and improvements from that branch.

I am reverting previous commit b631d5a9c933fa849f6887c433020d0125bc08f4 to address the problems identified in that commit message.

This change stops infinite HttpStateData recursion with at-max-capacity inBuf. Such inBuf prevents progress in the following call chain: * processReply() * processReplyBody() and decodeAndWriteReplyBody() * maybeReadVirginBody() * maybeMakeSpaceAvailable() -- tries but fails to quit processing * processReply() HttpStateData::maybeMakeSpaceAvailable() no longer calls processReply(), preventing recursion. maybeReadVirginBody() now aborts transactions that would otherwise get stalled due to full read buffer at its maximum capacity. This change requires that all maybeReadVirginBody() callers do actually need more response data to make progress. AFAICT, that (natural) invariant holds. We moved transaction stalling check from maybeMakeSpaceAvailable() into its previous callers. Without that move, maybeMakeSpaceAvailable() would have to handle both abortTransaction() and delayRead() cases. Besides increased code complexity, that would trigger some premature delayRead() calls (at maybeReadVirginBody() time). Deciding whether to delay socket reads is complicated, the delay mechanism is expensive, and delaying may become unnecessary by the time the socket becomes readable, so it is best to continue to only delayRead() at readReply() time, when there is no other choice left. maybeReadVirginBody() mishandled cases where progress was possible, but not _immediately_ -- it did nothing in those cases, probably stalling transactions when maybeMakeSpaceAvailable() returned false but did not call processReply(). This is now fixed: maybeReadVirginBody() now starts waiting for the socket to be ready for reading in those cases, effectively passing control to readReply() that handles them. maybeReadVirginBody() prematurely grew buffer for future socket reads. As a (positive) side effect of the above refactoring, we now delay buffer growth until the actual read(2) time, which is best for performance. Most likely, this premature buffer growth was an accident: maybeReadVirginBody() correctly called maybeMakeSpaceAvailable() with doGrow set to false. However, maybeMakeSpaceAvailable() misinterpreted doGrow as a "do not actually do it" parameter. That bug is now gone. This recursion bug was discovered and detailed by Joshua Rogers at https://megamansec.github.io/Squid-Security-Audit/ where it was filed as "Chunked Encoding Stack Overflow".

These should probably wait until we resume official PR squid-cache#736 (i.e. old Factory branch SQUID-533-Optimize-memalloc-server-reads) work or start pushing tiny minimal PRs in preparation for that.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix infinite recursion when parsing HTTP chunks #1553

Fix infinite recursion when parsing HTTP chunks #1553

Commits on Oct 29, 2023