Fix for 'with active responder' llerrs crash.

This fixes at least one case (crash report 8407), which comes
down to not cleanly informing a responder of failure when the
request url is empty (or so badly formed that it isn't a valid
url). As a result, the statemachine would abort() without
informing the responder - which is bad, sort of.

The previous cases where the responder needed to be informed
of a failure, namely "statemachine timed_out()" and "bad_socket()"
when a socket suddenly becomes bad for unknown reason, have been
replaced with the more general 'aborted()' function, which must
be called before the statemachine calls abort(). Clearly this
has been done for all cases of abort() now, so that if the
llerrs fires again in the future then that would have to be
after the statemachine calls finish(), which is still as "impossible"
as it was - hence the llerrs is still there to make sure.

The reason that this seldom happened on SL, and more often on
opensim, even more often on home-brew test grids, seems plausible:
malformed urls happen more in those cases.

I also took the opportunity to improve the robustness of cases
where the curl error code is checked: it makes no sense to check
what curl gives as error code when an internal error occurred.
This commit is contained in:
Aleric Inglewood
2013-11-18 18:07:51 +01:00
parent bddb1ba48c
commit 7b9f854c66
6 changed files with 28 additions and 34 deletions

View File

@@ -52,7 +52,6 @@
#include "llsdserialize.h"
#include "aithreadsafe.h"
#include "llqueuedthread.h"
#include "lltimer.h" // ms_sleep
#include "llproxy.h"
#include "llhttpstatuscodes.h"
#include "aihttpheaders.h"
@@ -1287,8 +1286,9 @@ BufferedCurlEasyRequest::~BufferedCurlEasyRequest()
// If the responder is still alive, then that means that BufferedCurlEasyRequest::processOutput was
// never called, which means that the removed_from_multi_handle event never happened.
// This is definitely an internal error as it can only happen when libcurl is too slow,
// in which case AICurlEasyRequestStateMachine::mTimer times out, but that already
// calls BufferedCurlEasyRequest::timed_out().
// in which case AICurlEasyRequestStateMachine::mTimer times out, a socket goes bad, or
// the state machine is aborted, but those already call BufferedCurlEasyRequest::aborted()
// which sets mResponder to NULL.
llmaybeerrs << "Calling ~BufferedCurlEasyRequest() with active responder!" << llendl;
if (!LLApp::isRunning())
{
@@ -1298,30 +1298,23 @@ BufferedCurlEasyRequest::~BufferedCurlEasyRequest()
else
{
// User chose to continue.
timed_out();
aborted(HTTP_INTERNAL_ERROR_OTHER, "BufferedCurlEasyRequest destructed with active responder");
}
}
--AICurlInterface::Stats::BufferedCurlEasyRequest_count;
}
void BufferedCurlEasyRequest::timed_out(void)
void BufferedCurlEasyRequest::aborted(U32 http_status, std::string const& reason)
{
mResponder->finished(CURLE_OK, HTTP_INTERNAL_ERROR_CURL_LOCKUP, "Request timeout, aborted.", sChannels, mOutput);
if (mResponder->needsHeaders())
if (mResponder)
{
send_buffer_events_to(NULL); // Revoke buffer events: we send them to the responder.
mResponder->finished(CURLE_OK, http_status, reason, sChannels, mOutput);
if (mResponder->needsHeaders())
{
send_buffer_events_to(NULL); // Revoke buffer events: we send them to the responder.
}
mResponder = NULL;
}
mResponder = NULL;
}
void BufferedCurlEasyRequest::bad_socket(void)
{
mResponder->finished(CURLE_OK, HTTP_INTERNAL_ERROR_CURL_BADSOCKET, "File descriptor went bad! Aborted.", sChannels, mOutput);
if (mResponder->needsHeaders())
{
send_buffer_events_to(NULL); // Revoke buffer events: we send them to the responder.
}
mResponder = NULL;
}
#if defined(CWDEBUG) || defined(DEBUG_CURLIO)

View File

