Hello,
The attached patch avoids assertions on large FTP commands and
cleans up both general and FTP-specific error handling code during
request parsing. Please see the patch preamble for technical details.
Thank you,
Alex.
Do not assert on native FTP ERR_TOO_BIG. Do not check for ERR_TOO_BIG twice.
The assertion occurred because both the FTP request parser and the generic
ConnStateData::checkHeaderLimits() code would try to write their own error
message to the user. Reworked all error reporting code in the FTP parser to
avoid writing early responses (that were bypassing the overall transaction
flow with various negative side effects such as lack of logging).
Removed ConnStateData::checkHeaderLimits(): We already have protocol-specific
checks for huge HTTP and FTP requests. There is no point in duplicating them.
Centralizing them sounds like a good idea, but a general checkHeaderLimits()
cannot produce protocol-specific errors messages that we need, so it hurts
more than it helps. Moreover, checkHeaderLimits() was handling errors
differently than protocol parsing code, making the code more complex overall.
All that remains from the checkHeaderLimits() code now is a single Must(),
checking that the protocol parsers did what they were supposed to do: Return
NULL to request more data after checking any applicable limits. If parsers do
not (a Squid bug!), the ConnStateData job gets killed (and connection gets
closed) as the last resort.
Added clientReplyContext::setReplyToReply() and
StoreEntry::storeErrorResponse() to handle storing of a response to an FTP
command parsing error. The old code was using ErrorState to store parsing
errors, but ErrorState is still HTTP-specific and cannot relay the right FTP
codes/reasons to the user. The setReplyToReply() sounds silly but it matches
the existing setReplyTo*() naming scheme well.
Make sure parsed native FTP command tokens are not even close to the String
buffer limit. These checks are not a firm guarantee, but are better than
nothing until we replace String.
Handle ClientSocketContext registration centrally because all parsers need it.
Call quitAfterError() on fatal native FTP errors. Probably not necessary due
to fssError handling code that closes the FTP control connection, but adds
helpful debugging and brings us closer to the HTTP error handling code.
Described ConnStateData::clientParseRequests().
=== modified file 'src/Store.h'
--- src/Store.h 2014-06-25 00:14:36 +
+++ src/Store.h 2014-08-12 17:42:51 +
@@ -80,40 +80,42 @@ public:
StoreEntry();
virtual ~StoreEntry();
virtual HttpReply const *getReply() const;
virtual void write (StoreIOBuffer);
/** Check if the Store entry is emtpty
* \retval true Store contains 0 bytes of data.
* \retval false Store contains 1 or more bytes of data.
* \retval false Store contains negative content !!
*/
virtual bool isEmpty() const {
assert (mem_obj);
return mem_obj-endOffset() == 0;
}
virtual bool isAccepting() const;
virtual size_t bytesWanted(Rangesize_t const aRange, bool ignoreDelayPool = false) const;
virtual void complete();
virtual store_client_t storeClientType() const;
virtual char const *getSerialisedMetaData();
+/// Store a prepared error response. MemObject locks the reply object.
+void storeErrorResponse(HttpReply *reply);
void replaceHttpReply(HttpReply *, bool andStartWriting = true);
void startWriting(); /// pack and write reply headers and, maybe, body
/// whether we may start writing to disk (now or in the future)
virtual bool mayStartSwapOut();
virtual void trimMemory(const bool preserveSwappable);
// called when a decision to cache in memory has been made
void memOutDecision(const bool willCacheInRam);
// called when a decision to cache on disk has been made
void swapOutDecision(const MemObject::SwapOut::Decision decision);
void abort();
void unlink();
void makePublic();
void makePrivate();
void setPublicKey();
void setPrivateKey();
void expireNow();
void releaseRequest();
void negativeCache();
=== modified file 'src/client_side.cc'
--- src/client_side.cc 2014-08-11 16:09:06 +
+++ src/client_side.cc 2014-08-12 23:20:15 +
@@ -180,41 +180,40 @@ public:
private:
AnyP::PortCfgPointer portCfg; /// from HttpPortList
Ipc::FdNoteId portTypeNote;/// Type of IPC socket being opened
Subscription::Pointer sub; /// The handler to be subscribed for this connetion listener
};
static void clientListenerConnectionOpened(AnyP::PortCfgPointer s, const Ipc::FdNoteId portTypeNote, const Subscription::Pointer sub);
/* our socket-related context */
CBDATA_CLASS_INIT(ClientSocketContext);
/* Local functions */
static IOCB clientWriteComplete;
static IOCB clientWriteBodyComplete;
static IOACB httpAccept;
#if USE_OPENSSL
static IOACB
