Skip to content

Commit

Permalink
Just close after a write(2) response sending error (#1582)
Browse files Browse the repository at this point in the history
    FATAL: assertion failed: Http1Server.cc:322: "rep"

2015 commit 21cd322 started to continue ClientStream processing after
socket write(2) failures. In most cases, the code still "worked". For
example, initiateClose() would close the client-Squid connection, and
connStateClosed() would be called before Store has a chance to deliver
response body data requested by pullData() in writeComplete().

However, that response body data could sometimes reach Server, and
handleReply() would assert because startOfOutput() says that we have not
written the headers, but ClientStream state (i.e. a nil `rep` parameter)
says that we have. These assertion can be triggered by disabling
initiateClose(), and they can probably be triggered by traffic as well.

Now, after a Comm::Write() error, we terminateAll() client transactions
on the failed connection[^1] and do not call afterClientWrite() that is
not equipped to handle I/O errors and would continue ClientStream
processing if called.

This bug was discovered and detailed by Joshua Rogers at
https://megamansec.github.io/Squid-Security-Audit/stream-assert.html
where it was filed as "Implicit Assertion in Stream Handling".

[^1]: We terminateAll() instead of potentially postponing closure with
initiateClose() because the failed client-Squid connection most likely
cannot be salvaged for, say, reading the remainder of the request body.
  • Loading branch information
rousskov authored and yadij committed Nov 28, 2023
1 parent 1820919 commit 290ae20
Showing 1 changed file with 8 additions and 2 deletions.
10 changes: 8 additions & 2 deletions src/servers/Server.cc
Original file line number Diff line number Diff line change
Expand Up @@ -204,8 +204,14 @@ Server::clientWriteDone(const CommIoCbParams &io)

Must(io.conn->fd == clientConnection->fd);

if (io.flag && pipeline.front())
pipeline.front()->initiateClose("write failure");
if (io.flag) {
debugs(33, 2, "bailing after a write failure: " << xstrerr(io.xerrno));
LogTagsErrors lte;
lte.timedout = io.xerrno == ETIMEDOUT;
lte.aborted = !lte.timedout; // intentionally true for zero io.xerrno
terminateAll(Error(ERR_WRITE_ERROR, SysErrorDetail::NewIfAny(io.xerrno)), lte);
return;
}

afterClientWrite(io.size); // update state
writeSomeData(); // maybe schedules another write
Expand Down

0 comments on commit 290ae20

Please sign in to comment.