@@ -121,6 +121,7 @@ void AICurlEasyRequestStateMachine::multiplex_impl(state_type run_state)
bool empty_url = AICurlEasyRequest_rat(*mCurlEasyRequest)->getLowercaseServicename().empty();
if (empty_url)
{
AICurlEasyRequest_wat(*mCurlEasyRequest)->aborted(HTTP_INTERNAL_ERROR_OTHER, "Not a valid URL.");
abort();
break;
}
@@ -195,16 +196,14 @@ void AICurlEasyRequestStateMachine::multiplex_impl(state_type run_state)
case AICurlEasyRequestStateMachine_removed:
{
// The request was removed from the multi handle.
if (mTimedOut)
{
AICurlEasyRequest_wat easy_request_w(*mCurlEasyRequest);
easy_request_w->timed_out();
}
// We're done. If we timed out, abort -- or else the application will
// think that getResult() will return a valid error code from libcurl.
if (mTimedOut)
{
AICurlEasyRequest_wat(*mCurlEasyRequest)->aborted(HTTP_INTERNAL_ERROR_CURL_LOCKUP, "Request timeout, aborted.");
abort();
}
else
finish();
@@ -212,7 +211,7 @@ void AICurlEasyRequestStateMachine::multiplex_impl(state_type run_state)
}
case AICurlEasyRequestStateMachine_bad_file_descriptor:
{
AICurlEasyRequest_wat(*mCurlEasyRequest)->bad_socket();
AICurlEasyRequest_wat(*mCurlEasyRequest)->aborted(HTTP_INTERNAL_ERROR_CURL_BADSOCKET, "File descriptor went bad! Aborted.");
abort();
}
}
@@ -224,7 +223,7 @@ void AICurlEasyRequestStateMachine::abort_impl(void)
// Revert call to addRequest() if that was already called (and the request wasn't removed again already).
if (mAdded)
{
// Note that it's safe to call this even if the curl thread already removed it, or will removes it
// Note that it's safe to call this even if the curl thread already removed it, or will remove it
// after we called this, before processing the remove command; only the curl thread calls
// MultiHandle::remove_easy_request, which is a no-op when called twice for the same easy request.
mAdded = false;

View File

@@ -385,11 +385,8 @@ class BufferedCurlEasyRequest : public CurlEasyRequest {
buffer_ptr_t& getInput(void) { return mInput; }
buffer_ptr_t& getOutput(void) { return mOutput; }
// Called if libcurl doesn't deliver within AIHTTPTimeoutPolicy::mMaximumTotalDelay seconds.
void timed_out(void);
// Called if the underlaying socket went bad (ie, when accidently closed by a buggy library).
void bad_socket(void);
// Called if the state machine is (about to be) aborted due to some error.
void aborted(U32 http_status, std::string const& reason);
// Called after removed_from_multi_handle was called.
void processOutput(void);
@@ -462,7 +459,7 @@ class BufferedCurlEasyRequest : public CurlEasyRequest {
bool success(void) const { return mResult == CURLE_OK && mStatus >= 200 && mStatus < 400; }
// Return true when prepRequest was already called and the object has not been
// invalidated as a result of calling timed_out().
// invalidated as a result of calling aborted().
bool isValid(void) const { return mResponder; }
// Return the capability type of this request.

View File

@@ -636,7 +636,7 @@ static LLSD blocking_request(
response["body"] = responder->getLLSD();
}
}
else if (result == CURLE_OK)
else if (result == CURLE_OK && !is_internal_http_error(http_status))
{
// We expect 404s, don't spam for them.
if (http_status != 404)

View File

@@ -298,6 +298,11 @@ LLUserAuth::UserAuthcode LLUserAuth::authResponse()
// if curl was ok, parse the download area.
CURLcode result = mResponder->result_code();
if (is_internal_http_error(mResponder->http_status()))
{
// result can be a meaningless CURLE_OK in the case of an internal error.
result = CURLE_FAILED_INIT; // Just some random error to get the default case below.
}
switch (result)
{
case CURLE_OK:

View File

@@ -168,7 +168,7 @@ void XMLRPCResponder::completed_headers(U32 status, std::string const& reason, A
void XMLRPCResponder::completedRaw(U32 status, std::string const& reason, LLChannelDescriptors const& channels, buffer_ptr_t const& buffer)
{
if (mCode == CURLE_OK)
if (mCode == CURLE_OK && !is_internal_http_error(status))
{
mBufferSize = buffer->count(channels.in());
if (200 <= status && status < 400)