From 304fa05094932cae7ebc0a9de0fd8c3950e0ce59 Mon Sep 17 00:00:00 2001 From: Aleric Inglewood Date: Sun, 12 Aug 2012 16:34:20 +0200 Subject: [PATCH] Suppress "Expect:" header for POST and PUT. No longer use CURLOPT_COPYPOSTFIELDS. For POST and PUT, libcurl adds by default a "Expect: 100-continue" header and wait for the server to reply with a 100 response. However, since the server never does that (LL comment in the code "I'm not sure what it means") it's up to libcurl to continue anyway after a while and that part is apparently bugged, causing people not to be able to login sometimes. But suppressing the header, libcurl doesn't wait for it and we worked around this bug. The commit introduces a much improved CurlEasyRequest::setPost method that enforces the above for POST, but that also no longer uses CURLOPT_COPYPOSTFIELDS (but CURLOPT_POSTFIELDS), no longer making a copy of the body of what we are going to send to the server. Instead it uses a new object, derived from AIPostField, to keep track of this data and to dispose of it once the transaction is complete (and no sooner). Copied from llcorehttp, we now also always set a "Connection: keep-alive" and "Keep-alive: 300" header for POST. This was already done for texture downloads (HttpOpRequest), but now we do it always :p. Might need to be changed in the future, but currently those headers are ignored by the server anyway. --- indra/llmessage/aicurl.cpp | 53 +++++++++++++++++++++++---- indra/llmessage/aicurl.h | 15 ++++++++ indra/llmessage/aicurlprivate.h | 10 ++++- indra/llmessage/llhttpclient.cpp | 21 +++-------- indra/llmessage/llurlrequest.cpp | 14 ++----- indra/newview/llcurlrequest.cpp | 4 +- indra/newview/llxmlrpctransaction.cpp | 27 ++++++-------- 7 files changed, 91 insertions(+), 53 deletions(-) diff --git a/indra/llmessage/aicurl.cpp b/indra/llmessage/aicurl.cpp index daba6f923..ceb1000d5 100644 --- a/indra/llmessage/aicurl.cpp +++ b/indra/llmessage/aicurl.cpp @@ -701,6 +701,7 @@ CURLMcode CurlEasyHandle::remove_handle_from_multi(AICurlEasyRequest_wat& curl_e mActiveMultiHandle = NULL; CURLMcode res = check_multi_code(curl_multi_remove_handle(multi, mEasyHandle)); removed_from_multi_handle(curl_easy_request_w); + mPostField = NULL; return res; } @@ -796,11 +797,33 @@ void CurlEasyRequest::setoptString(CURLoption option, std::string const& value) setopt(option, value.c_str()); } -void CurlEasyRequest::setPost(char const* postdata, S32 size) +void CurlEasyRequest::setPost(AIPostFieldPtr const& postdata, S32 size) { - setopt(CURLOPT_POST, 1L); + llassert_always(postdata->data()); + + Dout(dc::curl, "POST size is " << size << " bytes: \"" << libcwd::buf2str(postdata->data(), size) << "\"."); + setPostField(postdata); // Make sure the data stays around until we don't need it anymore. + + setPost_raw(size, postdata->data()); +} + +void CurlEasyRequest::setPost_raw(S32 size, char const* data) +{ + if (!data) + { + // data == NULL when we're going to read the data using CURLOPT_READFUNCTION. + Dout(dc::curl, "POST size is " << size << " bytes."); + } + + // The server never replies with 100-continue, so suppress the "Expect: 100-continue" header that libcurl adds by default. + addHeader("Expect:"); + if (size > 0) + { + addHeader("Connection: keep-alive"); + addHeader("Keep-alive: 300"); + } setopt(CURLOPT_POSTFIELDSIZE, size); - setopt(CURLOPT_POSTFIELDS, postdata); + setopt(CURLOPT_POSTFIELDS, data); } ThreadSafeCurlEasyRequest* CurlEasyRequest::get_lockobj(void) @@ -1133,8 +1156,23 @@ void CurlEasyRequest::finalizeRequest(std::string const& url) { llassert(!mRequestFinalized); mResult = CURLE_FAILED_INIT; // General error code, the final code is written here in MultiHandle::check_run_count when msg is CURLMSG_DONE. - mRequestFinalized = true; lldebugs << url << llendl; +#ifdef SHOW_ASSERT + // Do a sanity check on the headers. + int content_type_count = 0; + for (curl_slist* list = mHeaders; list; list = list->next) + { + if (strncmp(list->data, "Content-Type:", 13) == 0) + { + ++content_type_count; + } + } + if (content_type_count > 1) + { + llwarns << content_type_count << " Content-Type: headers!" << llendl; + } +#endif + mRequestFinalized = true; setopt(CURLOPT_HTTPHEADER, mHeaders); setoptString(CURLOPT_URL, url); // The following line is a bit tricky: we store a pointer to the object without increasing its reference count. @@ -1270,7 +1308,8 @@ void CurlResponderBuffer::prepRequest(AICurlEasyRequest_wat& curl_easy_request_w { if (post) { - curl_easy_request_w->setoptString(CURLOPT_ENCODING, ""); + // Accept everything (send an Accept-Encoding header containing all encodings we support (zlib and gzip)). + curl_easy_request_w->setoptString(CURLOPT_ENCODING, ""); // CURLOPT_ACCEPT_ENCODING } mInput.reset(new LLBufferArray); @@ -1303,9 +1342,7 @@ void CurlResponderBuffer::prepRequest(AICurlEasyRequest_wat& curl_easy_request_w if (!post) { - curl_easy_request_w->addHeader("Connection: keep-alive"); - curl_easy_request_w->addHeader("Keep-alive: 300"); - // Add other headers. + // Add extra headers. for (std::vector::const_iterator iter = headers.begin(); iter != headers.end(); ++iter) { curl_easy_request_w->addHeader((*iter).c_str()); diff --git a/indra/llmessage/aicurl.h b/indra/llmessage/aicurl.h index 895650ec8..26491da67 100644 --- a/indra/llmessage/aicurl.h +++ b/indra/llmessage/aicurl.h @@ -247,6 +247,21 @@ struct AICurlEasyHandleEvents { virtual ~AICurlEasyHandleEvents() { } }; +// Pointer to data we're going to POST. +class AIPostField : public LLThreadSafeRefCount { + protected: + char const* mData; + + public: + AIPostField(char const* data) : mData(data) { } + char const* data(void) const { return mData; } +}; + +// The pointer to the data that we have to POST is passed around as AIPostFieldPtr, +// which causes it to automatically clean up when there are no pointers left +// pointing to it. +typedef LLPointer AIPostFieldPtr; + #include "aicurlprivate.h" // AICurlPrivate::CurlEasyRequestPtr, a boost::intrusive_ptr, is no more threadsafe than a diff --git a/indra/llmessage/aicurlprivate.h b/indra/llmessage/aicurlprivate.h index 309992973..a475504b1 100644 --- a/indra/llmessage/aicurlprivate.h +++ b/indra/llmessage/aicurlprivate.h @@ -143,6 +143,7 @@ class CurlEasyHandle : public boost::noncopyable, protected AICurlEasyHandleEven CURL* mEasyHandle; CURLM* mActiveMultiHandle; char* mErrorBuffer; + AIPostFieldPtr mPostField; // This keeps the POSTFIELD data alive for as long as the easy handle exists. bool mQueuedForRemoval; // Set if the easy handle is (probably) added to the multi handle, but is queued for removal. #ifdef SHOW_ASSERT public: @@ -190,6 +191,9 @@ class CurlEasyHandle : public boost::noncopyable, protected AICurlEasyHandleEven // Return the underlying curl easy handle. CURL* getEasyHandle(void) const { return mEasyHandle; } + // Keep POSTFIELD data alive. + void setPostField(AIPostFieldPtr const& post_field_ptr) { mPostField = post_field_ptr; } + private: // Return, and possibly create, the curl (easy) error buffer used by the current thread. static char* getTLErrorBuffer(void); @@ -208,9 +212,13 @@ class CurlEasyHandle : public boost::noncopyable, protected AICurlEasyHandleEven // AICurlEasyRequest is deleted, then also the ThreadSafeCurlEasyRequest is deleted // and the CurlEasyRequest destructed. class CurlEasyRequest : public CurlEasyHandle { + private: + void setPost_raw(S32 size, char const* data); public: + void setPost(S32 size) { setPost_raw(size, NULL); } + void setPost(AIPostFieldPtr const& postdata, S32 size); + void setPost(char const* data, S32 size) { setPost(new AIPostField(data), size); } void setoptString(CURLoption option, std::string const& value); - void setPost(char const* postdata, S32 size); void addHeader(char const* str); private: diff --git a/indra/llmessage/llhttpclient.cpp b/indra/llmessage/llhttpclient.cpp index 9c58bc466..5e201710f 100644 --- a/indra/llmessage/llhttpclient.cpp +++ b/indra/llmessage/llhttpclient.cpp @@ -443,7 +443,6 @@ static LLSD blocking_request( AICurlEasyRequest_wat curlEasyRequest_w(*easy_request); LLHTTPBuffer http_buffer; - std::string body_str; // * Set curl handle options curlEasyRequest_w->setopt(CURLOPT_TIMEOUT, (long)timeout); // seconds, see warning at top of function. @@ -463,6 +462,9 @@ static LLSD blocking_request( } } + // Needs to stay alive until after the call to perform(). + std::ostringstream ostr; + // * Setup specific method / "verb" for the URI (currently only GET and POST supported + poppy) if (method == LLURLRequest::HTTP_GET) { @@ -470,24 +472,13 @@ static LLSD blocking_request( } else if (method == LLURLRequest::HTTP_POST) { - curlEasyRequest_w->setopt(CURLOPT_POST, 1); - //serialize to ostr then copy to str - need to because ostr ptr is unstable :( - std::ostringstream ostr; - LLSDSerialize::toXML(body, ostr); - body_str = ostr.str(); - curlEasyRequest_w->setopt(CURLOPT_POSTFIELDS, body_str.c_str()); //copied from PHP libs, correct? curlEasyRequest_w->addHeader("Content-Type: application/llsd+xml"); - - // copied from llurlrequest.cpp - // it appears that apache2.2.3 or django in etch is busted. If - // we do not clear the expect header, we get a 500. May be - // limited to django/mod_wsgi. - curlEasyRequest_w->addHeader("Expect:"); + LLSDSerialize::toXML(body, ostr); + curlEasyRequest_w->setPost(ostr.str().c_str(), ostr.str().length()); } // * Do the action using curl, handle results - lldebugs << "HTTP body: " << body_str << llendl; curlEasyRequest_w->addHeader("Accept: application/llsd+xml"); curlEasyRequest_w->finalizeRequest(url); @@ -500,7 +491,7 @@ static LLSD blocking_request( llwarns << "CURL REQ URL: " << url << llendl; llwarns << "CURL REQ METHOD TYPE: " << method << llendl; llwarns << "CURL REQ HEADERS: " << headers.asString() << llendl; - llwarns << "CURL REQ BODY: " << body_str << llendl; + llwarns << "CURL REQ BODY: " << ostr.str() << llendl; llwarns << "CURL HTTP_STATUS: " << http_status << llendl; llwarns << "CURL ERROR: " << curlEasyRequest_w->getErrorString() << llendl; llwarns << "CURL ERROR BODY: " << http_buffer.asString() << llendl; diff --git a/indra/llmessage/llurlrequest.cpp b/indra/llmessage/llurlrequest.cpp index 91021186f..c59c5bb3a 100644 --- a/indra/llmessage/llurlrequest.cpp +++ b/indra/llmessage/llurlrequest.cpp @@ -526,6 +526,7 @@ bool LLURLRequest::configure() curlEasyRequest_w->setopt(CURLOPT_FOLLOWLOCATION, 1); rv = true; break; + case HTTP_GET: curlEasyRequest_w->setopt(CURLOPT_HTTPGET, 1); curlEasyRequest_w->setopt(CURLOPT_FOLLOWLOCATION, 1); @@ -538,24 +539,15 @@ bool LLURLRequest::configure() case HTTP_PUT: // Disable the expect http 1.1 extension. POST and PUT default // to turning this on, and I am not too sure what it means. - addHeader("Expect:"); - + curlEasyRequest_w->addHeader("Expect:"); curlEasyRequest_w->setopt(CURLOPT_UPLOAD, 1); curlEasyRequest_w->setopt(CURLOPT_INFILESIZE, bytes); rv = true; break; case HTTP_POST: - // Disable the expect http 1.1 extension. POST and PUT default - // to turning this on, and I am not too sure what it means. - addHeader("Expect:"); - - // Disable the content type http header. - // *FIX: what should it be? - addHeader("Content-Type:"); - // Set the handle for an http post - curlEasyRequest_w->setPost(NULL, bytes); + curlEasyRequest_w->setPost(bytes); // Set Accept-Encoding to allow response compression curlEasyRequest_w->setoptString(CURLOPT_ENCODING, ""); diff --git a/indra/newview/llcurlrequest.cpp b/indra/newview/llcurlrequest.cpp index 0c566fed2..4084b729f 100644 --- a/indra/newview/llcurlrequest.cpp +++ b/indra/newview/llcurlrequest.cpp @@ -101,7 +101,7 @@ bool Request::post(std::string const& url, headers_t const& headers, std::string llassert_always(success); // AIFIXME: Maybe throw an error. if (!success) return false; - buffered_easy_request_w->setPost(NULL, bytes); + buffered_easy_request_w->setPost(bytes); buffered_easy_request_w->addHeader("Content-Type: application/octet-stream"); buffered_easy_request_w->finalizeRequest(url); @@ -129,7 +129,7 @@ bool Request::post(std::string const& url, headers_t const& headers, LLSD const& LLBufferStream buffer_stream(buffer_w->sChannels, buffer_w->getInput().get()); LLSDSerialize::toXML(data, buffer_stream); S32 bytes = buffer_w->getInput()->countAfter(buffer_w->sChannels.out(), NULL); - buffered_easy_request_w->setPost(NULL, bytes); + buffered_easy_request_w->setPost(bytes); buffered_easy_request_w->addHeader("Content-Type: application/llsd+xml"); buffered_easy_request_w->finalizeRequest(url); diff --git a/indra/newview/llxmlrpctransaction.cpp b/indra/newview/llxmlrpctransaction.cpp index b1bb2ee0f..227904f90 100644 --- a/indra/newview/llxmlrpctransaction.cpp +++ b/indra/newview/llxmlrpctransaction.cpp @@ -168,8 +168,6 @@ public: LLCurl::TransferInfo mTransferInfo; std::string mURI; - char* mRequestText; - int mRequestTextSize; std::string mProxyAddress; @@ -200,7 +198,6 @@ LLXMLRPCTransaction::Impl::Impl(const std::string& uri, : mCurlEasyRequestStateMachinePtr(NULL), mStatus(LLXMLRPCTransaction::StatusNotStarted), mURI(uri), - mRequestText(0), mResponse(0) { init(request, useGzip); @@ -212,7 +209,6 @@ LLXMLRPCTransaction::Impl::Impl(const std::string& uri, : mCurlEasyRequestStateMachinePtr(NULL), mStatus(LLXMLRPCTransaction::StatusNotStarted), mURI(uri), - mRequestText(0), mResponse(0) { XMLRPC_REQUEST request = XMLRPC_RequestNew(); @@ -223,8 +219,13 @@ LLXMLRPCTransaction::Impl::Impl(const std::string& uri, init(request, useGzip); } - - +// Store pointer to data allocated with XMLRPC_REQUEST_ToXML and call XMLRPC_Free to free it upon destruction. +class AIXMLRPCData : public AIPostField +{ + public: + AIXMLRPCData(char const* allocated_data) : AIPostField(allocated_data) { } + /*virtual*/ ~AIXMLRPCData() { XMLRPC_Free(const_cast(mData)); mData = NULL; } +}; void LLXMLRPCTransaction::Impl::init(XMLRPC_REQUEST request, bool useGzip) { @@ -259,12 +260,11 @@ void LLXMLRPCTransaction::Impl::init(XMLRPC_REQUEST request, bool useGzip) curlEasyRequest_w->setoptString(CURLOPT_ENCODING, ""); } - mRequestText = XMLRPC_REQUEST_ToXML(request, &mRequestTextSize); - if (mRequestText) + int requestTextSize; + char* requestText = XMLRPC_REQUEST_ToXML(request, &requestTextSize); + if (requestText) { - Dout(dc::curl, "Writing " << mRequestTextSize << " bytes: \"" << libcwd::buf2str(mRequestText, mRequestTextSize) << "\".");; - curlEasyRequest_w->setopt(CURLOPT_POSTFIELDSIZE, mRequestTextSize); - curlEasyRequest_w->setoptString(CURLOPT_COPYPOSTFIELDS, mRequestText); + curlEasyRequest_w->setPost(new AIXMLRPCData(requestText), requestTextSize); } else { @@ -298,11 +298,6 @@ LLXMLRPCTransaction::Impl::~Impl() { XMLRPC_RequestFree(mResponse, 1); } - - if (mRequestText) - { - XMLRPC_Free(mRequestText); - } } bool LLXMLRPCTransaction::Impl::is_finished(void) const