BUG-2295/MAINT-2624 unexpected crash around Content-Range: header processing

Not certain what the source of the short data is with one resident but I'm
going to make these problems retryable as they are transport-related.  Lift
the retry detection into a method that should be reusable by others interested
in determining what is retryable.  Trace output handling on the libcurl debug
callback was attrocious.  Some unsafe length handling on my part was protected
by a second layer of defense.  Made that correct and more useful by logging
actual data sizes during trace.

6 files changed, 121 insertions, 48 deletions

diff --git a/indra/llcorehttp/_httpoprequest.cpp b/indra/llcorehttp/_httpoprequest.cpp
index 12eed06b10..207ed8e1e4 100644
--- a/indra/llcorehttp/_httpoprequest.cpp
+++ b/indra/llcorehttp/_httpoprequest.cpp
@@ -669,7 +669,7 @@ int HttpOpRequest::debugCallback(CURL * handle, curl_infotype info, char * buffe
 	std::string safe_line;
 	std::string tag;
 	bool logit(false);
-	len = (std::min)(len, size_t(256));					// Keep things reasonable in all cases
+	const size_t log_len((std::min)(len, size_t(256)));		// Keep things reasonable in all cases
 	
 	switch (info)
 	{
@@ -677,7 +677,7 @@ int HttpOpRequest::debugCallback(CURL * handle, curl_infotype info, char * buffe
 		if (op->mTracing >= HTTP_TRACE_CURL_HEADERS)
 		{
 			tag = "TEXT";
-			escape_libcurl_debug_data(buffer, len, true, safe_line);
+			escape_libcurl_debug_data(buffer, log_len, true, safe_line);
 			logit = true;
 		}
 		break;
@@ -686,7 +686,7 @@ int HttpOpRequest::debugCallback(CURL * handle, curl_infotype info, char * buffe
 		if (op->mTracing >= HTTP_TRACE_CURL_HEADERS)
 		{
 			tag = "HEADERIN";
-			escape_libcurl_debug_data(buffer, len, true, safe_line);
+			escape_libcurl_debug_data(buffer, log_len, true, safe_line);
 			logit = true;
 		}
 		break;
@@ -695,7 +695,7 @@ int HttpOpRequest::debugCallback(CURL * handle, curl_infotype info, char * buffe
 		if (op->mTracing >= HTTP_TRACE_CURL_HEADERS)
 		{
 			tag = "HEADEROUT";
-			escape_libcurl_debug_data(buffer, 2 * len, true, safe_line);		// Goes out as one line
+			escape_libcurl_debug_data(buffer, log_len, true, safe_line);	// Goes out as one line unlike header_in
 			logit = true;
 		}
 		break;
@@ -707,7 +707,7 @@ int HttpOpRequest::debugCallback(CURL * handle, curl_infotype info, char * buffe
 			logit = true;
 			if (op->mTracing >= HTTP_TRACE_CURL_BODIES)
 			{
-				escape_libcurl_debug_data(buffer, len, false, safe_line);
+				escape_libcurl_debug_data(buffer, log_len, false, safe_line);
 			}
 			else
 			{
@@ -725,7 +725,7 @@ int HttpOpRequest::debugCallback(CURL * handle, curl_infotype info, char * buffe
 			logit = true;
 			if (op->mTracing >= HTTP_TRACE_CURL_BODIES)
 			{
-				escape_libcurl_debug_data(buffer, len, false, safe_line);
+				escape_libcurl_debug_data(buffer, log_len, false, safe_line);
 			}
 			else
 			{
diff --git a/indra/llcorehttp/_httppolicy.cpp b/indra/llcorehttp/_httppolicy.cpp
index 76c1e22431..014bd37e2e 100644
--- a/indra/llcorehttp/_httppolicy.cpp
+++ b/indra/llcorehttp/_httppolicy.cpp
@@ -4,7 +4,7 @@
  *
  * $LicenseInfo:firstyear=2012&license=viewerlgpl$
  * Second Life Viewer Source Code
- * Copyright (C) 2012, Linden Research, Inc.
+ * Copyright (C) 2012-2013, Linden Research, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -319,33 +319,13 @@ bool HttpPolicy::cancel(HttpHandle handle)
 
 bool HttpPolicy::stageAfterCompletion(HttpOpRequest * op)
 {
-	static const HttpStatus cant_connect(HttpStatus::EXT_CURL_EASY, CURLE_COULDNT_CONNECT);
-	static const HttpStatus cant_res_proxy(HttpStatus::EXT_CURL_EASY, CURLE_COULDNT_RESOLVE_PROXY);
-	static const HttpStatus cant_res_host(HttpStatus::EXT_CURL_EASY, CURLE_COULDNT_RESOLVE_HOST);
-	static const HttpStatus send_error(HttpStatus::EXT_CURL_EASY, CURLE_SEND_ERROR);
-	static const HttpStatus recv_error(HttpStatus::EXT_CURL_EASY, CURLE_RECV_ERROR);
-	static const HttpStatus upload_failed(HttpStatus::EXT_CURL_EASY, CURLE_UPLOAD_FAILED);
-	static const HttpStatus op_timedout(HttpStatus::EXT_CURL_EASY, CURLE_OPERATION_TIMEDOUT);
-	static const HttpStatus post_error(HttpStatus::EXT_CURL_EASY, CURLE_HTTP_POST_ERROR);
-
 	// Retry or finalize
 	if (! op->mStatus)
 	{
-		// If this failed, we might want to retry.  Have to inspect
-		// the status a little more deeply for those reasons worth retrying...
-		if (op->mPolicyRetries < op->mPolicyRetryLimit &&
-			((op->mStatus.isHttpStatus() && op->mStatus.mType >= 499 && op->mStatus.mType <= 599) ||
-			 cant_connect == op->mStatus ||
-			 cant_res_proxy == op->mStatus ||
-			 cant_res_host == op->mStatus ||
-			 send_error == op->mStatus ||
-			 recv_error == op->mStatus ||
-			 upload_failed == op->mStatus ||
-			 op_timedout == op->mStatus ||
-			 post_error == op->mStatus))
+		// If this failed, we might want to retry.
+		if (op->mPolicyRetries < op->mPolicyRetryLimit && op->mStatus.isRetryable())
 		{
-			// Okay, worth a retry.  We include 499 in this test as
-			// it's the old 'who knows?' error from many grid services...
+			// Okay, worth a retry.
 			retryOp(op);
 			return true;				// still active/ready
 		}
diff --git a/indra/llcorehttp/httpcommon.cpp b/indra/llcorehttp/httpcommon.cpp
index f2fcbf77a3..0738760763 100644
--- a/indra/llcorehttp/httpcommon.cpp
+++ b/indra/llcorehttp/httpcommon.cpp
@@ -4,7 +4,7 @@
  *
  * $LicenseInfo:firstyear=2012&license=viewerlgpl$
  * Second Life Viewer Source Code
- * Copyright (C) 2012, Linden Research, Inc.
+ * Copyright (C) 2012-2013, Linden Research, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -116,6 +116,7 @@ std::string HttpStatus::toString() const
 			{ 415, "Unsupported Media Type" },
 			{ 416, "Requested range not satisfiable" },
 			{ 417, "Expectation Failed" },
+			{ 499, "Linden Catch-All" },
 			{ 500, "Internal Server Error" },
 			{ 501, "Not Implemented" },
 			{ 502, "Bad Gateway" },
@@ -174,6 +175,37 @@ std::string HttpStatus::toString() const
 	}
 	return std::string("Unknown error");
 }
+
+
+// Pass true on statuses that might actually be cleared by a
+// retry.  Library failures, calling problems, etc. aren't
+// going to be fixed by squirting bits all over the Net.
+bool HttpStatus::isRetryable() const
+{
+	static const HttpStatus cant_connect(HttpStatus::EXT_CURL_EASY, CURLE_COULDNT_CONNECT);
+	static const HttpStatus cant_res_proxy(HttpStatus::EXT_CURL_EASY, CURLE_COULDNT_RESOLVE_PROXY);
+	static const HttpStatus cant_res_host(HttpStatus::EXT_CURL_EASY, CURLE_COULDNT_RESOLVE_HOST);
+	static const HttpStatus send_error(HttpStatus::EXT_CURL_EASY, CURLE_SEND_ERROR);
+	static const HttpStatus recv_error(HttpStatus::EXT_CURL_EASY, CURLE_RECV_ERROR);
+	static const HttpStatus upload_failed(HttpStatus::EXT_CURL_EASY, CURLE_UPLOAD_FAILED);
+	static const HttpStatus op_timedout(HttpStatus::EXT_CURL_EASY, CURLE_OPERATION_TIMEDOUT);
+	static const HttpStatus post_error(HttpStatus::EXT_CURL_EASY, CURLE_HTTP_POST_ERROR);
+	static const HttpStatus partial_file(HttpStatus::EXT_CURL_EASY, CURLE_PARTIAL_FILE);
+	static const HttpStatus inv_cont_range(HttpStatus::LLCORE, HE_INV_CONTENT_RANGE_HDR);
+
+	return ((isHttpStatus() && mType >= 499 && mType <= 599) ||	// Include special 499 in retryables
+			*this == cant_connect ||	// Connection reset/endpoint problems
+			*this == cant_res_proxy ||	// DNS problems
+			*this == cant_res_host ||	// DNS problems
+			*this == send_error ||		// General socket problems 
+			*this == recv_error ||		// General socket problems 
+			*this == upload_failed ||	// Transport problem
+			*this == op_timedout ||		// Timer expired
+			*this == post_error ||		// Transport problem
+			*this == partial_file ||	// Data inconsistency in response
+			*this == inv_cont_range);	// Short data read disagrees with content-range
+}
+
 		
 } // end namespace LLCore
 
diff --git a/indra/llcorehttp/httpcommon.h b/indra/llcorehttp/httpcommon.h
index c0d4ec5aad..41fb5164cf 100644
--- a/indra/llcorehttp/httpcommon.h
+++ b/indra/llcorehttp/httpcommon.h
@@ -4,7 +4,7 @@
  *
  * $LicenseInfo:firstyear=2012&license=viewerlgpl$
  * Second Life Viewer Source Code
- * Copyright (C) 2012, Linden Research, Inc.
+ * Copyright (C) 2012-2013, Linden Research, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -303,6 +303,12 @@ struct HttpStatus
 	{
 		return 	mType >= type_enum_t(100) && mType <= type_enum_t(999);
 	}
+
+	/// Returns true if the status is one that will be retried
+	/// internally.  Provided for external consumption for cases
+	/// where that logic needs to be replicated.  Only applies
+	/// to failed statuses, successful statuses will return false.
+	bool isRetryable() const;
 	
 }; // end struct HttpStatus
 
diff --git a/indra/llcorehttp/tests/test_httprequest.hpp b/indra/llcorehttp/tests/test_httprequest.hpp
index 16f39845bb..ff84b04070 100644
--- a/indra/llcorehttp/tests/test_httprequest.hpp
+++ b/indra/llcorehttp/tests/test_httprequest.hpp
@@ -2670,10 +2670,15 @@ void HttpRequestTestObjectType::test<22>()
 	mMemTotal = GetMemTotal();
 	mHandlerCalls = 0;
 
+	HttpOptions * options = NULL;
 	HttpRequest * req = NULL;
 
 	try
 	{
+		// options set
+		options = new HttpOptions();
+		options->setRetries(1);			// Partial_File is retryable and can timeout in here
+
 		// Get singletons created
 		HttpRequest::createService();
 		
@@ -2699,7 +2704,7 @@ void HttpRequestTestObjectType::test<22>()
 														 url_base + buffer,
 														 0,
 														 25,
-														 NULL,
+														 options,
 														 NULL,
 														 &handler);
 			ensure("Valid handle returned for ranged request", handle != LLCORE_HTTP_HANDLE_INVALID);
@@ -2707,18 +2712,19 @@ void HttpRequestTestObjectType::test<22>()
 		
 		// Run the notification pump.
 		int count(0);
-		int limit(10);
+		int limit(30);
 		while (count++ < limit && mHandlerCalls < test_count)
 		{
 			req->update(1000000);
 			usleep(100000);
 		}
-		ensure("Request executed in reasonable time", count < limit);
-		ensure("One handler invocation for each request", mHandlerCalls == test_count);
+		ensure("Request executed in reasonable time - ms1", count < limit);
+		ensure("One handler invocation for each request - ms1", mHandlerCalls == test_count);
 
 		// ======================================
 		// Issue bug2295 GETs that will get a libcurl 18 (PARTIAL_FILE)
 		// ======================================
+		mHandlerCalls = 0;
 		mStatus = HttpStatus(HttpStatus::EXT_CURL_EASY, CURLE_PARTIAL_FILE);
 		static const int test2_count(1);
 		for (int i(0); i < test2_count; ++i)
@@ -2730,7 +2736,39 @@ void HttpRequestTestObjectType::test<22>()
 														 url_base + buffer,
 														 0,
 														 25,
+														 options,
 														 NULL,
+														 &handler);
+			ensure("Valid handle returned for ranged request", handle != LLCORE_HTTP_HANDLE_INVALID);
+		}
+		
+		// Run the notification pump.
+		count = 0;
+		limit = 30;
+		while (count++ < limit && mHandlerCalls < test2_count)
+		{
+			req->update(1000000);
+			usleep(100000);
+		}
+		ensure("Request executed in reasonable time - ms2", count < limit);
+		ensure("One handler invocation for each request - ms2", mHandlerCalls == test2_count);
+
+		// ======================================
+		// Issue bug2295 GETs that will get an llcorehttp HE_INV_CONTENT_RANGE_HDR status
+		// ======================================
+		mHandlerCalls = 0;
+		mStatus = HttpStatus(HttpStatus::LLCORE, HE_INV_CONTENT_RANGE_HDR);
+		static const int test3_count(1);
+		for (int i(0); i < test3_count; ++i)
+		{
+			char buffer[128];
+			sprintf(buffer, "/bug2295/inv_cont_range/%d/", i);
+			HttpHandle handle = req->requestGetByteRange(HttpRequest::DEFAULT_POLICY_ID,
+														 0U,
+														 url_base + buffer,
+														 0,
+														 25,
+														 options,
 														 NULL,
 														 &handler);
 			ensure("Valid handle returned for ranged request", handle != LLCORE_HTTP_HANDLE_INVALID);
@@ -2738,32 +2776,33 @@ void HttpRequestTestObjectType::test<22>()
 		
 		// Run the notification pump.
 		count = 0;
-		limit = 10;
-		while (count++ < limit && mHandlerCalls < (test_count + test2_count))
+		limit = 30;
+		while (count++ < limit && mHandlerCalls < test3_count)
 		{
 			req->update(1000000);
 			usleep(100000);
 		}
-		ensure("Request executed in reasonable time", count < limit);
-		ensure("One handler invocation for each request", mHandlerCalls == (test_count + test2_count));
+		ensure("Request executed in reasonable time - ms3", count < limit);
+		ensure("One handler invocation for each request - ms3", mHandlerCalls == test3_count);
 
 		// ======================================
 		// Okay, request a shutdown of the servicing thread
 		// ======================================
 		mStatus = HttpStatus();
+		mHandlerCalls = 0;
 		HttpHandle handle = req->requestStopThread(&handler);
 		ensure("Valid handle returned for second request", handle != LLCORE_HTTP_HANDLE_INVALID);
 	
 		// Run the notification pump again
 		count = 0;
-		limit = 10;
-		while (count++ < limit && mHandlerCalls < (test_count + test2_count + 1))
+		limit = 20;
+		while (count++ < limit && mHandlerCalls < 1)
 		{
 			req->update(1000000);
 			usleep(100000);
 		}
-		ensure("Second request executed in reasonable time", count < limit);
-		ensure("Second handler invocation", mHandlerCalls == (test_count + test2_count + 1));
+		ensure("Shutdown request executed in reasonable time", count < limit);
+		ensure("Shutdown handler invocation", mHandlerCalls == 1);
 
 		// See that we actually shutdown the thread
 		count = 0;
@@ -2773,7 +2812,14 @@ void HttpRequestTestObjectType::test<22>()
 			usleep(100000);
 		}
 		ensure("Thread actually stopped running", HttpService::isStopped());
-	
+
+		// release options
+		if (options)
+		{
+			options->release();
+			options = NULL;
+		}
+		
 		// release the request object
 		delete req;
 		req = NULL;
@@ -2781,8 +2827,6 @@ void HttpRequestTestObjectType::test<22>()
 		// Shut down service
 		HttpRequest::destroyService();
 	
-		ensure("4 + 1 handler calls on the way out", (test_count + test2_count + 1) == mHandlerCalls);
-
 #if defined(WIN32)
 		// Can only do this memory test on Windows.  On other platforms,
 		// the LL logging system holds on to memory and produces what looks
diff --git a/indra/llcorehttp/tests/test_llcorehttp_peer.py b/indra/llcorehttp/tests/test_llcorehttp_peer.py
index 7f8f765366..8796ae57c7 100644
--- a/indra/llcorehttp/tests/test_llcorehttp_peer.py
+++ b/indra/llcorehttp/tests/test_llcorehttp_peer.py
@@ -35,6 +35,10 @@ import time
 import select
 import getopt
 from threading import Thread
+try:
+    from cStringIO import StringIO
+except ImportError:
+    from StringIO import StringIO
 from BaseHTTPServer import HTTPServer, BaseHTTPRequestHandler
 from SocketServer import ThreadingMixIn
 
@@ -64,6 +68,7 @@ class TestHTTPRequestHandler(BaseHTTPRequestHandler):
     -- '/bug2295/00000018/0/'  Generates PARTIAL_FILE (18) error in libcurl.
                            "Content-Range: bytes 0-75/2983",
                            "Content-Length: 76"
+    -- '/bug2295/inv_cont_range/0/'  Generates HE_INVALID_CONTENT_RANGE error in llcorehttp.
 
     Some combinations make no sense, there's no effort to protect
     you from that.
@@ -150,6 +155,7 @@ class TestHTTPRequestHandler(BaseHTTPRequestHandler):
             # appear in the body without actually getting the body.
             # Library needs to defend against this case.
             #
+            body = None
             if "/bug2295/0/" in self.path:
                 self.send_response(206)
                 self.send_header("Content-Range", "bytes 0-75/2983")
@@ -164,6 +170,10 @@ class TestHTTPRequestHandler(BaseHTTPRequestHandler):
                 self.send_response(206)
                 self.send_header("Content-Range", "bytes 0-75/2983")
                 self.send_header("Content-Length", "76")
+            elif "/bug2295/inv_cont_range/0/" in self.path:
+                self.send_response(206)
+                self.send_header("Content-Range", "bytes 0-75/2983")
+                body = "Some text, but not enough."
             else:
                 # Unknown request
                 self.send_response(400)
@@ -171,7 +181,8 @@ class TestHTTPRequestHandler(BaseHTTPRequestHandler):
                 self.reflect_headers()
             self.send_header("Content-type", "text/plain")
             self.end_headers()
-            # No data
+            if body:
+                self.wfile.write(body)
         else:
             # Normal response path
             data = data.copy()          # we're going to modify